[libvirt] 答复： Re: [PATCH] virsh: add transient interfaces check for iface-unbridge

[Lin Ma]

>>> 在 1:02 的 2014/12/18 上，在讯息 <5491B730.5050804 at laine.org> 中，Laine Stump
<laine at laine.org> 写入：
> (BTW, I assume that you are running suse, and using netcf behind the
> iface-* commands. I didn't realize that the suse port of netcf was being
> officially used anywhere, as there hasn't been any suse-specific
> modification to upstream netcf since the suse port was originally added
> in 2011. If there are downstream patches, it would be great to have them
> upstream as well.)
> 
> On 12/17/2014 05:34 AM, Lin Ma wrote:
>> iface-unbridge(netcf interface backend) checks multiple interfaces
>> attaching based on static configuration.
>> If guests interfaces(says tun/tap devices) are attaching to the bridge,
>> iface-unbridge doesn't check them, It causes the bridge is removed
>> although it's in use by other guests.
>>
>> Please refer to:
>> https://bugzilla.suse.com/show_bug.cgi?id=813117 
> 
> In my opinion, if we're going to do this, then we should also modify
> virsh net-destroy to not allow shutting down a network if there are
> guests still using it - it's essentially the same thing. This isn't a
> simple bug that has a definite simple fix, it requires agreement on
> philosophy. For a very long time virsh has allowed removing networks
> (net-destroy) even when a guest is connected (just as it permits "virsh
> destroy" of a guest that hasn't cleanly shutdown its OS); I don't know
> if this was originally an omission, if it was done because there was
> originally no simple way to check (the reporting of number of
> connections in the network status xml is a fairly recent addition), or
> if it was done on purpose in order to not prevent something that someone
> may legitimately want to do, but virsh iface-unbridge was written to
> have the same laissez faire attitude about guest connections.
> 
> The existing check for multiple *configured* attached devices though, is
> there because I wanted iface-unbridge to be able to undo exactly what
> iface-bridge was doing - put a single ethernet interface behind a
> bridge. If there are multiple configured interfaces, that can't be done
> because there is no way to know which of those two interfaces should get
> the IP configuration from the bridge device (and if there are *0*
> configured interfaces, the IP config from the bridge would be lost).
> 
> 
> On 12/17/2014 10:48 AM, Lin Ma wrote:
>>
>>  
>>
>> >>> Michal Privoznik <mprivozn at redhat.com> 2014-12-17 下午 20:34 >>>
>> >On 17.12.2014 11:34, Lin Ma wrote:
>> >> iface-unbridge(netcf interface backend) checks multiple interfaces
>> >> attaching based on static configuration.
>> >> If guests interfaces(says tun/tap devices) are attaching to the bridge,
>> >> iface-unbridge doesn't check them, It causes the bridge is removed
>> >> although it's in use by other guests.
>> >>
>> >> Please refer to:
>> >> https://bugzilla.suse.com/show_bug.cgi?id=813117 
>> >
>> > The bug is not publicly viewable. We tend to not put the BZ URL in the
>> > commit message in that case.
>>
>> sorry about that, the URL will be removed, Thanks.
> 
> 
> Even better - if there is no sensitive information in the BZ, perhaps
> you could make it public (or maybe the BZ can be sanitized to mark only
> the sensitive parts as private).
> 
> 
>>
>> >> @@ -1103,6 +1103,21 @@ cmdInterfaceUnbridge(vshControl *ctl, const
>> vshCmd *cmd)
>> >> goto cleanup;
>> >> }
>> >>
>> >> + /* verify whether there is any transient interface attached to
>> bridge. */
>> >> + if (!(br_xml_transient_if = virInterfaceGetXMLDesc(br_handle, 0)))
>> >> + goto cleanup;
>> >> + if (!(xml_doc_transient_if =
>> virXMLParseStringCtxt(br_xml_transient_if,
>> >> + _("(bridge interface definition)"),
>> >> + &ctxt_transient_if))) {
>> >> + vshError(ctl, _("Failed to parse configuration of %s"), br_name);
>> >> + goto cleanup;
>> >> + }
>> >> +
>> >> + if (virXPathNode("./bridge/interface[2]", ctxt_transient_if) !=
>> NULL) {
>> >> + vshError(ctl, "%s", _("The bridge is in use by transient
>> interfaces"));
>> >> + goto cleanup;
>> >> + }
>> >> +
>> >
>> > This effectively copies the code just a few lines above (not to be seen
>> > in this context though). The only difference is, that your code gest
>> > LIVE bridge XML, while pre-exisitng code dumps INACTIVE XML. Can we turn
>> > already existing code into checking LIVE XML instead?
> 
> 
> No. That is there for a different reason, and is checking something
> different (see above).
> 
> 
>> It feels more
>> > appropriate too. Live bridge can have only one interface plugged in (due
>> > to hot-unplugging) regardless of what INACTIVE XML says.
> 
> 
> The interfaces listed in the config are the ethernet/bond/vlan
> interfaces that are configured to be attached to the bridge when it is
> created. Any interfaces beyond that are most likely tap devices
> connecting guests (although that isn't necessarily the case).
> 
> 
>> Actually, at the beginning of writing this patch, I tended to turn
>> already existing code
>> into checking LIVE XML instead, But if I do so, it can't deal with
>> this extreme circumstance:
>> says no any physical interface attached to the bridge, But one guest
>> tap backend is
>> attaching to this bridge. The bridge will be removed in this case
>> instead of reporting "No
>> interface attached to bridge".
> 
> 
> Again, if there is no configured interface that is attached to the
> bridge, there is no place to put the bridge's IP config, so executing
> "iface-unbridge" would permanently lose that information, and must be
> avoided.
> 
> 
>>
>> Now I thought about it, no body configures the bridge and guest
>> interface like that,
>> that extreme circumstance is meaningless and useless scenario, So I
>> agreed with your idea.
>>
>> any else suggestions?
> 
> 
> I think the reasons for those two checks are being misunderstood. When
> there is no configured interface attached to the bridge, removing the
> bridge will lose the IP config; where there are multiple configured
> interfaces attached to the bridge, the desired outcome is ambiguous. The
> live XML gives us different information.
> 
> My opinion is that this bug report should be closed as NOTABUG (or virsh
> net-destroy should also be changed to be prohibited when guests are
> connected, 
In my understanding, virsh net-destroy should be changed as well and I will include it in
patch.

> but it's possible that could break existing scripts - I
> remember that before "virsh net-update" existed, when somebody wanted to
> change a running network, they would make the changes to the network,
> net-destroy and net-start it, then go through all the running domains,
> detaching and re-attaching all interfaces using that network. It's very
> likely there are still scripts like that hanging around)
What if I add a "--force" option for virsh net-destroy? then those existing scrips can work
without too much changes.

Thanks,
Lin