Proxmox ve plugin additions

[fdriessler]

General information

This PR adds 3 checks and changes 1 to extend monitoring of Proxmox guests that do not use a checkmk guest agent,
to allow monitoring of cpu, disk and network throughput as well as uptime, even if the guest doesnt use a Guest Agent.

Proposed changes

This PR is for the most part a extension of the proxmox_ve plugin, to improve monitoring of piggyback only guests.

Following changes were introduced:
Additional Check:

• proxmox_ve_cpu_util: Service that yields CPU Utilization stats for monitored VMs (as % and # of cores under load (this is just the % * max cpu cores available)). This service also allows for a optional Metric for a averaged Utilization Value.
• proxmox_ve_disk_throughput: Service that yields Disk Read/Write Throughput.
• proxmox_ve_network_throughput: Service that yields Net In/Out Throughput.
  Changed:
• proxmox_ve_vm_info: Addition of a Uptime

Additionally, 2 check_parameter files were renamed, so that all check_parameters for proxmox_ve follow a common naming scheme.

[phauch]

@mo-ki do you have any info on when we can expect a merge? Or do we still have to adjust/improve something?

[NikCheckmk]

Hi, we'll do our best to review this pull request as soon as possible. Thank you for your patience.

[phauch]

Hey @NikCheckmk, thanks for the quick reply. Let us know if we can help

[NotANormalNerd]

@phauch @fdriessler Thank you for the PR.

Could you please refactor the Valuespecs to use the "new" RuleSpec v1 API:
https://docs.checkmk.com/latest/en/devel_check_plugins.html#rule_set

The API documentation for rule sets can be found in your Checkmk site on the same page as the Check API under Rulesets > Version 1.

Edit: Also for a plugin this size we would appreciate some basic tests. like: https://github.com/Checkmk/checkmk/blob/master/tests/unit/cmk/plugins/fritzbox/agent_based/test_fritz.py

[fdriessler]

@phauch @fdriessler Thank you for the PR.

Could you please refactor the Valuespecs to use the "new" RuleSpec v1 API: https://docs.checkmk.com/latest/en/devel_check_plugins.html#rule_set

The API documentation for rule sets can be found in your Checkmk site on the same page as the Check API under Rulesets > Version 1.

Edit: Also for a plugin this size we would appreciate some basic tests. like: https://github.com/Checkmk/checkmk/blob/master/tests/unit/cmk/plugins/fritzbox/agent_based/test_fritz.py

Refactors are done and should be part of this PR now.

• proxmox_ve_vm_info and proxmox_ve_node_info: changed slightly as it used to have a redundant Optional
• proxmox_ve_snapshot_age and proxmox_ve_vm_backup_status: due to change from old Age to Timespan the Datatype changed to Float -> introduced cast to int in the Check

Lastly, a question regarding the Data Rate Parameters that are used in proxmox_ve_vm_backup_status, proxmox_ve_disk_throughput and proxmox_ve_network_throughput:
For these I use Float with the unit_symbol 'MB/s'. Should these instead make use of DataSize, or alternatively, would it maybe make sense to add additional enums to be used with DataSize for Transfers (e.g. MB/s, MiB/s), as this would make options easier to understand.
For example: if DataSize was to be used in proxmox_ve_vm_backup_status, it could mean both minimum file size of a backup allowed before a WARN is returned, or minimum transfer speed.

[NotANormalNerd]

Hello @fdriessler,

thank you for the changes so far. Definitely looks better already. Would you be able to rebase the change and create one commit per plugin? This makes it easier for us to review the changes internally.

As to your question regarding the use of the DataSize: Yes, you should use DataSize and add a Label with "/s" as we did in the disk_io.py:

checkmk/cmk/plugins/collection/rulesets/disk_io.py

Line 38 in fe18f4d

form_spec_template=DataSize(

[fdriessler]

Hi @NotANormalNerd,

I've tried to use a Label to add a /s before, but even if I copy it 1:1 from your example, it ends up not showing up.

def _parameter_valuespec_proxmox_ve_disk_throughput():
    return Dictionary(
        elements={
            "read_levels": DictElement(
                required=True,
                parameter_form=Levels(
                    title=Title("Read levels"),
                    level_direction=LevelDirection.UPPER,
                    form_spec_template=DataSize(
                        label=Label("/s"),
                        displayed_magnitudes=[SIMagnitude.MEGA],
                    ),
                    prefill_fixed_levels=DefaultValue(value=(50_000_000, 100_000_000)),
                    predictive=PredictiveLevels(
                        reference_metric="disk_read_throughput",
                        prefill_abs_diff=DefaultValue(value=(0.0, 0.0)),
                    ),
                    migrate=migrate_to_upper_integer_levels,
                ),
            ),
            "write_levels": DictElement(
                required=True,
                parameter_form=Levels(
                    title=Title("Write levels"),
                    level_direction=LevelDirection.UPPER,
                    form_spec_template=DataSize(
                        label=Label("/s"),
                        displayed_magnitudes=[SIMagnitude.MEGA],
                    ),
                    prefill_fixed_levels=DefaultValue(value=(50_000_000, 100_000_000)),
                    predictive=PredictiveLevels(
                        reference_metric="disk_write_throughput",
                        prefill_abs_diff=DefaultValue(value=(0.0, 0.0)),
                    ),
                    migrate=migrate_to_upper_integer_levels,
                ),
            ),
        }
    )


rule_spec_proxmox_ve_disk_throughput = CheckParameters(
    name="proxmox_ve_disk_throughput",
    topic=Topic.CLOUD,
    parameter_form=_parameter_valuespec_proxmox_ve_disk_throughput,
    title=Title("Proxmox VE disk throughput"),
    condition=HostCondition(),
)

[NotANormalNerd]

I see. I have talked with the frontend team and this indeed seems like missing functionality with a existing API, I will open a (internal) Ticket for this case.

If there is only one magnitude you can adopt the title and either use "Read Levels (per second)". Usage of the DataSize is definitely encouraged in this case.

[github-actions]

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA or my organization already has a signed CLA.

---

1 out of 3 committers have signed the CLA.
✅ (fdriessler)[https://github.com/fdriessler]
❌ @logan-connolly
❌ @DavidGerva
_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[fdriessler]

thank you for the changes so far. Definitely looks better already. Would you be able to rebase the change and create one commit per plugin? This makes it easier for us to review the changes internally.

Hi @NotANormalNerd,
I've added all changes into a single commit, as I only change the proxmox_ve plugin and rebased the changes.

[NotANormalNerd]

hi @fdriessler,

we define proxmox_ve as a collection and the actual check plugins as plugins.

So it would be amazing if you could do one commit per plugin:

• proxmox_ve_backup_status (just the change)
• proxmox_ve_cpu_util (with plugin, manpage, graphing and ruleset)
• proxmox_ve_disk_throughput (with plugin, manpage, graphing and ruleset)
• proxmox_ve_network_throughput (with plugin, manpage, graphing and ruleset)
• proxmox_ve_snapshot_age (with plugin and ruleset)
• ...

You can see my changes for the acme collection and the respective plugins.
https://github.com/Checkmk/checkmk/commits/master/?author=NotANormalNerd

I am sorry if this is more work for you and we are already working on updating the contribution guidelines to be more clear about that. It will help us tremendously to assign developers internally to review the changes.

[fdriessler]

hi @NotANormalNerd, commits have been split now. If anything else is left to do I'd be happy to help out.

[NotANormalNerd]

Good Morning @fdriessler, I will take this over now and give it into internal review.

[NotANormalNerd]

@fdriessler Most changes are merged already from your PR. There are two or three outstanding that are awaiting a review. I had to refactor some of the commits and make sure they are up to our code quality standard and internal CI.

Please take some time to review the commits to see which changes I made. Author attribution is still completely with you.

As soon as everything is merged I will close this PR.