Refactoring and improvements discussion

[marcelmamula]

@richm @tomjelinek

Premise

I am currently implementing QDevice, QNetd, Diskless SBD for crmsh and there are some topics that are constantly being raised:

• main.yml complexity: There are some tasks that should be converted into "runners" inside of shell folders. This way we can do more complex differences or reordering of tasks, if required.
• Pacemaker version: We need to add __ha_cluster_pacemaker_version and handling, because of stonith > fencing changes as we are preloading some default properties.
• SBD might need split off as well, because there are some differences in SBD_OPTS and Timeout values.
• FQCN module names: Self explanatory
• 80 character line limit forces line breaks to conform with this limitation, which results in almost unreadable code and comments.

Proposed changes

I will complete current PR without going deeper into issues above, so subsequent changes can go separately.

1. Restructure

• main:

  • Leave only set vars, install packages, configure cluster, destroy cluster, qnetd, export
  • Everything else should stay inside of shell runners, which will help us customize better.

• Divergence of current structure: I want to try implementing using crm init, which is very rigid and opinionated. It does not work with current structure so it would have to come inside of shell_crmsh runner, but it is not possible with current main structure.

2. Pacemaker Version

This one is fairly simple as we need to drive logic based on versions now. Example:

# We need to query the Pacemaker version for version specific workflows.
# This is required for example for 'stonith' to 'fencing' switch in 3.0.1.
# Reason for noqa: Direct use of rpm removes dependency on python3-rpm.
- name: Get Pacemaker version  # noqa command-instead-of-module
  ansible.builtin.command:
    cmd: "rpm -q --queryformat '%{VERSION}' pacemaker"
  register: __ha_cluster_pacemaker_version_command
  changed_when: false

- name: Set fact with Pacemaker version
  ansible.builtin.set_fact:
    __ha_cluster_pacemaker_version:
      "{{ __ha_cluster_pacemaker_version_command.stdout }}"

Which can be consumed:

      vars:
        __property_name: >-
          {{ 'fencing-enabled'
            if __ha_cluster_pacemaker_version | d('1.0.0') is version('3.0.1', '>=')
            else 'stonith-enabled' }}

3. Code Quality

• FQCN is already in shell_crmsh task files, but it would be good idea to improve this in main and shell_pcs.
• Remove 80 character limit from tests and linting. Default 160 limit is good for readability and formatting.
• Module formatting: Switch like this will make this look better:

# Before
    - name: Configure SBD
      include_tasks: shell_{{ ha_cluster_pacemaker_shell }}/sbd.yml

# After
    - name: Configure SBD
      ansible.builtin.include_tasks:
        file: "shell_{{ ha_cluster_pacemaker_shell }}/sbd.yml"

4. Loop Control

• item should no longer be used as it breaks multi-layer loops so we need to start using loop_var more.
• This is also very useful for looping over list of dictionaries, because label simplifies output.

[spetrosi]

main.yml complexity: There are some tasks that should be converted into "runners" inside of shell folders. This way we can do more complex differences or reordering of tasks, if required.

Isn't it the case now?

Pacemaker version: We need to add __ha_cluster_pacemaker_version and handling, because of stonith > fencing changes as we are preloading some default properties.

I like it

FQCN module names: Self explanatory

Yeah, we historically do not use FQCNs, but feel free to update code to use them.

80 character line limit forces line breaks to conform with this limitation, which results in almost unreadable code and comments.

This limit comes from yaml, and we have to adhere to most yaml policies. I find this limit useful, if code becomes unreadable due to this limit, maybe our variables names are too long.

In exceptional places where this makes code unreadable it's fine to ignore these places.

[tomjelinek]

Hi @marcelmamula ,
thanks for starting this discussion before opening a PR. It's great to agree on the direction first, especially with large changes, so I really appreciate it. I see why the current structure is causing you issues. In general, I like your proposal and I agree with it. It looks like a good way to go forward.

1. Restructure

• main:
  • Leave only set vars, install packages, configure cluster, destroy cluster, qnetd, export
  • Everything else should stay inside of shell runners, which will help us customize better.

Agreed. The role was originally built around pcs and its architecture, so the current structure is not ideal for crmsh. Splitting to pcs vs. crmsh early will give us both more flexibility and prevent us from hitting limits and constraints imposed by one tool on the other.

