Migrate Rule module to new API

[lgetwan]

I migrated the rule module to the new API and made it a bit nicer. Will update this description soon. Just wanted to save my work before the weekend. ;-)

Pull request type

Please check the type of change your PR introduces:

• [ ] Bugfix
• [x] Feature
• [x] Code style update (formatting, renaming)
• [ ] Refactoring (no functional changes, no API changes)
• [ ] Build related changes
• [x] Documentation content changes
• [ ] Other (please describe):

What is the current behavior?

Rule module is a bit ugly and only ~23% idempotent.

What is the new behavior?

Rule module

• is nicer,
• uses the new API,
• is ~87% idempotent.
• The rules lookup module accepts filtering for folders, now.

Other information

100% idempotency will be reached, once the value_raw is handled differently by Checkmk and its REST API.

[lgetwan]

@msekania: You already spent a lot of time diving into the rule module and its flaws. Can you have a look at my completely rewritten rule module and test it, if you have some time?

[msekania]

@lgetwan, yes I'll do

@msekania: You already spent a lot of time diving into the rule module and its flaws. Can you have a look at my completely rewritten rule module and test it, if you have some time?

[msekania]

@lgetwan,

I've gone through the code and have more than a few suggestions for improvement and cleanup. I'll do the code review comments right after.

As for the rest, overwriting, it's good overall, but I don't like that the functionality has been reduced compared to the existing version. Namely, the check whether a rule already exists no longer takes place if rule_id is not specified. I understand that the existing version was not perfect in this regard, but it was still useful and all my use cases relied on this functionality. Currently there is also no way to compensate the "removed" functionality with the lookup rule plugin.

I am currently working to bring that functionality back, if you do not mind.

[lgetwan]

Hi @msekania

Thanks a lot for your comprehensive review! I need some time to reply to the single findings though, as my calendar is currently packed with customer calls.

As for the rest, overwriting, it's good overall, but I don't like that the functionality has been reduced compared to the existing version. Namely, the check whether a rule already exists no longer takes place if rule_id is not specified. I understand that the existing version was not perfect in this regard, but it was still useful and all my use cases relied on this functionality. Currently there is also no way to compensate the "removed" functionality with the lookup rule plugin.

I am currently working to bring that functionality back, if you do not mind.

I know that my changes are breaking existing stuff. I decided to do that, because the current module is kind of unreliable and unpredictable in its behavior, and so I wanted to move the module and its use cases to the direction of how the API is intended to be used. I will discuss with my colleagues if we might bring back the previous behavior at least as a side-feature. Or probably (as you also wrote) it would help extending the rules lookup module, so it includes conditions and raw_value to search for. Would that be an option for you? If not, How should the rules lookup module behave, so you can keep your current use cases?

Best regards Lars

[msekania]

Hi @lgetwan,

General problem which I have with a new version (without modification) is that that one creates all the time the same rule, because there is no any mechanisms that allows to check and stop recreation of the same rule again and again. Currently one requires to provide rule_id in order to compare, edit, or move the rule. This will stay as it is.

I would appreciate if one would have a mechanism to try to deduce the rule_id if user is not providing it from the rest of parameters. Positioning in folder issues one can solve by introducing extra position value "any" in addition to "top", "bottom", "before", and "after", which will ignore precise position check within folder in is_equal check,

I have finished implementation of this features, and it's more or less like what I would like to have. I also found some extra bugs in current version.

In which form I should provide the changes?

Best regards, Michael

[robin-checkmk]

In which form I should provide the changes?

@msekania may I suggest a branch based on this branch? That should allow you to open a PR onto this one.

PR on top of this PR: #586

[lgetwan]

Again, it's complicated when modifying an existing rule (by providing an ID) to decide if conditions should be set or not. Querying the values of an existing rule and re-applying them leads to problems.

[msekania]

Again, it's complicated when modifying an existing rule (by providing an ID) to decide if conditions should be set or not. Querying the values of an existing rule and re-applying them leads to problems.

@lgetwan can you provide some examples

[robin-checkmk]

My initial tests show, that both for Checkmk 2.2.0p25 and 2.3.0b6 the idempotent is still completely broken without specifying the rule_id.

Representative example rule definition:

  - ruleset: "checkgroup_parameters:cpu_utilization_os"
    rule:
      location:
        folder: "/"
        position: "bottom"
      properties: {
        "comment": "Set by Ansible",
        "disabled": false
      }
      value_raw: "{'core_util_time': (100.0, 300, 900), 'core_util_time_total': (100.0, 300, 900)}"
      conditions:
        host_labels: []
        service_labels: []
        host_tags: []

Accompanying task:

