Replacing edpm_chrony with linux-systemroles.timesync #373
Conversation
jpodivin
force-pushed
the
timesync
branch
2 times, most recently
from
f0f287e to
f75496a
Compare
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/399e2e41cc2a437eb4fca726b515bb75 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 34m 37s |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/565868dd752c45628600cc54c735a423 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 37m 48s |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/f6c484867ffe4ea485ec1fcc20ca839d ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 04m 14s |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/ff8a9be787e3495a9c251c54bfd217a2 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 06m 31s |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/e61b7267acf24962999e19211f85bbe5 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 20m 09s |
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/f5593539f2fb422187121e3617b9c852 ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 11m 58s |
jpodivin
changed the title
|
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/19d4399489ae4ecab1f780968f15a26e ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 38m 53s |
jpodivin
force-pushed
the
timesync
branch
2 times, most recently
from
720ca9e to
5621dd8
Compare
|
I've updated the docs with some notes on the playbook. The defaults situation is not exactly clean, but it's unavoidable until the install_yamls is patched to use new var names. |
There was a problem hiding this comment.
- What is order you want these patches merged in?
- Replacing edpm_chrony_ntp_servers with timesync_ntp_servers install_yamls#575
- Replacing edpm_chrony_ntp_servers with timesync_ntp_servers dataplane-operator#463
- Replacing edpm_chrony with linux-systemroles.timesync #373
I see you have the following depends-on relationships:
https://github.com/openstack-k8s-operators/dataplane-operator/pull/463
https://github.com/openstack-k8s-operators/edpm-ansible/pull/373
https://github.com/openstack-k8s-operators/install_yamls/pull/575
https://github.com/openstack-k8s-operators/edpm-ansible/pull/373
Shouldn't dataplane-operator go before install_yamls? If so shouldn't the depends-on order be like this?
https://github.com/openstack-k8s-operators/install_yamls/pull/575
https://github.com/openstack-k8s-operators/dataplane-operator/pull/463
https://github.com/openstack-k8s-operators/edpm-ansible/pull/373
Then when the above is done would you patch edpm-ansible to remove the old tripleo chrony role?
I mentioned this before, why change the name of edpm_ntp_servers but have it keep the same structure as edpm_chrony_ntp_servers?
There was a problem hiding this comment.
I'm also not following how this will eventually evolve to be used.
I would instead add the new functionality to use the timesync system role to edpm-ansible, and guard that with a "when" on if timesync_ntp_servers is defined and set.
Then update dataplane-operator and install_yamls to use the system role by defining timesync_ntp_servers. I don't see a reason for edpm_ntp_servers, as we don't want that abstraction over the system role variables. Just use the system role variables directly.
Then followup with an edpm-ansible PR to remove the chrony role, both the role iteself and from the playbooks. You could also drop the "when" at that point guarding the usage of the timesync role, assuming that role does the right thing when timesync_ntp_servers is not defined.
jpodivin
force-pushed
the
timesync
branch
2 times, most recently
from
4dc195e to
95eb7c1
Compare
The edpm_chrony role is getting replaced with timesync system role. This variable changed to list of dictionaries, in order to fit timesync role expectactions. Depends-On: openstack-k8s-operators/edpm-ansible#373 Signed-off-by: Jiri Podivin <[email protected]>
The edpm_chrony role is getting replaced with timesync system role. This variable changed to list of dictionaries, in order to fit timesync role expectactions. Depends-On: openstack-k8s-operators/edpm-ansible#373 Signed-off-by: Jiri Podivin <[email protected]>
The dependent patches should be fine to merge in any order. At least it appears to be the case from the CI. There may be a short period of time when something doesn't quite work. But that would be the case anyway when the backwards compatibility is not an issue. The variable now actually holds a dictionary. |
The edpm_chrony role is getting replaced with timesync system role. This variable changed to list of dictionaries, in order to fit timesync role expectactions. Depends-On: openstack-k8s-operators/edpm-ansible#373 Signed-off-by: Jiri Podivin <[email protected]>
Variables were renamed to reflect that there is no longer any edpm_chrony role to consume them. Default values were changed to accomodate timesync spec. Depends-On: openstack-k8s-operators/edpm-ansible#373 Signed-off-by: Jiri Podivin <[email protected]>
|
The changes that I have requested have been addressed, so thanks for that. As per @slagle's comments: You've added Would you please reply to these comments with an ack or reasons for a nack instead of simply requesting re-review? |
Original proposal was backwards compatible, in that the incoming variable was checked and turned into format the timesync role expected, no matter what was used with the playbook. This one is not backwards compatible. But it has following advantages:
Wrapping original role invocation in conditional is possible, but imho inferior approach. The conditional would have to be implemented not just in one, but in all three playbooks using the This way we would just need to remove If you want backwards compatibility, with no chance of failure, I can restore the original argument checking task, with some adjustment to names of variables. |
There was a problem hiding this comment.
I left a single comment for the code, just a minor issue on tag.
BTW, I'm fine with this approach, as long as it followed pretty soon by a followup PR which removes the edpm_chrony role and its invocation within the playbooks.
Keeping 2 ways to configure ntp it could only generate noise during the CRs generation.
playbooks/install_os.yml
Outdated
| ansible.builtin.include_role: | ||
| name: "{{timesyncfqcn}}" | ||
| tags: | ||
| - edpm_chrony |
There was a problem hiding this comment.
I'd remove edpm_chrony tag with something like timesync or chrony. I don't think that edpm_ prefix is mandatory for the tags.
There was a problem hiding this comment.
I'm fine with the tag being called edpm_chrony since it involes calling the edpm_chrony role. I expect the calls to that role and the tag will be removed soon so I don't think this needs further revision on that level.
There was a problem hiding this comment.
Ok, then edpm_chrony when it calls the edpm_chrony, otherwise the new tag.
Signed-off-by: Jiri Podivin <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpodivin, rebtoor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit f1b8046
into
openstack-k8s-operators:main
No description provided.