New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

machines: Split out usage stats polling #7131

Merged
merged 4 commits into from Jul 4, 2017

Conversation

Projects
None yet
3 participants
@martinpitt
Member

martinpitt commented Jun 28, 2017

Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

  • fix CPU usage
  • add new updateVm() to set polling flag to avoid race condition
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 28, 2017

Member

Note that a lot of the scary looking diff in hostvmslist.jsx is just due to indentation change - I had to make VmUsageTab a class component (previously a functional component).

Member

martinpitt commented Jun 28, 2017

Note that a lot of the scary looking diff in hostvmslist.jsx is just due to indentation change - I had to make VmUsageTab a class component (previously a functional component).

Show outdated Hide outdated pkg/machines/actions.es6

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jun 28, 2017

machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

Closes #7131
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 28, 2017

Member

This also somehow breaks the CPU usage graph. It jumps to a real usage, but quickly afterwards gets bumped back to 0. This doesn't happen with the event based updates, so I figure the global getVms() polling loop resets the CPU usage. It does work fine for memory usage.

Nevertheless this should not be broken even just intermediately, so I added a TODO item for this.

Member

martinpitt commented Jun 28, 2017

This also somehow breaks the CPU usage graph. It jumps to a real usage, but quickly afterwards gets bumped back to 0. This doesn't happen with the event based updates, so I figure the global getVms() polling loop resets the CPU usage. It does work fine for memory usage.

Nevertheless this should not be broken even just intermediately, so I added a TODO item for this.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jun 28, 2017

machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

Closes #7131
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 28, 2017

Member

Found and fixed the CPU usage bug, force-pushed.

Member

martinpitt commented Jun 28, 2017

Found and fixed the CPU usage bug, force-pushed.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jun 29, 2017

machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

Closes #7131
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 29, 2017

Member

@mareklibra: I reworked the updateVm() bits accordingly, see the second-to-last commit for details.

Member

martinpitt commented Jun 29, 2017

@mareklibra: I reworked the updateVm() bits accordingly, see the second-to-last commit for details.

@martinpitt martinpitt referenced this pull request Jun 29, 2017

Merged

machines: Replace state polling with event handling #7141

5 of 5 tasks complete

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jun 29, 2017

machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

Closes #7131
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 29, 2017

Member

Rebased to current master, resolved conflicts.

Member

martinpitt commented Jun 29, 2017

Rebased to current master, resolved conflicts.

@@ -192,7 +192,7 @@ export function fileDownload ({ data, fileName = 'myFile.dat', mimeType = 'appli
window.setTimeout(() => { // give phantomJS time ...
logDebug('removing temporary A.HREF for filedownload');
document.body.removeChild(a);
}, 500);
}, 5000);

This comment has been minimized.

@mareklibra

mareklibra Jun 30, 2017

Contributor

The temporary link might become visible within this timewindow.
If so, we can either hide it (not sure whether it breaks phantom test or not) or just set link's color to the backgound's color

@mareklibra

mareklibra Jun 30, 2017

Contributor

The temporary link might become visible within this timewindow.
If so, we can either hide it (not sure whether it breaks phantom test or not) or just set link's color to the backgound's color

This comment has been minimized.

@martinpitt

martinpitt Jun 30, 2017

Member

Thanks for pointing out! Indeed there is a little flickering spot on the bottom of the page. I tried to add a.style.visibility = 'hidden', but that only hides the link itself. But since it has no content, it is already not visible (you can test with adding a.appendChild(document.createTextNode("blablabla"))).

So that little speck of ugliness is not the <a> link itself, and it also isn't sensitive to 500 vs. 5000 ms, it just appears there for a split second either way. So I think this can be fixed independently.

@mareklibra, I retried the tests. Once they get green, could you land this? Thanks for the review!

@martinpitt

martinpitt Jun 30, 2017

Member

Thanks for pointing out! Indeed there is a little flickering spot on the bottom of the page. I tried to add a.style.visibility = 'hidden', but that only hides the link itself. But since it has no content, it is already not visible (you can test with adding a.appendChild(document.createTextNode("blablabla"))).