What about the tasks for checking and preparing variables, setting the hacluster password, and configuring the shell, firewall, and SELinux? They sit between the package installation tasks but weren't mentioned in your list. Since these are either tool-agnostic or have variants for both pcs and crmsh, do you think they're fine where they are, or would you prefer to move them as well? If so, where do you see them fitting in?

2. Pacemaker Version

This is currently not needed by pcs, but I understand why it is important.

I'm not sure if calling rpm directly is the best way to go. Have you considered running pacemakerd --version and parsing the version from its output instead?

3. Code Quality

• FQCN is already in shell_crmsh task files, but it would be good idea to improve this in main and shell_pcs.

I'm fine with that.

• Remove 80 character limit from tests and linting. Default 160 limit is good for readability and formatting.

I’ve also hit that 80-character limit a few times myself. While I still prefer keeping lines short where possible, I agree that forcing it can sometimes hurt readability and formatting. I’ll probably stick to 80 characters in the pcs sections where it makes sense, but I’m totally fine with you using a more relaxed limit for the crmsh parts.

• Module formatting: Switch like this will make this look better:

# Before
    - name: Configure SBD
      include_tasks: shell_{{ ha_cluster_pacemaker_shell }}/sbd.yml

# After
    - name: Configure SBD
      ansible.builtin.include_tasks:
        file: "shell_{{ ha_cluster_pacemaker_shell }}/sbd.yml"

Yep, that makes sense to me.

4. Loop Control

• item should no longer be used as it breaks multi-layer loops so we need to start using loop_var more.
• This is also very useful for looping over list of dictionaries, because label simplifies output.

Sounds good!

How would you like to proceed?

@richm , I'd appreciate your input on this as well. Do you think these changes align with our general guidelines, or is there anything we should be careful about?

[marcelmamula]

Thank you for your responses @tomjelinek @spetrosi

1. Restructure

It is split little bit, but there is too much granularity inside of main, which means both CLIs have to use exact same steps, or hide unrelated inside of wrong task files.

This can be simplified with tasks like this (All would point to shell folders, so main would serve as simplified semaphore):

1. Load variables: include_vars.yml or prepare_vars.yml
2. Everything packages: install_packages.yml
   • RHEL: Load repositories and install packages
   • SLES: Check version, install patterns and packages
3. Configure before cluster preconfigure.yml
   • SLES: Attempt to implement crm init which does lot of heavy lifting, but it is very opinionated.
4. Configure cluster: cluster_present.yml
5. Destroy cluster: cluster_destroy.yml
6. Configure QNetd since this is not run on cluster hosts.

NOTE: This would not initially change anything for RHEL functionality, because existing tasks would just be moved. Then you have freedom to split it up if needed.

2. Pacemaker Version

I tested rpm and it was giving me consistent results because of --queryformat '%{VERSION}' but I think both should work fine with version() filter.

SLES 16.1 is coming with pacemaker 3.0.2 so this will become big topic and 3.0.1 is already causing FAILs because crm verify is forcing new fencing properties.

3. Code Quality

This can come as general refactor including Loop control. We can do it even in phases if you want to make smaller chunks: one for each shell.
I have already done most of work in shell_crmsh files so I can improve rest, then shell_pcs can follow up using it as template maybe?

[richm]

re: line length - this and other yaml style restrictions are imposed on us by ansible-lint/ansible-test, which are used for gating into AAP/Automation Hub - yes, we can use exceptions inline in the code or in .yamllint.yml and .ansible-lint to work around these. I think the 80 character line length restriction is now 100 or 120 - not sure - newer versions of ansible-lint seem to allow more than 80.

Guidelines:

• in general, if the line length or yaml style is acceptable to the ansible-lint/ansible-test github action checks, then it is ok from the system roles project standpoint - there may be exceptions to this, but I would have to see some ugly yaml formatting to object to it
• exceptions should be rare - use code comments to suppress checking - for example

# yamllint disable rule:line-length
  parameter: this is a very long line ........ that I don't want to break
# yamllint enable rule:line-length

This is already done in the ha_cluster role in a few places

[richm]

re: component version - the usual way to do this is to use package_facts: to get the info for installed packages, then extract the version from ansible_facts["packages"][package_name] like https://github.com/linux-system-roles/postgresql/blob/main/tasks/main.yml#L44 - however, you can also get it from command --version if that is easier

[marcelmamula]

re: component version - the usual way to do this is to use package_facts: to get the info for installed packages, then extract the version from ansible_facts["packages"][package_name] like https://github.com/linux-system-roles/postgresql/blob/main/tasks/main.yml#L44 - however, you can also get it from command --version if that is easier

Yes,, but it requires python3-rpm which is not baked into all images so I want to avoid it.

[marcelmamula]

re: line length

@richm
Could you double check and let me know what is allowed length as of now? I dont see any override in ansible or yaml lint configs, but my linting fails at Line too long (188 > 160 characters). So maybe 160 is current limit?

[tomjelinek]

1. Restructure

It is split little bit, but there is too much granularity inside of main, which means both CLIs have to use exact same steps, or hide unrelated inside of wrong task files.

This can be simplified with tasks like this (All would point to shell folders, so main would serve as simplified semaphore):

1. Load variables: include_vars.yml or prepare_vars.yml
2. Everything packages: install_packages.yml
   • RHEL: Load repositories and install packages
   • SLES: Check version, install patterns and packages
3. Configure before cluster preconfigure.yml
   • SLES: Attempt to implement crm init which does lot of heavy lifting, but it is very opinionated.
4. Configure cluster: cluster_present.yml
5. Destroy cluster: cluster_destroy.yml
6. Configure QNetd since this is not run on cluster hosts.

Sounds good.

There is a catch, though. Currently, the workflow is:

1. install essential packages
2. check and prepare role variables
3. install more packages based on results of step 2

This would need to be resolved somehow.

3. Code Quality

This can come as general refactor including Loop control. We can do it even in phases if you want to make smaller chunks: one for each shell. I have already done most of work in shell_crmsh files so I can improve rest, then shell_pcs can follow up using it as template maybe?

That would work for me. It would be nice if you could split the refactoring to commits by topic, for example FQCN + formatting in one, loop control in another one. If you already done it all together, there is no need to spend time on separating it, though.

[marcelmamula]

@tomjelinek I have solved this in other SLL roles:

1. Include vars which is baseline for role + OS
2. Prepare vars, which already contains detection and actions. This means package installation can fit nicely in between them.

Few examples:
https://github.com/sap-linuxlab/community.sap_operations/blob/main/roles/sap_control/tasks/main.yml
https://github.com/sap-linuxlab/community.sap_install/blob/main/roles/sap_ha_pacemaker_cluster/tasks/main.yml

[richm]

re: component version - the usual way to do this is to use package_facts: to get the info for installed packages, then extract the version from ansible_facts["packages"][package_name] like https://github.com/linux-system-roles/postgresql/blob/main/tasks/main.yml#L44 - however, you can also get it from command --version if that is easier

Yes,, but it requires python3-rpm which is not baked into all images so I want to avoid it.

ok - then I would suggest using a command: task which runs command --version and parses stdout, or your method of using rpm if that doesn't work

[richm]

re: line length

@richm Could you double check and let me know what is allowed length as of now? I dont see any override in ansible or yaml lint configs, but my linting fails at Line too long (188 > 160 characters). So maybe 160 is current limit?

Yes, 160 is the current limit. If that passes our ansible-lint/ansible-test github action checks, then ok by me, unless the readability could be improved by a shorter line length with wrapping/folding.

[marcelmamula]

Yes,, but it requires python3-rpm which is not baked into all images so I want to avoid it.

ok - then I would suggest using a command: task which runs command --version and parses stdout, or your method of using rpm if that doesn't work

I agree. With split mentioned in topic 1, we can have pcs using package_facts and crmsh using command to limit dependency.

[tomjelinek]

@tomjelinek I have solved this in other SLL roles:

1. Include vars which is baseline for role + OS
2. Prepare vars, which already contains detection and actions. This means package installation can fit nicely in between them.

Few examples: https://github.com/sap-linuxlab/community.sap_operations/blob/main/roles/sap_control/tasks/main.yml https://github.com/sap-linuxlab/community.sap_install/blob/main/roles/sap_ha_pacemaker_cluster/tasks/main.yml

Sure, it can be done. I'm just pointing out that

• specific tasks in "Check and prepare role variables" depend on pcs being installed
• installing packages depend on results of specific tasks in "Check and prepare role variables"

Obviously, the file can be split and tasks depending on pcs will be executed after package installation. I suppose they could be moved into shell_pcs/configure-shell.yml

I'm just saying that this needs to be handled.

[marcelmamula]

@tomjelinek That is one of main points of this discussion.
If we limit main to few task calls, then it means each shell have freedom to mold it as needed. If you are fine with it, then they can diverge quite a lot.

Example: crm init configures SSH keys and much more so I could make them conditional. This would change structure comparatively to pcs.