-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: export corosync configuration #231
base: main
Are you sure you want to change the base?
Conversation
[citest] |
I updated the pcs_version vs ubuntu version matrix, as pcs main no longer builds on ubuntu-22.04. And then Python Unit Tests / python (ubuntu-24.04, main) fails when trying to upgrade pip:
I suppose upgrading pip could be removed. But I'm afraid that would solve nothing, as the next command is |
ansible_test fails with this error:
Any idea what this means and how to fix it? |
CentOS-Stream-8|ansible-2.9 fails with I'm not sure why the other CentOS and Fedora tests are marked as failures, when all their logs are success. |
[citest] |
Looking into this, I think it used to work, idk what broke it.
Fixed in linux-system-roles/tft-tests#53, tests passed but some tasks run in background after the testing phase finished, it caused the failure of test plan. Now it's passing. |
Fixing issue with ansible-2.9 on CS8 in linux-system-roles/tft-tests#54 |
[citest] |
8b6546a
to
ab3bf58
Compare
README.md
Outdated
```yaml | ||
- name: Get current cluster configuration | ||
linux-system-roles.ha_cluster.ha_cluster_info: | ||
register: ha_cluster_info_result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System roles, by convention, do not support users using modules directly. In every other role that does something like this, users use the role with either no arguments like https://github.com/linux-system-roles/firewall?tab=readme-ov-file#gathering-firewall-ansible-facts:
- name: Get current cluster configuration
include_role:
name: linux-system-roles.ha_cluster
or with some special variable
- name: Get current cluster configuration
include_role:
name: linux-system-roles.ha_cluster
vars:
ha_cluster_get_info: true
I think the ha_cluster role will have to do something like the latter, since there are numerous public api variables, as opposed to the firewall role which just has the one main firewall
variable. The latter also makes it possible for the role to
- set the state of the cluster and return the cluster configuration
- return a subset of the information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role would then set a global variable e.g. ha_cluster_info
that users would use. This return variable will be declared in the README.md in the section Variables Exported by the Role
e.g. https://github.com/linux-system-roles/kernel_settings?tab=readme-ov-file#variables-exported-by-the-role
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bootloader and snapshot roles export info with <rolename>_facts
variable. Let's be consistent with this naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've been busy with other projects.
I wasn't sure what would be the correct way to expose the export functionality. So I'm glad you pointed me in the right direction. I'm going to implement this change, hopefully in a couple of weeks, once I finish tasks that require my immediate attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed new commits which implement this requested change.
ab3bf58
to
58ae985
Compare
Implementing changes proposed in code review
Implementing changes proposed in code review
Implementing changes proposed in code review
Implementing changes proposed in code review
58ae985
to
5cf96e5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
===========================================
+ Coverage 68.50% 78.94% +10.43%
===========================================
Files 3 6 +3
Lines 181 361 +180
===========================================
+ Hits 124 285 +161
- Misses 57 76 +19 ☔ View full report in Codecov by Sentry. |
[citest] |
Enhancement:
Provide
ha_cluster_info
module to export current cluster configuration. This PR implements a first stage, exporting corosync configuration. Other parts of configuration will follow in other PRs.Reason:
This is the first step in implementing an info module which exports cluster configuration in a variables structure in the same format as ha_cluster role accepts.
Result:
ha_cluster_info module exports corosync configuration, which can be used to recreate the same corosync cluster when passed to the role
Issue Tracker Tickets (Jira or BZ if any):
https://issues.redhat.com/browse/RHEL-46219