Upstream PR for all bugfixes since first commit of Nutanix Prism checks #638

Yogibaer75 · 2023-10-15T09:04:29Z

This PR request includes two bug fixes and 1 missing check for Nutanix.

Missing checks - host network interfaces
Bug fixes

vm memory -> unknown if no memory values existing fixed
vm_tools -> state can be ignored

github-actions · 2023-10-15T09:04:48Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Yogibaer75 · 2023-10-15T09:05:14Z

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

martinhv · 2023-10-24T08:13:08Z

From a PM perspective, I approve this change. It creates value for Checkmk and fixes bugs.
The technical evaluation I leave to dev.

BenediktSeidl

Thank you very much for your contribution. In general, it looks good, it even has tests ❤️ very nice!

To merge I would add three (or two) werks, but I need some more context to the following items:

vm memory -> unknown if no memory values existing fixed
vm_tools -> state can be ignored
It would be good to know what the previous behavior was, and how it was fixed, so I can come up with a Werk that other customers can understand.

Can you split your two commits into three commits? Two commits for the two fixes and an additional commit for the new feature.

You can add the style change in cmk/base/plugins/agent_based/prism_cluster_io.py to any of these commits, but it would be cool if you could mention it in the commit message.

This probably seems overly picky, but git history is often the only way to understand the intent of changes from years ago, so we try to maintain it as best we can.

I've also left four code annotations with questions and comments, I'm open to discussiong these issues.

BenediktSeidl · 2023-10-24T11:46:14Z

cmk/base/plugins/agent_based/prism_host_networks.py

@@ -0,0 +1,118 @@
+#!/usr/bin/env python3
+# -*- encoding: utf-8; py-indent-offset: 4 -*-


please use the checkmk license header

Why is this now a problem in all the other checks made before there is the same header?

before asking you to change this, i asked internally and the official statement is, that we should only use the checkmk license header.
your files were the only exception that used non-official license headers. our license header check was very lax in this regard. today i merged a change which applies stricter rules from now on.

cmk/base/plugins/agent_based/utils/prism.py

BenediktSeidl · 2023-10-24T11:53:30Z

cmk/base/plugins/agent_based/prism_host_networks.py

+)
+
+
+def _create_interface(section: Section) -> interfaces.Section[interfaces.InterfaceWithRates]:


would it be possible to move the content of this function into the parse function?
making the return type of the parse function interfaces.Section[interfaces.InterfaceWithRates]
this way you could remove the calls to _create_interface from the discovery and check function.

I must added this workaround to make tests working. I copied this part from one of your own checks :)

This is 100% the same as the code for esx_vsphere_counters_if

BenediktSeidl · 2023-10-24T12:03:38Z

cmk/base/plugins/agent_based/prism_host_networks.py

+def check_prism_host_networks(
+    item: str, params: Mapping[str, Any], section: Section
+) -> CheckResult:
+    data = section.get(item)


are you sure that those statements are really needed?
i would think that this case is already handled by interfaces.check_multiple_interfaces.

All the other checked/tested code versions where not working together with the tests.
That was the only way the test subsystem was happy. My original code (2.2 mkp file) was not so "bloated".

Yogibaer75 · 2023-10-25T18:15:54Z

vm memory -> unknown if no memory values existing fixed
It is possible the one defined VM don't report a memory value and then the check was crashing if no value exists.

vm_tools -> state can be ignored
It would be good to know what the previous behavior was, and how it was fixed, so I can come up with a Werk that other customers can understand.
Before you needed to specify a wanted state. If nothing was specified the default parameter state was used. Now you can define to ignore the state what can be helpful for machines where no tools can be installed like appliances and so on.

Can you split your two commits into three commits? Two commits for the two fixes and an additional commit for the new feature.
After the submit no, I don't know how to split the commits.

You can add the style change in cmk/base/plugins/agent_based/prism_cluster_io.py to any of these commits, but it would be cool if you could mention it in the commit message.

Ok that's a point. As this was a commit of all the collected changes from my repo it is possible that i forgot to mention.

This probably seems overly picky, but git history is often the only way to understand the intent of changes from years ago, so we try to maintain it as best we can.

I've also left four code annotations with questions and comments, I'm open to discussiong these issues.
I wrote some comments. All these "problems" are only there as your tests wanted these changes. Before my simple code was rejected from the mypy/pytest tests 🤣

I think some constructs are over complicated inside CMK if you want to make the test environment happy.
Later bugfixes a can submit as simple small PRs, but then we also need a better way for the processing of these PRs.

BenediktSeidl · 2023-10-26T13:23:22Z

