Re: [libvirt] [PATCH 0/2] Fix interface state transitions logic
On 12/16/2013 04:32 PM, Michal Privoznik wrote: > On 11.12.2013 10:16, Michal Privoznik wrote: >> Right now it's possible to start an interface that is already running, or >> destroy an interface multiple times. Such state transitions are not allowed >> and >> we check for such cases explicitly in other areas like qemu driver. >> >> Michal Privoznik (2): >> interface: Introduce netcfInterfaceObjIsActive >> interface: Take interface status into account when starting and >> destroying >> >> src/interface/interface_backend_netcf.c | 77 >> +++-- >> 1 file changed, 53 insertions(+), 24 deletions(-) >> > So it seems to me like we have agreement that the current state > transitions should be prohibited. So I guess absence of protest is considered agreement? :-) Yeah, I suppose I can live with it. Dan does have a point that the current situation (although it is reproducing the initscripts behavior) is overloading a single API with multiple behaviors. > If that's the case, can somebody > please review the patches? Thanks! I'll look through them now. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix interface state transitions logic
On 11.12.2013 10:16, Michal Privoznik wrote: > Right now it's possible to start an interface that is already running, or > destroy an interface multiple times. Such state transitions are not allowed > and > we check for such cases explicitly in other areas like qemu driver. > > Michal Privoznik (2): > interface: Introduce netcfInterfaceObjIsActive > interface: Take interface status into account when starting and > destroying > > src/interface/interface_backend_netcf.c | 77 > +++-- > 1 file changed, 53 insertions(+), 24 deletions(-) > So it seems to me like we have agreement that the current state transitions should be prohibited. If that's the case, can somebody please review the patches? Thanks! Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix interface state transitions logic
On Thu, Dec 12, 2013 at 07:54:34AM -0700, Eric Blake wrote: > On 12/12/2013 07:41 AM, Daniel P. Berrange wrote: > > > > The usage you describe here is not within the scope of the > > virInterfaceCreate() API IMHO. If we want users to have the > > ability to "re start" an interface without taking it offline > > first, then we should add another API that explicitly supports > > that use case. > > Or even a flag to the existing argument that says to restart if already > running, where the normal case of not having the flag is an error if > already running. > > > Overloading a single virInterfaceCreate to support two different use > > cases puts applications in an impossible position if they *want* to > > see an error from attempting to start an already active interface. > > Overloading without the use of a flag bit, that is. In this scenario I'd prefer a separate API. Flags are good when they are tweaking some aspect of behaviour but leaving the overall semantics of the method intact. A virInterfaceCreate API call is a lifecycle operation, which should result in a lifecycle change (and event emission) upon success. Just doing a refresh of the config isn't a lifecycle change and would not be expected to trigger any lifecycle events either. This is different enough that I think it would belong in a separate API. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix interface state transitions logic
On 12/12/2013 07:41 AM, Daniel P. Berrange wrote: > > The usage you describe here is not within the scope of the > virInterfaceCreate() API IMHO. If we want users to have the > ability to "re start" an interface without taking it offline > first, then we should add another API that explicitly supports > that use case. Or even a flag to the existing argument that says to restart if already running, where the normal case of not having the flag is an error if already running. > Overloading a single virInterfaceCreate to support two different use > cases puts applications in an impossible position if they *want* to > see an error from attempting to start an already active interface. Overloading without the use of a flag bit, that is. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix interface state transitions logic
On Thu, Dec 12, 2013 at 12:23:48PM +0200, Laine Stump wrote: > On 12/11/2013 11:16 AM, Michal Privoznik wrote: > > Right now it's possible to start an interface that is already running, or > > destroy an interface multiple times. Such state transitions are not allowed > > and > > we check for such cases explicitly in other areas like qemu driver. > > I recall that someone brought up this issue before, and we didn't change > the code. If I remember correctly, the reason was that the > virInterfaceCreate() API is basically just a wrapper around /sbin/ifup, > and /sbin/ifup can be called (and returns success after doing something > useful - see below) if the interface is already up. > > Why would you want to do that? Well, calling ifup on an interface that > is already up causes it to be "re-started", renewing its IP (and other) > configuration from any changes that have been made, *without* needing to > ifdown the interface in the process (and completely losing network > connectivity, possibly making it impossible for the program calling the > API to re-start the interface). (It's not directly applicable in this > case, but when a bridge device is added, subsuming an existing ethernet, > we rely on allowing virInterfaceCreate() to be called for the new bridge > device even though the subordinate ethernet is already up; this permits > us to add a bridge to the host's config without losing network > connectivity.) The usage you describe here is not within the scope of the virInterfaceCreate() API IMHO. If we want users to have the ability to "re start" an interface without taking it offline first, then we should add another API that explicitly supports that use case. > I'm not convinced that it's a bad thing that virInterfaceCreate/Destroy > can, by definition, be called when the device is already in the desired > state, and wouldn't want to end up with other unforeseen problems just > because we disabled it. Overloading a single virInterfaceCreate to support two different use cases puts applications in an impossible position if they *want* to see an error from attempting to start an already active interface. They can't just check virInterfaceIsActive before calling the virInterfaceCreate API because that is racy from the apps POV. If we explicitly have separate APIs for starting vs refreshing the config then we can cleanly support all use scenarios. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix interface state transitions logic
On 12.12.2013 11:23, Laine Stump wrote: > On 12/11/2013 11:16 AM, Michal Privoznik wrote: >> Right now it's possible to start an interface that is already running, or >> destroy an interface multiple times. Such state transitions are not allowed >> and >> we check for such cases explicitly in other areas like qemu driver. > > I recall that someone brought up this issue before, and we didn't change > the code. If I remember correctly, the reason was that the > virInterfaceCreate() API is basically just a wrapper around /sbin/ifup, > and /sbin/ifup can be called (and returns success after doing something > useful - see below) if the interface is already up. > > Why would you want to do that? Well, calling ifup on an interface that > is already up causes it to be "re-started", renewing its IP (and other) > configuration from any changes that have been made, *without* needing to > ifdown the interface in the process (and completely losing network > connectivity, possibly making it impossible for the program calling the > API to re-start the interface). (It's not directly applicable in this > case, but when a bridge device is added, subsuming an existing ethernet, > we rely on allowing virInterfaceCreate() to be called for the new bridge > device even though the subordinate ethernet is already up; this permits > us to add a bridge to the host's config without losing network > connectivity.) > > I'm not convinced that it's a bad thing that virInterfaceCreate/Destroy > can, by definition, be called when the device is already in the desired > state, and wouldn't want to end up with other unforeseen problems just > because we disabled it. > Well, by the same token virDomainCreate() called against a running domain should impersonate all the changes made to config, e.g. (un-)plug devices, change VNC/SPICE listen address, etc. But it's not doing that. Anyway, I can live with status quo. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] Fix interface state transitions logic
On 12/11/2013 11:16 AM, Michal Privoznik wrote: > Right now it's possible to start an interface that is already running, or > destroy an interface multiple times. Such state transitions are not allowed > and > we check for such cases explicitly in other areas like qemu driver. I recall that someone brought up this issue before, and we didn't change the code. If I remember correctly, the reason was that the virInterfaceCreate() API is basically just a wrapper around /sbin/ifup, and /sbin/ifup can be called (and returns success after doing something useful - see below) if the interface is already up. Why would you want to do that? Well, calling ifup on an interface that is already up causes it to be "re-started", renewing its IP (and other) configuration from any changes that have been made, *without* needing to ifdown the interface in the process (and completely losing network connectivity, possibly making it impossible for the program calling the API to re-start the interface). (It's not directly applicable in this case, but when a bridge device is added, subsuming an existing ethernet, we rely on allowing virInterfaceCreate() to be called for the new bridge device even though the subordinate ethernet is already up; this permits us to add a bridge to the host's config without losing network connectivity.) I'm not convinced that it's a bad thing that virInterfaceCreate/Destroy can, by definition, be called when the device is already in the desired state, and wouldn't want to end up with other unforeseen problems just because we disabled it. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/2] Fix interface state transitions logic
Right now it's possible to start an interface that is already running, or destroy an interface multiple times. Such state transitions are not allowed and we check for such cases explicitly in other areas like qemu driver. Michal Privoznik (2): interface: Introduce netcfInterfaceObjIsActive interface: Take interface status into account when starting and destroying src/interface/interface_backend_netcf.c | 77 +++-- 1 file changed, 53 insertions(+), 24 deletions(-) -- 1.8.5.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