- name: "Create Rules."
  checkmk.general.rule:
    server_url: "{{ server_url }}"
    site: "{{ site }}"
    automation_user: "{{ automation_user }}"
    automation_secret: "{{ automation_secret }}"
    ruleset: "{{ item.ruleset }}"
    rule: "{{ item.rule }}"
    state: "present"
  loop: "{{ rules_global }}"
  delegate_to: localhost
  run_once: true  # noqa run-once[task]

I am still investigating and will update this post, if I find something else.

[msekania]

Thanks @robin-checkmk, for the example,

I'll have some tests.

Concerning rule_id, problem is that it's not existing when rule is created, therefor it's useless in bare create task. If rule_id will remain the only solution for the moment, then working example would be:

• search if the rule with desired properties already exists, and if yes get rule_id,
• incorporate rule_id if found in the task which creates rule, otherwise omit this filed entirely.

[msekania]

@robin-checkmk,

In current rue module setting any value in description let's say

  - ruleset: "checkgroup_parameters:cpu_utilization_os"
    rule:
      location:
        folder: "/"
        position: "bottom"
      properties: {
        "description": "test"
        "comment": "Set by Ansible",
        "disabled": false
      }
      value_raw: "{'core_util_time': (100.0, 300, 900), 'core_util_time_total': (100.0, 300, 900)}"
      conditions:
        host_labels: []
        service_labels: []
        host_tags: []

solves the idempotency peoblem. I'll check new version next, but this could be essential.

OK, works for new version also on Check mk 2.2.0p16.

[msekania]

@robin-checkmk, @lgetwan,

I have a fix for the problem, namely issue is description field in properties in DESIRED_DEFAULTS line 353 https://github.com/Checkmk/ansible-collection-checkmk.general/blob/39f143e456ccd2e40431830deb72c6b4843ca915/plugins/modules/rule.py#L353, may be also https://github.com/Checkmk/ansible-collection-checkmk.general/blob/39f143e456ccd2e40431830deb72c6b4843ca915/plugins/modules/rule.py#L365, description should not be the part of properties in DESIRED_DEFAULTS (at least for pre_230)

[robin-checkmk]

In current rue module setting any value in description let's say [...] solves the idempotency peoblem.

I can confirm this for 2.2. Setting "description": "test", makes virtually all my example rules idempotent again. In 2.3 this seems not to help though.

I have a fix for the problem, namely issue is description field in properties in DESIRED_DEFAULTS line 353 [...], may be also [365], description should not be the part of properties in DESIRED_DEFAULTS (at least for pre_230)

I pushed a change commenting out these lines. But I want @lgetwan to get a chance to verify, if this is a sensible fix before merging it.

[lgetwan]

I will check that tomorrow.

[lgetwan]

The last commit solves the problem for 2.2.0, but I still don't get why it doesn't work for 2.3.0...

[lgetwan]

@robin-checkmk & @msekania, That was a hard piece of work considering that the diff is quite small, but now I'm pretty sure, that the idempotency in 2.3.0 is as "good" as in 2.2.0. Can you run your tests, again, as well, please?

[msekania]

@lgetwan,

there are some more improvements see #602 , I could not test 2.3.0 yet, but similar fixes might be due for 2.3.0 as well.

I also have a question concerning your last changes. What if I intentionally provide empty list just to clean up already set one? I think this changes should also go similar to what I have done for comment and description in #602

[msekania]

@lgetwan,

I have an Idea how to incorporate logic of your changes in what I wrote above. I'll need hour or two at most. Of course you can drop them if you do not like them, no problem.

[msekania]

@lgetwan

@lgetwan,

I have an Idea how to incorporate logic of your changes in what I wrote above. I'll need hour or two at most. Of course you can drop them if you do not like them, no problem.

DONE

[lgetwan]

Ok, now idempotency looks quite good with 2.2.0 and 2.3.0, at least for my own tests and the integration tests. Thanks again for #602, @msekania! Can you run your tests against the newest version of this PR?

[msekania]

@lgetwan

Ok, now idempotency looks quite good with 2.2.0 and 2.3.0, at least for my own tests and the integration tests. Thanks again for #602, @msekania! Can you run your tests against the newest version of this PR?

I really love solution with _normalize_rule!

I have tested with 2.2.0 and it works for all my rules too. Unfortunately, I cannot test 2.3.0 for the moment.

[robin-checkmk]

I can confirm ~85% idempotency as well for 2.2 and 2.3. The only hiccup is, that "comment": "{{ ansible_date_time.iso8601 }} Set by Ansible" will not work, as you always get a different comment. But I think that is fine. In that case one needs to use the rule_id.

The only challenges that remain from my point of view are:

1. Verify that the module documentation is up-to-date with the recent changes.
2. Decide whether this is a breaking change or not and how to release it.