Skip to content

Fix wrong memory measurements of containers and vms#8290

Merged
ssoroka merged 3 commits intoinfluxdata:masterfrom
tlusser-inv:master
Oct 21, 2020
Merged

Fix wrong memory measurements of containers and vms#8290
ssoroka merged 3 commits intoinfluxdata:masterfrom
tlusser-inv:master

Conversation

@tlusser-inv
Copy link
Copy Markdown
Contributor

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@tlusser-inv
Copy link
Copy Markdown
Contributor Author

Linked PR to issue #8291

Copy link
Copy Markdown
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. I'd like to avoid the NodeName blank check before merge. see comment.

}
tags := getTags(px, vmStat.Name, vmConfig, rt)
fields, err := getFields(vmStat)
currentVMStatus, err := getCurrentVMStatus(px, rt, vmStat.ID)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that this adds an http request inside a loop. It's going to have some negative performance implications, especially on systems with a large number of VMs. If there's any way to get this data in aggregate with one call, that would be preferable.

Copy link
Copy Markdown
Contributor Author

@tlusser-inv tlusser-inv Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are aware of this, but AFAIK there is no other possibility in the REST-API to query the status of all virtual machines/containers in one step (please see: https://pve.proxmox.com/pve-docs/api-viewer/)

@sjwang90 sjwang90 added the fix pr to fix corresponding bug label Oct 20, 2020
@ssoroka ssoroka merged commit 9c2979d into influxdata:master Oct 21, 2020
@ssoroka
Copy link
Copy Markdown
Contributor

ssoroka commented Oct 21, 2020

Awesome, Thanks!

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

Labels

fix pr to fix corresponding bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants