Trying to handle the changes that came with werk #16466. #734
Conversation
lgetwan
changed the title
|
The "Wait for completion" mechanism changed with the 2.4.0. |
…s of the REST API better.
|
Ok, the integration tests are now also successful. And they are quite extensive for the discovery module. |
|
I will review the code today, tomorrow at the latest. |
There was a problem hiding this comment.
First of all, I like very much the idea to split module in version dependent parts!
The logic of the code is clearer and easier to follow in this form!
I commented on discovery_240.py file mainly, for the moment, in order to clarify the logic change introduced by the "wait" concept.
Otherwise, the comments would be very similar to other version-parts of the code.
| if self.single_mode and result.http_code != 200: | ||
| result = self._wait_for_completion("current") | ||
| elif self.bulk_mode: | ||
| job_id = json.loads(result.content).get("id") | ||
| result = self._wait_for_completion("current", job_id=job_id) |
There was a problem hiding this comment.
again, it might timeout here, but it does not mean that the process will not be finished successfully after some time without intervention from the user side.
There was a problem hiding this comment.
That's true, and that was by intention (see my use-case "1." in the other comment).
| if now > deadline: | ||
| return generate_result( | ||
| msg="Timeout reached while waiting for %s discovery" % what | ||
| ) |
There was a problem hiding this comment.
I understand the idea of timeout and it might be appealing but I see couple of issues which should be clarified:
- when it timeouts here it does not mean that process is still not running in background,
- it does not return failed state and next call of rediscovery might fail.
If one still decides to go with timeout concept, then one should also make clear in documentation that in essence it might take up to twice the timeout before plugin returns timeout, but it also does not mean that the process will not be finished successfully after some time without intervention from the user side.
There was a problem hiding this comment.
The use-cases I had in mind, were:
- I only want to trigger the discovery, and I don't mind if it finishes successfully. I might check later in the playbook
- I want to wait for the discovery to finish, and the play should only continue if the discovery was successful.
- A mix: 99% of my discoveries finish within 30s. I don't want to wait longer in my playbook. The 1% that fails (or succeeds in background), I will check manually, later or in another playbook. (~ Pareto principle. ;-))
After implementing the default "infinite", these should be possible, with "2." being the default, to make the change non-breaking.
Does that make sense to you? Are there other use-cases that you can think of?
There was a problem hiding this comment.
Does that make sense to you? Are there other use-cases that you can think of?
Yes! I think that's it!
I will check the changed implementation more closely today, with a quick glance something is wrong.
There was a problem hiding this comment.
v2.2 and v2.1 have not yet received the updates (wait_time: -1 treatment), right?
There was a problem hiding this comment.
Yes, 2.2 and 2.1 were missing, but I had to leave into Feierabend. ;-)
Fixed that this morning.
|
It's fine from my point of view! |
|
Thanks a lot, @msekania! @robin-checkmk, |
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
The discovery module is not handling the changes that came with Werk #16466
What is the new behavior?
The discovery module works with all supported Checkmk versions up to to 2.4.0