[libvirt] [PATCH 0/2] Fix updating domain live XML
Erik Skultety (2): qemu: Fix updating bandwidth limits in live XML qemu: Fix updating balloon period in live XML src/qemu/qemu_driver.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) -- 1.9.3
When trying to update bandwidth limits on a running domain, limits get updated in our internal structures, however XML parser reads bandwidth limits from network 'actual' definition. Commiting this patch it is now available to update bandwidth 'actual' definition as well, thus updating domain runtime XML --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 702d3cc..ede8880 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10028,7 +10028,19 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, } else { net->bandwidth = NULL; } + + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + virNetDevBandwidthFree(net->data.network.actual->bandwidth); + if (!net->bandwidth || + virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, + net->bandwidth) < 0) + net->data.network.actual->bandwidth = NULL; + } + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto cleanup; } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!persistentNet->bandwidth) { persistentNet->bandwidth = bandwidth; -- 1.9.3
On 09/22/2014 06:41 AM, Erik Skultety wrote:
When trying to update bandwidth limits on a running domain, limits get updated in our internal structures, however XML parser reads bandwidth limits from network 'actual' definition. Commiting this patch
s/Commiting/Committing
it is now available to update bandwidth 'actual' definition as well, thus updating domain runtime XML --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 702d3cc..ede8880 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10028,7 +10028,19 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, } else { net->bandwidth = NULL; } + + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + virNetDevBandwidthFree(net->data.network.actual->bandwidth);
This will set net->data.network.actual->bandwidth to NULL It's also remove it when net->bandwidth == NULL thus causing actual to be lost, which it doesn't seem is desired, but perhaps it is. Gues
+ if (!net->bandwidth || + virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, + net->bandwidth) < 0) + net->data.network.actual->bandwidth = NULL;
Making this irrelevant, but I wonder if the < 0 here meant to do something else perhaps?
+ }
The above hunk needs some space formatting. Also since the virNetDevBandwidthCopy() has a if (!src) check, "(!net->bandwidth) ||" is unnecessary. if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virNetDevBandwidthFree(net->data.network.actual->bandwidth); if (virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, net->bandwidth) < 0) goto cleanup } Whether this is what is "expected" is perhaps something Laine can answer... John
+ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto cleanup; } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!persistentNet->bandwidth) { persistentNet->bandwidth = bandwidth;
On 10/01/2014 01:55 AM, John Ferlan wrote:
On 09/22/2014 06:41 AM, Erik Skultety wrote:
When trying to update bandwidth limits on a running domain, limits get updated in our internal structures, however XML parser reads bandwidth limits from network 'actual' definition. Commiting this patch
s/Commiting/Committing
it is now available to update bandwidth 'actual' definition as well, thus updating domain runtime XML --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 702d3cc..ede8880 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10028,7 +10028,19 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, } else { net->bandwidth = NULL; } + + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + virNetDevBandwidthFree(net->data.network.actual->bandwidth);
This will set net->data.network.actual->bandwidth to NULL
It's also remove it when net->bandwidth == NULL thus causing actual to be lost, which it doesn't seem is desired, but perhaps it is. Gues
+ if (!net->bandwidth || + virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, + net->bandwidth) < 0) + net->data.network.actual->bandwidth = NULL;
Making this irrelevant, but I wonder if the < 0 here meant to do something else perhaps?
+ }
The above hunk needs some space formatting. Also since the virNetDevBandwidthCopy() has a if (!src) check, "(!net->bandwidth) ||" is unnecessary.
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virNetDevBandwidthFree(net->data.network.actual->bandwidth); if (virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, net->bandwidth) < 0) goto cleanup }
Looks better, thanks Jon, but you'd still loose 'actual' in the hunk above, maybe add a check like if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && net->bandwidth) if you want to preserve 'actual' when net->bandwidth is NULL??
Whether this is what is "expected" is perhaps something Laine can answer...
John
+ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto cleanup; } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!persistentNet->bandwidth) { persistentNet->bandwidth = bandwidth;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Erik
On 10/01/2014 11:17 AM, Erik Skultety wrote:
On 10/01/2014 01:55 AM, John Ferlan wrote:
On 09/22/2014 06:41 AM, Erik Skultety wrote:
When trying to update bandwidth limits on a running domain, limits get updated in our internal structures, however XML parser reads bandwidth limits from network 'actual' definition. Commiting this patch
s/Commiting/Committing
it is now available to update bandwidth 'actual' definition as well, thus updating domain runtime XML --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 702d3cc..ede8880 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10028,7 +10028,19 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, } else { net->bandwidth = NULL; } + + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + virNetDevBandwidthFree(net->data.network.actual->bandwidth);
This will set net->data.network.actual->bandwidth to NULL
It's also remove it when net->bandwidth == NULL thus causing actual to be lost, which it doesn't seem is desired, but perhaps it is. Gues
+ if (!net->bandwidth || + virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, + net->bandwidth) < 0) + net->data.network.actual->bandwidth = NULL;
Making this irrelevant, but I wonder if the < 0 here meant to do something else perhaps?
+ }
The above hunk needs some space formatting. Also since the virNetDevBandwidthCopy() has a if (!src) check, "(!net->bandwidth) ||" is unnecessary.
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virNetDevBandwidthFree(net->data.network.actual->bandwidth); if (virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, net->bandwidth) < 0) goto cleanup }
Looks better, thanks Jon, but you'd still loose 'actual' in the hunk above, maybe add a check like
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && net->bandwidth)
if you want to preserve 'actual' when net->bandwidth is NULL??
Right - I thought about that too, but then I thought the purpose of making the change to actual was to copy what happened to net->bandwidth. Looking at the hunk just above: virNetDevBandwidthFree(net->bandwidth); if (newBandwidth->in || newBandwidth->out) { net->bandwidth = newBandwidth; newBandwidth = NULL; } else { net->bandwidth = NULL; } That says to me in this live path, we're freeing net->bandwidth and only replacing it with 'newBandwidth' if in/out were found. Thus, it seems reasonable that we'd want to remove the bandwidth from actual in this case as well which we wouldn't do if the the "&& net->bandwidth" was added to the condition. Also, upon further reflection the "net->data.network.actual->bandwidth = NULL;" after the virNetDevBandwidthFree() will be necessary since we're passing by value and not reference... John
Whether this is what is "expected" is perhaps something Laine can answer...
John
+ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto cleanup; } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!persistentNet->bandwidth) { persistentNet->bandwidth = bandwidth;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/01/2014 05:39 PM, John Ferlan wrote:
On 10/01/2014 11:17 AM, Erik Skultety wrote:
On 10/01/2014 01:55 AM, John Ferlan wrote:
On 09/22/2014 06:41 AM, Erik Skultety wrote:
When trying to update bandwidth limits on a running domain, limits get updated in our internal structures, however XML parser reads bandwidth limits from network 'actual' definition. Commiting this patch
s/Commiting/Committing
it is now available to update bandwidth 'actual' definition as well, thus updating domain runtime XML --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 702d3cc..ede8880 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10028,7 +10028,19 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, } else { net->bandwidth = NULL; } + + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + virNetDevBandwidthFree(net->data.network.actual->bandwidth);
This will set net->data.network.actual->bandwidth to NULL
It's also remove it when net->bandwidth == NULL thus causing actual to be lost, which it doesn't seem is desired, but perhaps it is. Gues
+ if (!net->bandwidth || + virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, + net->bandwidth) < 0) + net->data.network.actual->bandwidth = NULL;
Making this irrelevant, but I wonder if the < 0 here meant to do something else perhaps?
+ }
The above hunk needs some space formatting. Also since the virNetDevBandwidthCopy() has a if (!src) check, "(!net->bandwidth) ||" is unnecessary.
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virNetDevBandwidthFree(net->data.network.actual->bandwidth); if (virNetDevBandwidthCopy(&net->data.network.actual->bandwidth, net->bandwidth) < 0) goto cleanup }
Looks better, thanks Jon, but you'd still loose 'actual' in the hunk above, maybe add a check like
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && net->bandwidth)
if you want to preserve 'actual' when net->bandwidth is NULL??
Right - I thought about that too, but then I thought the purpose of making the change to actual was to copy what happened to net->bandwidth.
Looking at the hunk just above:
virNetDevBandwidthFree(net->bandwidth); if (newBandwidth->in || newBandwidth->out) { net->bandwidth = newBandwidth; newBandwidth = NULL; } else { net->bandwidth = NULL; }
That says to me in this live path, we're freeing net->bandwidth and only replacing it with 'newBandwidth' if in/out were found.
Thus, it seems reasonable that we'd want to remove the bandwidth from actual in this case as well which we wouldn't do if the the "&& net->bandwidth" was added to the condition.
Also, upon further reflection the "net->data.network.actual->bandwidth = NULL;" after the virNetDevBandwidthFree() will be necessary since we're passing by value and not reference...
Hmm, I think this last thing you mentioned won't be necessary at all, if you further inspect virNetDevBandwidthCopy(), we're passing by reference and the second action that takes place is assigning NULL to destination pointer. Sent v2 series. Erik
John
Whether this is what is "expected" is perhaps something Laine can answer...
John
+ + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto cleanup; } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!persistentNet->bandwidth) { persistentNet->bandwidth = bandwidth;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Up until now, we set memballoon period in monitor successfully, however we did not update domain definition structure, thus dumpxml was omitting period attribute in memballoon element Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140960 --- src/qemu/qemu_driver.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ede8880..d73288a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2460,9 +2460,15 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); qemuDomainObjExitMonitor(driver, vm); - if (r < 0) + if (r < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unable to set balloon driver collection period")); + goto endjob; + } + + vm->def->memballoon->period = period; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { -- 1.9.3
On 09/22/2014 06:41 AM, Erik Skultety wrote:
Up until now, we set memballoon period in monitor successfully, however we did not update domain definition structure, thus dumpxml was omitting period attribute in memballoon element
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140960 --- src/qemu/qemu_driver.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ede8880..d73288a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2460,9 +2460,15 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); qemuDomainObjExitMonitor(driver, vm); - if (r < 0) + if (r < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unable to set balloon driver collection period")); + goto endjob; + }
I'm trying to remember if there was a reason for not jumping to error. It probably has to do with "at some point in time" during development this setting would/could be done through calls via qemu_process.c and causing a failure through that path wasn't good. Now since this only accessible via a virsh command - I agree going to endjob is right... If you care to walk the history - start here: http://www.redhat.com/archives/libvir-list/2013-July/msg00770.html ACK (to what's here) John
+ + vm->def->memballoon->period = period; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
On 10/01/2014 01:55 AM, John Ferlan wrote:
On 09/22/2014 06:41 AM, Erik Skultety wrote:
Up until now, we set memballoon period in monitor successfully, however we did not update domain definition structure, thus dumpxml was omitting period attribute in memballoon element
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140960 --- src/qemu/qemu_driver.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ede8880..d73288a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2460,9 +2460,15 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); qemuDomainObjExitMonitor(driver, vm); - if (r < 0) + if (r < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unable to set balloon driver collection period")); + goto endjob; + }
I'm trying to remember if there was a reason for not jumping to error. It probably has to do with "at some point in time" during development this setting would/could be done through calls via qemu_process.c and causing a failure through that path wasn't good. Now since this only accessible via a virsh command - I agree going to endjob is right...
If you care to walk the history - start here:
http://www.redhat.com/archives/libvir-list/2013-July/msg00770.html
I had a little look at the history you pointed out. From what I've found, the sub-element 'period' was introduced along with qemu driver-specific API and libvirt domain API in 10-part v3 patch series (http://www.redhat.com/archives/libvir-list/2013-July/msg00774.html). I detached HEAD before these commits were pushed and I didn't find any trace about memballoon period setting. All the APIs and necessary structures/members were introduced by the patch series mentioned above, is that correct? :) (in that case I guess it's fine to add the jump to 'endjob' label)
ACK (to what's here)
John
+ + vm->def->memballoon->period = period; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Erik
On 10/02/2014 05:47 AM, Erik Skultety wrote:
On 10/01/2014 01:55 AM, John Ferlan wrote:
On 09/22/2014 06:41 AM, Erik Skultety wrote:
Up until now, we set memballoon period in monitor successfully, however we did not update domain definition structure, thus dumpxml was omitting period attribute in memballoon element
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140960 --- src/qemu/qemu_driver.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ede8880..d73288a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2460,9 +2460,15 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); qemuDomainObjExitMonitor(driver, vm); - if (r < 0) + if (r < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unable to set balloon driver collection period")); + goto endjob; + }
I'm trying to remember if there was a reason for not jumping to error. It probably has to do with "at some point in time" during development this setting would/could be done through calls via qemu_process.c and causing a failure through that path wasn't good. Now since this only accessible via a virsh command - I agree going to endjob is right...
If you care to walk the history - start here:
http://www.redhat.com/archives/libvir-list/2013-July/msg00770.html
I had a little look at the history you pointed out. From what I've found, the sub-element 'period' was introduced along with qemu driver-specific API and libvirt domain API in 10-part v3 patch series (http://www.redhat.com/archives/libvir-list/2013-July/msg00774.html). I detached HEAD before these commits were pushed and I didn't find any trace about memballoon period setting. All the APIs and necessary structures/members were introduced by the patch series mentioned above, is that correct? :) (in that case I guess it's fine to add the jump to 'endjob' label)
Right - those commits added the period setting. There were a couple of iterations of the work. I don't recall exactly since some time has passed since I did that work; however, I'm trying to post rationalize why I wouldn't have gone to endjob.. The call to virDomainSaveStatus is something I certainly missed. In any case, this hunk does look good. Let me know what you think about the latest response to 1/2 and I can push these for you. John
ACK (to what's here)
John
+ + vm->def->memballoon->period = period; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; }
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Erik
participants (2)
-
Erik Skultety -
John Ferlan