feat: crmsh workflow and SUSE support #186
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
=========================================
+ Coverage 0 50.54% +50.54%
=========================================
Files 0 1 +1
Lines 0 91 +91
Branches 0 12 +12
=========================================
+ Hits 0 46 +46
- Misses 0 45 +45
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
marcelmamula
changed the title
There was a problem hiding this comment.
I'm getting a lot of errors from linters in CI. Please, fix those:
yamllint:
./vars/Suse.yml
19:21 warning too few spaces before comment (comments)
23:81 error line too long (82 > 80 characters) (line-length)
./tasks/main.yml
95:81 error line too long (87 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-constraint-location.yml
7:81 error line too long (99 > 80 characters) (line-length)
8:81 error line too long (104 > 80 characters) (line-length)
13:81 error line too long (93 > 80 characters) (line-length)
18:81 error line too long (85 > 80 characters) (line-length)
21:81 error line too long (95 > 80 characters) (line-length)
29:81 error line too long (94 > 80 characters) (line-length)
./tasks/shell_crmsh/create-and-push-cib.yml
237:81 error line too long (81 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-constraint-ticket.yml
12:81 error line too long (93 > 80 characters) (line-length)
17:81 error line too long (85 > 80 characters) (line-length)
20:81 error line too long (95 > 80 characters) (line-length)
28:81 error line too long (96 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-constraint-set.yml
3:88 warning too few spaces before comment (comments)
3:81 error line too long (108 > 80 characters) (line-length)
12:81 error line too long (93 > 80 characters) (line-length)
17:81 error line too long (85 > 80 characters) (line-length)
20:81 error line too long (95 > 80 characters) (line-length)
25:89 warning too few spaces before comment (comments)
25:81 error line too long (109 > 80 characters) (line-length)
33:81 error line too long (81 > 80 characters) (line-length)
39:81 error line too long (82 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-resource-primitive.yml
11:81 error line too long (83 > 80 characters) (line-length)
20:28 warning too few spaces before comment (comments)
./tasks/shell_crmsh/crm-cib-stonith-level.yml
12:81 error line too long (88 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cluster-properties.yml
6:81 error line too long (110 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-constraint-order.yml
12:81 error line too long (93 > 80 characters) (line-length)
17:81 error line too long (85 > 80 characters) (line-length)
20:81 error line too long (95 > 80 characters) (line-length)
28:81 error line too long (99 > 80 characters) (line-length)
29:81 error line too long (95 > 80 characters) (line-length)
30:81 error line too long (93 > 80 characters) (line-length)
./tasks/shell_crmsh/sbd.yml
15:81 error line too long (82 > 80 characters) (line-length)
16:81 error line too long (92 > 80 characters) (line-length)
17:81 error line too long (81 > 80 characters) (line-length)
18:81 error line too long (93 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-resource-group.yml
6:81 error line too long (84 > 80 characters) (line-length)
11:81 error line too long (83 > 80 characters) (line-length)
14:81 error line too long (86 > 80 characters) (line-length)
./tasks/shell_crmsh/cluster-start-and-reload.yml
64:81 error line too long (85 > 80 characters) (line-length)
98:81 error line too long (85 > 80 characters) (line-length)
103:81 error line too long (86 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-resource-clone.yml
3:81 error line too long (113 > 80 characters) (line-length)
6:81 error line too long (106 > 80 characters) (line-length)
11:81 error line too long (91 > 80 characters) (line-length)
16:81 error line too long (83 > 80 characters) (line-length)
19:81 error line too long (93 > 80 characters) (line-length)
27:81 error line too long (89 > 80 characters) (line-length)
./tasks/shell_crmsh/crm-cib-constraint-colocation.yml
3:81 error line too long (81 > 80 characters) (line-length)
12:81 error line too long (93 > 80 characters) (line-length)
17:81 error line too long (85 > 80 characters) (line-length)
20:81 error line too long (95 > 80 characters) (line-length)
28:81 error line too long (96 > 80 characters) (line-length)
34:81 error line too long (97 > 80 characters) (line-length)
ansible-lint:
yaml[line-length]: Line too long (87 > 80 characters)
tasks/main.yml:95
yaml[line-length]: Line too long (85 > 80 characters)
tasks/shell_crmsh/cluster-start-and-reload.yml:64
yaml[line-length]: Line too long (85 > 80 characters)
tasks/shell_crmsh/cluster-start-and-reload.yml:98
yaml[line-length]: Line too long (86 > 80 characters)
tasks/shell_crmsh/cluster-start-and-reload.yml:103
yaml[line-length]: Line too long (81 > 80 characters)
tasks/shell_crmsh/create-and-push-cib.yml:237
yaml[line-length]: Line too long (81 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-colocation.yml:3
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-colocation.yml:12
yaml[line-length]: Line too long (85 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-colocation.yml:17
yaml[line-length]: Line too long (95 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-colocation.yml:20
yaml[line-length]: Line too long (96 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-colocation.yml:28
yaml[line-length]: Line too long (97 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-colocation.yml:34
yaml[line-length]: Line too long (99 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-location.yml:7
yaml[line-length]: Line too long (104 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-location.yml:8
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-location.yml:13
yaml[line-length]: Line too long (85 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-location.yml:18
yaml[line-length]: Line too long (95 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-location.yml:21
yaml[line-length]: Line too long (94 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-location.yml:29
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-order.yml:12
yaml[line-length]: Line too long (85 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-order.yml:17
yaml[line-length]: Line too long (95 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-order.yml:20
yaml[line-length]: Line too long (99 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-order.yml:28
yaml[line-length]: Line too long (95 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-order.yml:29
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-order.yml:30
yaml[line-length]: Line too long (108 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-set.yml:3
yaml[comments]: Too few spaces before comment
tasks/shell_crmsh/crm-cib-constraint-set.yml:3
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-set.yml:12
yaml[line-length]: Line too long (85 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-set.yml:17
yaml[line-length]: Line too long (95 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-set.yml:20
yaml[line-length]: Line too long (109 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-set.yml:25
yaml[comments]: Too few spaces before comment
tasks/shell_crmsh/crm-cib-constraint-set.yml:25
yaml[line-length]: Line too long (81 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-set.yml:33
yaml[line-length]: Line too long (82 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-set.yml:39
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-ticket.yml:12
yaml[line-length]: Line too long (85 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-ticket.yml:17
yaml[line-length]: Line too long (95 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-ticket.yml:20
yaml[line-length]: Line too long (96 > 80 characters)
tasks/shell_crmsh/crm-cib-constraint-ticket.yml:28
yaml[line-length]: Line too long (113 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-clone.yml:3
yaml[line-length]: Line too long (106 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-clone.yml:6
yaml[line-length]: Line too long (91 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-clone.yml:11
yaml[line-length]: Line too long (83 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-clone.yml:16
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-clone.yml:19
yaml[line-length]: Line too long (89 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-clone.yml:27
yaml[line-length]: Line too long (84 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-group.yml:6
yaml[line-length]: Line too long (83 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-group.yml:11
yaml[line-length]: Line too long (86 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-group.yml:14
yaml[line-length]: Line too long (83 > 80 characters)
tasks/shell_crmsh/crm-cib-resource-primitive.yml:11
yaml[comments]: Too few spaces before comment
tasks/shell_crmsh/crm-cib-resource-primitive.yml:20
yaml[line-length]: Line too long (88 > 80 characters)
tasks/shell_crmsh/crm-cib-stonith-level.yml:12
yaml[line-length]: Line too long (110 > 80 characters)
tasks/shell_crmsh/crm-cluster-properties.yml:6
yaml[line-length]: Line too long (82 > 80 characters)
tasks/shell_crmsh/sbd.yml:15
yaml[line-length]: Line too long (92 > 80 characters)
tasks/shell_crmsh/sbd.yml:16
yaml[line-length]: Line too long (81 > 80 characters)
tasks/shell_crmsh/sbd.yml:17
yaml[line-length]: Line too long (93 > 80 characters)
tasks/shell_crmsh/sbd.yml:18
yaml[comments]: Too few spaces before comment
vars/Suse.yml:19
yaml[line-length]: Line too long (82 > 80 characters)
vars/Suse.yml:23
|
If you need help with line wrapping, see https://linux-system-roles.github.io/documentation/howto/working-with-ansible-jinja-code-and-filters.html "How to solve some common ansible-lint issues" |
My ansible lint is not showing line length warnings, but I am updating them now based on Tomas's list. |
How are you running ansible-lint? |
on linux bastion machine, with simple config, which does not include any line skips.
|
|
This repo CI uses the .ansible-lint in the ha_cluster root directory. https://github.com/linux-system-roles/ha_cluster/blob/main/.ansible-lint skip_list:
- fqcn-builtins
- var-naming[no-role-prefix]
- sanity[cannot-ignore] # wokeignore:rule=sanityThe reason why we don't see line length errors in the github action CI is because the latest ansible-lint github action only works on collections - so we have to convert the role to collection format before running ansible-lint - https://github.com/linux-system-roles/ha_cluster/blob/main/.github/workflows/ansible-lint.yml#L37 - when we convert to collection format, we cannot preserve line wrapping (due to ruamel roundtrip), so we disable line-length checking. |
@richm Does that mean we don't care about line length anymore? Should I disable those checks in my tests? I still run ansible-lint and yamllint on the original source files. |
I think we should care about line length - not sure if Galaxy role gating checks. I'm considering putting in a separate github action check to run ansible-lint against the role.
No, I think we should still care about line length.
ansible-lint now runs yamllint, so you shouldn't need a separate check (and yes, ansible-lint will use .yamllint.yml if you have one). |
Co-authored-by: Richard Megginson <[email protected]>
|
[citest] |
defaults/main.yml
Outdated
| + | ||
| (['fence-virt'] if ansible_architecture == 'x86_64' else []) | ||
| }}" | ||
| ha_cluster_fence_agent_packages: [] |
There was a problem hiding this comment.
| ha_cluster_fence_agent_packages: [] | |
| ha_cluster_fence_agent_packages: "{{ __ha_cluster_fence_agent_packages }}" |
There was a problem hiding this comment.
I don't think that is correct place, because it would expose private variable to potential user inputs.
I have added conditional into main yesterday to do switch, depending if users added something to it.
+
ha_cluster_fence_agent_packages
if ha_cluster_fence_agent_packages | length > 0
else __ha_cluster_fence_agent_packages }}"
There was a problem hiding this comment.
I don't think that is correct place, because it would expose private variable to potential user inputs.
I'm not sure what you mean. I believe my proposal encapsulates the desired behavior - putting this in defaults/main.yml:
ha_cluster_fence_agent_packages: "{{ __ha_cluster_fence_agent_packages }}"This allows users to provide their own list of packages for ha_cluster_fence_agent_packages (which is the desired behavior - @tomjelinek please correct me if I'm wrong), and if the user does not specify ha_cluster_fence_agent_packages, it will be set to the default value __ha_cluster_fence_agent_packages. And, since __ha_cluster_fence_agent_packages is defined for different values depending on the platform/version, we get the correct value for ha_cluster_fence_agent_packages for all platforms/versions.
I have added conditional into main yesterday to do switch, depending if users added something to it.
+ ha_cluster_fence_agent_packages if ha_cluster_fence_agent_packages | length > 0 else __ha_cluster_fence_agent_packages }}"
I think this is not the "Ansible way" to do this. I believe the correct way to do this is the way I outlined this above.
There was a problem hiding this comment.
@richm I have changed to in latest commit after conversation with @tomjelinek.
There was a problem hiding this comment.
Looks like the default variable was renamed to __ha_cluster_fence_agent_packages_default, so this should be
ha_cluster_fence_agent_packages: "{{ __ha_cluster_fence_agent_packages_default }}"There was a problem hiding this comment.
@richm Changes are completed and pushed.
There was a problem hiding this comment.
@richm this can be marked as resolved?
|
I can see Workflow test failed on firewall and qdevice, are those CI environments in good shape? PR does not touch firewall or qdevices. |
Tests are failing with the same errors on my systems as well. This needs to be investigated. |
I tried running this PR and existing collection (without PR) on CentOS9, but it fails in different place, which seems as wrong order of steps in pcs. So i cannot re-test this with my current setup. Let me know if you identify something., since it seems you have more variables defined to overcome error below. Issue I have in mine are from pcs attempting to do property change (empty value) on nonexistent CIB |
The firewall failures are caused by not enabling port 1229/tcp. This port is required for fence-virt. This got broken by changes related to |
I see, I have tried to add it to include_vars/os_family yesterday but it resulted in critical nesting, so I moved it to main.yml into "Install cluster packages" as But there are other places that use it before that - firewall and selinux. Where would be best place to do this assignment then if include_vars cannot be used with same syntax? Nesting came from this in include_vars |
@tomjelinek We can simplify it with pre-step in main.yml which avoids nesting and still leaves user input as option. There is also second approach, to always add os_family fence agents on top of user input with | unique, which would be more manageable. |
|
@tomjelinek I have updated code for 2nd approach and it works well by combining both and then running unique on top. selinux: firewall main: |
@marcelmamula That is not going to work. Originally, users could redefine |
Qdevice tests are failing due to corosync-qdevice package not being installed by the role. This is caused by changes in "Install cluster packages" task in |
|
@marcelmamula re: I would try to resolve this by setting fact |
@tomjelinek I have retested using ternary It might be simplest to do set_facts before firewall/selinux block: |
This would work, but it modifies user-defined Also, I would put this task into |
|
@tomjelinek Thank you for suggestion. I have implemented it and retested in rhel, centos and sles4sap and it was working correctly. New commit was pushed for restesting. |
|
[citest] |
I think my proposal #186 (comment) will solve this problem. |
| __ha_cluster_fence_agent_packages_final: | ||
| "{{ (ha_cluster_fence_agent_packages | length == 0) | | ||
| ternary(__ha_cluster_fence_agent_packages_default, | ||
| ha_cluster_fence_agent_packages) }}" |
There was a problem hiding this comment.
This is exactly what my proposal does, without having to resort to set_fact and defining yet another variable.
There was a problem hiding this comment.
Oh, yes. Somehow, I missed your comment about this. @marcelmamula, can you try implementing @richm's proposal regarding this?
There was a problem hiding this comment.
defaults/main.yml does not contain any __private variables. Do you want to expose them there now?
Latest commit has option for users to define fence agents and then decide based on that decision, without having them to to go and replace this assignment in defaults.
There was a problem hiding this comment.
defaults/main.yml does not contain any __private variables. Do you want to expose them there now?
Yes. This is quite a common idiom in system roles.
Latest commit has option for users to define fence agents and then decide based on that decision, without having them to to go and replace this assignment in defaults.
Not sure what you mean. With my proposal:
- if the user defines
ha_cluster_fence_agent_packagesthen the role will use that definition - if the user does not define
ha_cluster_fence_agent_packagesthenha_cluster_fence_agent_packageswill be set to the default value__ha_cluster_fence_agent_packages_defaultand the default value will be used
There was a problem hiding this comment.
There was a problem hiding this comment.
Also note - in Ansible there is no way to "protect" variables - we use the convention that any variable that starts with __ha_cluster_... should not be set by users, and the role is "unsupported" if such variables are set by users.
There was a problem hiding this comment.
@richm Both solution works, but I will update it so we can move past this.
There was a problem hiding this comment.
[citest]
Changes are pushed
There was a problem hiding this comment.
@richm this can be marked as resolved?
|
[citest] is only allowed by role maintainers or linux-system-roles org admins, for security purposes |
Enhancement: This PR adds support for crmsh shell and covers most of its functionalities, with few exceptions.
List of changes:
Following functionalities were not part of current scope:
Reason: Support for SUSE systems and allow wider use, including community.sap_install which is being enabled for SUSE as well.
Result: Testing was successfully completed on RHEL9.2 and SLES for SAP 15 SP5 on AWS. It includes:
Issue Tracker Tickets (Jira or BZ if any): #175