Thanks for your response and the additional info about your change. We are happy that you share your domain knowledge with checkmk and want to include this in our product.
I would suggest, that I take over from here, adapt your code to meet our coding standards, split it into three commits and add the necessary werks. The business logic will not be altered. You are still the main author of the git commits, to show that there were additional changes, I would add myself as a co-author.
Is this okay for you?

Yogibaer75 · 2023-10-26T15:22:37Z

Is this okay for you?

Yes why not :)

Add a new check to monitor host networks via prism refs #638 Change-Id: I600a434b11f4c7aee3bdb29a30d3560c65e5542c Co-Authored-By: Benedikt Seidl <[email protected]>

BenediktSeidl · 2023-11-09T07:06:19Z

@Yogibaer75 yesterday i merged a first version of your change into the master branch. can you please check in a daily master build if it is working as intended? i've implemented some bigger changes for parsing the json file, as it was suggested in the internal code review.

the internal code review also opened the question if the api already returns rates, or if those are global counters? as far as i understand those are rates based on a 30 seconds interval. is the comment i added correct?

checkmk/cmk/base/plugins/agent_based/prism_host_networks.py

Lines 78 to 80 in b9f640b

    
           # assuming a 30 seconds window in which those counters are accumulated. 
        
           # could be verified via /get/hosts/{uuid}/host_nics/{pnic_id}/stats 
        
           # https://www.nutanix.dev/api_references/prism-v2-0/#/3a7cca6f493d6-list-host-host-nic-stats

thanks again for your contribution!

Yogibaer75 · 2023-11-09T10:12:06Z

the internal code review also opened the question if the api already returns rates, or if those are global counters? as far as i understand those are rates based on a 30 seconds interval. is the comment i added correct?

Yes these are already rates and no counters. And the rate is based on the last 30 seconds. I found no other way than this to get the interface data.

For the master build i must check the next days if it does what it should :)

BenediktSeidl self-requested a review October 24, 2023 11:45

BenediktSeidl self-assigned this Oct 24, 2023

BenediktSeidl requested changes Oct 24, 2023

View reviewed changes

Yogibaer75 requested a review from BenediktSeidl October 25, 2023 18:16

mo-ki added bug Something isn't working enhancement New feature or request labels Oct 31, 2023

CheckmkCI pushed a commit that referenced this pull request Nov 8, 2023

15306 prism_host_networks: new check

8f2bd9a

Add a new check to monitor host networks via prism refs #638 Change-Id: I600a434b11f4c7aee3bdb29a30d3560c65e5542c Co-Authored-By: Benedikt Seidl <[email protected]>

Yogibaer75 closed this Nov 24, 2023

Yogibaer75 force-pushed the master branch from d10379a to bbaa536 Compare November 24, 2023 07:48

github-actions bot locked and limited conversation to collaborators Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream PR for all bugfixes since first commit of Nutanix Prism checks #638

Upstream PR for all bugfixes since first commit of Nutanix Prism checks #638

Yogibaer75 commented Oct 15, 2023

github-actions bot commented Oct 15, 2023 •

edited

Loading

Yogibaer75 commented Oct 15, 2023

martinhv commented Oct 24, 2023

BenediktSeidl left a comment

BenediktSeidl Oct 24, 2023

Yogibaer75 Oct 25, 2023

BenediktSeidl Oct 26, 2023

BenediktSeidl Oct 24, 2023

Yogibaer75 Oct 25, 2023

Yogibaer75 Oct 25, 2023

BenediktSeidl Oct 24, 2023

Yogibaer75 Oct 25, 2023

Yogibaer75 commented Oct 25, 2023

BenediktSeidl commented Oct 26, 2023

Yogibaer75 commented Oct 26, 2023

BenediktSeidl commented Nov 9, 2023

Yogibaer75 commented Nov 9, 2023

		@@ -0,0 +1,118 @@
		#!/usr/bin/env python3
		# -- encoding: utf-8; py-indent-offset: 4 --

		)


		def _create_interface(section: Section) -> interfaces.Section[interfaces.InterfaceWithRates]:

Upstream PR for all bugfixes since first commit of Nutanix Prism checks #638

Upstream PR for all bugfixes since first commit of Nutanix Prism checks #638

Conversation

Yogibaer75 commented Oct 15, 2023

github-actions bot commented Oct 15, 2023 • edited Loading

Yogibaer75 commented Oct 15, 2023

martinhv commented Oct 24, 2023

BenediktSeidl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yogibaer75 commented Oct 25, 2023

BenediktSeidl commented Oct 26, 2023

Yogibaer75 commented Oct 26, 2023

BenediktSeidl commented Nov 9, 2023

Yogibaer75 commented Nov 9, 2023

github-actions bot commented Oct 15, 2023 •

edited

Loading