[libvirt] [PATCH] SNMP: include inactive domains in guest table
Hi, I've written a patch to include inactive domains in the guest table available via the libvirt-snmp subagent. This makes it possible to start domains via SNMP (a state transition which AFAICT was previously impossible, because inactive domains never appeared in the SNMP result). The code now collects the IDs of running domains along with the names of declared (but not running) domains and normalizes them into a list of domain pointers, which is then processed by the preexisting SNMP container row allocation code. The change was fairly straightforward, but certainly there might be Good Reasons it wasn't already implemented. As part of this change, the other (noteworthy) change I made was to move calls to virDomainFree() to the end of libvirtSnmpLoadGuests() since I have to free all of the used domain pointers there anyway. Some other issues which may need to be addressed: - This will be a breaking change for anyone who assumes that the old behavior is still in place (i.e., that the guest table only includes non-shutdown domains). However, it might not be *that* bad because the domain rows already included a 'state' that one could already filter on. - I don't know if there exists other code in libvirt-snmp that assumes inactive domains can't appear in the table. - I have not tested the inactive -> active state transition (because I don't want to enable writable SNMP OIDs on my server). Please let me know if there is anything further I need to do to make the patch acceptable. Thanks for libvirt-snmp! -- Jonathan Daugherty Software Engineer Galois, Inc.
On 05/30/2012 04:04 PM, Jonathan Daugherty wrote:
Hi,
I've written a patch to include inactive domains in the guest table available via the libvirt-snmp subagent. This makes it possible to start domains via SNMP (a state transition which AFAICT was previously impossible, because inactive domains never appeared in the SNMP result).
Sounds like a worthwhile change at the high level.
The code now collects the IDs of running domains along with the names of declared (but not running) domains and normalizes them into a list of domain pointers, which is then processed by the preexisting SNMP container row allocation code. The change was fairly straightforward, but certainly there might be Good Reasons it wasn't already implemented.
Even better would be to require libvirt 0.9.13 and use the proposed virConnectListAllDomains() API to get all domains in one shot, as the approach you have used is inherently racy due to the flaws in the older APIs (which reminds me, Peter is still working on posting a v2 of that series).
- This will be a breaking change for anyone who assumes that the old behavior is still in place (i.e., that the guest table only includes non-shutdown domains). However, it might not be *that* bad because the domain rows already included a 'state' that one could already filter on.
I'm not familiar enough with libvirt-snmp to say whether this is important, or to even know how the current bindings work. Is this something where you might be better served by instead adding new libvirt-snmp bindings to the existing virConnectListDefinedDomains and upcoming virConnectListAllDomains, instead of changing the behavior of the existing query method which is currently bound to virConnectListDomains?
Please let me know if there is anything further I need to do to make the patch acceptable.
I'll have to defer that to people more familiar with libvirt-snmp. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Even better would be to require libvirt 0.9.13 and use the proposed virConnectListAllDomains() API to get all domains in one shot, as the approach you have used is inherently racy due to the flaws in the older APIs (which reminds me, Peter is still working on posting a v2 of that series).
That would be great! In my case, I'm running Centos 6.2, and it uses libvirt 0.9.4 at the moment.
- This will be a breaking change for anyone who assumes that the old behavior is still in place (i.e., that the guest table only includes non-shutdown domains). However, it might not be *that* bad because the domain rows already included a 'state' that one could already filter on.
I'm not familiar enough with libvirt-snmp to say whether this is important, or to even know how the current bindings work. Is this something where you might be better served by instead adding new libvirt-snmp bindings to the existing virConnectListDefinedDomains and upcoming virConnectListAllDomains, instead of changing the behavior of the existing query method which is currently bound to virConnectListDomains?
I don't quite understand your question. To clarify: the issue I am raising here is that anyone who issues an "snmpwalk" command will now get (potentially) more domain entries in the result than they expected. Even if those entries have the proper state (i.e., not running), if they wrote code under the assumption that all of the guest domains in the SNMP result would be running (due to the behavior prior to my patch), they'd end up with a bug. This is an application behavior change in the "libvirtMib_subagent" program (which is the chief result of building libvirt-snmp). Does that clarify? Thanks! -- Jonathan Daugherty Software Engineer Galois, Inc.
On 31.05.2012 00:04, Jonathan Daugherty wrote:
Hi,
I've written a patch to include inactive domains in the guest table available via the libvirt-snmp subagent. This makes it possible to start domains via SNMP (a state transition which AFAICT was previously impossible, because inactive domains never appeared in the SNMP result).
The code now collects the IDs of running domains along with the names of declared (but not running) domains and normalizes them into a list of domain pointers, which is then processed by the preexisting SNMP container row allocation code. The change was fairly straightforward, but certainly there might be Good Reasons it wasn't already implemented.
As part of this change, the other (noteworthy) change I made was to move calls to virDomainFree() to the end of libvirtSnmpLoadGuests() since I have to free all of the used domain pointers there anyway.
Some other issues which may need to be addressed:
- This will be a breaking change for anyone who assumes that the old behavior is still in place (i.e., that the guest table only includes non-shutdown domains). However, it might not be *that* bad because the domain rows already included a 'state' that one could already filter on.
- I don't know if there exists other code in libvirt-snmp that assumes inactive domains can't appear in the table.
- I have not tested the inactive -> active state transition (because I don't want to enable writable SNMP OIDs on my server).
Please let me know if there is anything further I need to do to make the patch acceptable.
Thanks for libvirt-snmp!
Firstly, have you known that you are the first person outside RH to post patch for libvirt-snmp? It's awesome. Secondly, as Eric replied, I'd wait for the new API as well, so we can do an atomic list. Therefore ACK to the idea. That's for sure. So are you comfortable with this resolution? Michal
Firstly, have you known that you are the first person outside RH to post patch for libvirt-snmp? It's awesome.
Neat!
Secondly, as Eric replied, I'd wait for the new API as well, so we can do an atomic list. Therefore ACK to the idea. That's for sure. So are you comfortable with this resolution?
Right now it doesn't make much difference to me which approach we pick because it's probably going to be a long time before any changes make it all the way into my distro RPM. :) -- Jonathan Daugherty Software Engineer Galois, Inc.
participants (3)
-
Eric Blake -
Jonathan Daugherty -
Michal Privoznik