So that little speck of ugliness is not the <a> link itself, and it also isn't sensitive to 500 vs. 5000 ms, it just appears there for a split second either way. So I think this can be fixed independently.

@mareklibra, I retried the tests. Once they get green, could you land this? Thanks for the review!

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jun 30, 2017

Member

@mareklibra : green now \o/

Member

martinpitt commented Jun 30, 2017

@mareklibra : green now \o/

@mareklibra

This comment has been minimized.

Show comment
Hide comment
@mareklibra

mareklibra Jun 30, 2017

Contributor

@martinpitt , thanks! I've approved the PR but I don't have rights to merge.
@mvollmer, can you please land it?

Contributor

mareklibra commented Jun 30, 2017

@martinpitt , thanks! I've approved the PR but I don't have rights to merge.
@mvollmer, can you please land it?

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jul 3, 2017

machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

Closes #7131
@mvollmer

This comment has been minimized.

Show comment
Hide comment
@mvollmer

mvollmer Jul 3, 2017

Member

For the record: all tests green before rebasing and conflict resolving.

Member

mvollmer commented Jul 3, 2017

For the record: all tests green before rebasing and conflict resolving.

martinpitt added some commits Jun 28, 2017

test: Fix tight race condition in Machines external provider test
`check-machines TestMachines.testExternalConsole` would fail with a
timeout waiting for `#dynamically-generated-file` when running with
debugging/verbosity enabled, as the 500 ms window for catching the
temporary <a> was too short -- an Anti-Heisenbug.

Bump the timeout to 5s.
machines: Fix CPU utilization for multiple parallel polling loops
Fix timeSampleUsageData() to not reset the cpuUsage to 0 if the current
state update does not contain cpuTime. This happens once the usage
polling gets split from the global getAllVms() polling loop.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jul 3, 2017

machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

Closes #7131
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Jul 3, 2017

Member

Force-pushed rebase on current master.

Member

martinpitt commented Jul 3, 2017

Force-pushed rebase on current master.

Show outdated Hide outdated pkg/machines/actions.es6
Show outdated Hide outdated pkg/machines/actions.es6
machines: Generalize updateVmDisksStats() to updateVm()
updateVmDisksStats() only covers a very special case, but the "only
update existing VM" pattern applies to almost every other case where we
currently call updateOrAddVm(). So generalize it to updateVm() and
support a lot more properties, use it where it makes sense, and remove
the now unused properties from updateOrAddVm().

Simplify the interface of updateOrAddVm() and updateVm() in actions.es6
to just pass the properties object as a whole, instead of dissecting and
rebuilding essentially the same object.

Note: This breaks the provider API, as updating e. g. "state" with
updateOrAddVm() no longer works. But this was recently made internal
anyway, so this is no external API change.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jul 3, 2017

machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Extend the delayPolling() API to accept a VM name to do a VM specific
polling loop, which can check if polling is enabled for that VM, and if
that VM still exists in the first place.

Closes #7131
machines: Split out usage stats polling
Polling is legitimate for the RAM/CPU usage, but it only needs to be
done for those VMs which have the "Usage" tab open. So move the "virsh
domstats" and "virsh dommemstat" into a usage{Start,Stop)Polling() API
and call these in VmUsageTab (it has presence: 'onlyActive' already).

Closes #7131

@mvollmer mvollmer merged commit 565bd10 into cockpit-project:master Jul 4, 2017

16 checks passed

avocado/fedora-24 Tests passed
Details
container/kubernetes Tests passed
Details
selenium/chrome Tests passed
Details
selenium/firefox Tests passed
Details
semaphoreci The build passed on Semaphore.
Details
verify/centos-7 Tests passed
Details
verify/debian-stable Tests passed
Details
verify/debian-testing Tests passed
Details
verify/fedora-26 Tests passed
Details
verify/fedora-atomic Tests passed
Details
verify/fedora-i386 Tests passed
Details
verify/rhel-7 Tests passed
Details
verify/rhel-7-4 Tests passed
Details
verify/rhel-atomic Tests passed
Details
verify/ubuntu-1604 Tests passed
Details
verify/ubuntu-stable Tests passed
Details

@martinpitt martinpitt deleted the martinpitt:machines-usage-polling branch Jul 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment