Skip to content
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

added config read from pve #22

Merged
merged 6 commits into from
Apr 20, 2020
Merged

added config read from pve #22

merged 6 commits into from
Apr 20, 2020

Conversation

frenkye
Copy link
Contributor

@frenkye frenkye commented Apr 16, 2020

Hi, in our case we want to track if virtual is running and also have onboot variable set, so it does boot automatically next time. In this case var is not set if its false, so rule on prometheus side should look for absent(...), because if not set it also missing from pve config files on FS.

So this PR should do it, I know am not skilled programmer. Even if this get you idea for rework its ok.

I wanted not to use diffrent loops for qemu and lxc, but can't get working diffrent query where could be lxc and qemu variables.

Thank you!

@frenkye
Copy link
Contributor Author

frenkye commented Apr 16, 2020

Ill fixed all issue in my code, but the python 3.4 is not tied to the feature or my proposal and need to be update in repository.

@znerol
Copy link
Member

znerol commented Apr 18, 2020

Very cool, this sounds like very useful feature. Will add some questions as review comments.

Copy link
Member

@znerol znerol left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR, I like the feature a lot.

Comment on lines 266 to 269
'memory': GaugeMetricFamily(
'pve_vm_config_memory',
'Proxmox vm config memory value',
labels=['id', 'node', 'type']),
Copy link
Member

Choose a reason for hiding this comment

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

There is pve_memory_size_bytes already, so this metric seems duplicate. Do you have a case where the new metric is better than the existing pve_memory_size_bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it seems like duplicate. I wanted to test it with ballooning memory management if the API cluster/resources does not return current value of ballooning, but don't have time for that ATM. Maybe in the futre will sent another PR for this if i'll test that.

Comment on lines 262 to 265
'onboot': GaugeMetricFamily(
'pve_vm_config_onboot',
'Proxmox vm config onboot value',
labels=['id', 'node', 'type']),
Copy link
Member

@znerol znerol Apr 18, 2020

Choose a reason for hiding this comment

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

The prometheus metric naming scheme is something like <prefix>_<name>_<unit>(_<aggregation>), e.g. node_cpu_seconds_total. So I guess my preferred name would be pve_onboot_status. The vm prefix is not necessary in my opinion because that one is given by the labels. Looking on other exporters for naming examples I find that node exporter has node_timex_sync_status. Ceph has metrics ceph_health_status, ceph_mgr_status and ceph_mon_quorum_status. So it looks like status might be a good unit for boolean flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about this and I make the change as you suggest.

Copy link
Member

Choose a reason for hiding this comment

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

I only learned about that when they changed a big bunch of metric names in node exporter. There is also a post by Brian Brazil and some docs on the prometheus site about naming.

@znerol znerol merged commit e267f66 into prometheus-pve:master Apr 20, 2020
@znerol
Copy link
Member

znerol commented Apr 20, 2020

Released as part of v1.2.0 on pypi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants