Refactor bootstrap-os #10983

VannTen · 2024-03-06T17:59:48Z

What type of PR is this?
/kind design
/kind cleanup

What this PR does / why we need it:

Remove no longer needed workarounds
Don't set_fact for ansible_python_interpreter (I need this to have a proper virtualenv for Migrate DNS manifests to kubernetes.core.k8s #10701
Refactor the os_release -> "include which files" logic

Special notes for your reviewer:
I'll submit the ansible upgrade in a separate but first fount is broken on 2.15

Does this PR introduce a user-facing change?:

NONE

VannTen · 2024-03-12T12:19:51Z

actually include_vars still override tasks vars, so this won't help...

I wonder if we could rely more on ansible python autodiscovery since I guess it has improved since the time where ansible_python_interpreter workarounds was added in bootstrap 🤔
@floryut @mzaian @MrFreezeex Opinions ?

Coreos was replaced by flatcar in 058438a but the file was copied instead of moved.

Using directly ID and VARIANT_ID with first_found allow for less manual includes. Distro "families" are simply handled by symlinks.

- Allows users to override the chosen python_interpreter with group_vars easily (group_vars have lesser precedence than facts) - Allows us to use vars at the task scope to use a virtual env Ansible python discovery has improved, so those workarounds should not be necessary anymore. Special workaround for Flatcar, due to upstream ansible not willing to support it.

VannTen · 2024-03-26T14:18:59Z

/cc @MrFreezeex

MrFreezeex · 2024-03-26T16:42:34Z

roles/bootstrap-os/tasks/main.yml

+    - &search
+      files:
+      - "{{ os_release_dict['ID'] }}-{{ os_release_dict['VARIANT_ID'] }}.yml"
+      - "{{ os_release_dict['ID'] }}.yml"


It's a bit a nit but I kinda like the fact there was a prefix "bootstrap-" there but I am fine with that if you prefer this.

I tend to dislike repeated prefix stuff when there is already a "namespace" (roles/boostrap-os/tasks/bootstrap-*), it tends to make me cringe 😄 . But that's not something really important, I'm fine with either way.

Hmm well to me that was more something to quickly distinguish the code that loads through this than the rest but as it's only main.yml it's not super important either

Oh yeah I see that you mean. In that case I would rather have something like tasks/os/{{ distrib }}. But since we currently only have the distro specific tasks / vars here, I think that's not really needed for now.

MrFreezeex

Thanks for the very nice cleanup :D
/lgtm

VannTen · 2024-03-27T09:50:20Z

/assign @liupeng0518 @mzaian

mzaian

Thanks a lot @VannTen and @MrFreezeex for the review

/lgtm

k8s-ci-robot · 2024-03-27T12:55:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrFreezeex, mzaian, VannTen

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:

~~OWNERS~~ [mzaian]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

c58497c (Refactor bootstrap-os (kubernetes-sigs#10983), 2024-03-27) refactored the boostrap-os include but didn't adapt the amazon linux tasks to the actual ID of amazon linux ('amzn') Re-enable the CI so we can avoid that kind of breakage.

* Remove leftover files for Coreos Coreos was replaced by flatcar in 058438a but the file was copied instead of moved. * Remove workarounds for resolved ansible issues * boostrap: Use first_found to include per distro Using directly ID and VARIANT_ID with first_found allow for less manual includes. Distro "families" are simply handled by symlinks. * boostrap: don't set ansible_python_interpreter - Allows users to override the chosen python_interpreter with group_vars easily (group_vars have lesser precedence than facts) - Allows us to use vars at the task scope to use a virtual env Ansible python discovery has improved, so those workarounds should not be necessary anymore. Special workaround for Flatcar, due to upstream ansible not willing to support it.

c58497c (Refactor bootstrap-os (#10983), 2024-03-27) refactored the boostrap-os include but didn't adapt the amazon linux tasks to the actual ID of amazon linux ('amzn') Re-enable the CI so we can avoid that kind of breakage.

* Remove leftover files for Coreos Coreos was replaced by flatcar in 058438a but the file was copied instead of moved. * Remove workarounds for resolved ansible issues * boostrap: Use first_found to include per distro Using directly ID and VARIANT_ID with first_found allow for less manual includes. Distro "families" are simply handled by symlinks. * boostrap: don't set ansible_python_interpreter - Allows users to override the chosen python_interpreter with group_vars easily (group_vars have lesser precedence than facts) - Allows us to use vars at the task scope to use a virtual env Ansible python discovery has improved, so those workarounds should not be necessary anymore. Special workaround for Flatcar, due to upstream ansible not willing to support it.

c58497c (Refactor bootstrap-os (kubernetes-sigs#10983), 2024-03-27) refactored the boostrap-os include but didn't adapt the amazon linux tasks to the actual ID of amazon linux ('amzn') Re-enable the CI so we can avoid that kind of breakage.

* Remove leftover files for Coreos Coreos was replaced by flatcar in 058438a but the file was copied instead of moved. * Remove workarounds for resolved ansible issues * boostrap: Use first_found to include per distro Using directly ID and VARIANT_ID with first_found allow for less manual includes. Distro "families" are simply handled by symlinks. * boostrap: don't set ansible_python_interpreter - Allows users to override the chosen python_interpreter with group_vars easily (group_vars have lesser precedence than facts) - Allows us to use vars at the task scope to use a virtual env Ansible python discovery has improved, so those workarounds should not be necessary anymore. Special workaround for Flatcar, due to upstream ansible not willing to support it.

c58497c (Refactor bootstrap-os (kubernetes-sigs#10983), 2024-03-27) refactored the boostrap-os include but didn't adapt the amazon linux tasks to the actual ID of amazon linux ('amzn') Re-enable the CI so we can avoid that kind of breakage.

k8s-ci-robot added kind/design Categorizes issue or PR as related to design. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Mar 6, 2024

k8s-ci-robot requested review from MrFreezeex and yankay March 6, 2024 17:59

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 6, 2024

VannTen mentioned this pull request Mar 6, 2024

Upgrade ansible-core to 2.16.4 #10984

Merged

VannTen force-pushed the design/refactor_boostrap branch 8 times, most recently from be82f54 to ff9539c Compare March 8, 2024 19:49

VannTen marked this pull request as draft March 12, 2024 12:20

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2024

VannTen added 3 commits March 14, 2024 11:24

Remove leftover files for Coreos

bcb514b

Coreos was replaced by flatcar in 058438a but the file was copied instead of moved.

Remove workarounds for resolved ansible issues

f949ae9

boostrap: Use first_found to include per distro

fd8b8d9

Using directly ID and VARIANT_ID with first_found allow for less manual includes. Distro "families" are simply handled by symlinks.

VannTen mentioned this pull request Mar 14, 2024

Add /opt/bin/python to python fallback list ansible/ansible#82821

Closed

VannTen force-pushed the design/refactor_boostrap branch from ff9539c to 482eb9f Compare March 14, 2024 13:03

VannTen force-pushed the design/refactor_boostrap branch from 482eb9f to 55f2aca Compare March 15, 2024 09:14

VannTen marked this pull request as ready for review March 15, 2024 09:20

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2024

k8s-ci-robot requested review from cyclinder and mzaian March 15, 2024 09:20

VannTen mentioned this pull request Mar 21, 2024

Cleanup of kubernetes/preinstall #11010

Merged

MrFreezeex reviewed Mar 26, 2024

View reviewed changes

MrFreezeex approved these changes Mar 27, 2024

View reviewed changes

k8s-ci-robot assigned MrFreezeex Mar 27, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2024

k8s-ci-robot assigned liupeng0518 and mzaian Mar 27, 2024

mzaian approved these changes Mar 27, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2024

k8s-ci-robot merged commit c58497c into kubernetes-sigs:master Mar 27, 2024
60 checks passed

mzaian mentioned this pull request Apr 26, 2024

Release Proposal v2.25 #11126

Closed

oboudry-mvp mentioned this pull request Jun 5, 2024

Failure to upgrade a flatcar cluster due to patch "Refactor bootstrap-os (#10983)" #11268

Closed

snowmansora mentioned this pull request Jun 17, 2024

HTTP proxy not working on Rocky Linux (& likely other OS) after pull/10983 #11301

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor bootstrap-os #10983

Refactor bootstrap-os #10983

VannTen commented Mar 6, 2024 •

edited

Loading

VannTen commented Mar 12, 2024

VannTen commented Mar 26, 2024

MrFreezeex Mar 26, 2024

VannTen Mar 27, 2024 •

edited

Loading

MrFreezeex Mar 27, 2024 •

edited

Loading

VannTen Mar 27, 2024

MrFreezeex left a comment

VannTen commented Mar 27, 2024 via email

mzaian left a comment

k8s-ci-robot commented Mar 27, 2024

Refactor bootstrap-os #10983

Refactor bootstrap-os #10983

Conversation

VannTen commented Mar 6, 2024 • edited Loading

VannTen commented Mar 12, 2024

VannTen commented Mar 26, 2024

MrFreezeex Mar 26, 2024

Choose a reason for hiding this comment

VannTen Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

MrFreezeex Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

VannTen Mar 27, 2024

Choose a reason for hiding this comment

MrFreezeex left a comment

Choose a reason for hiding this comment

VannTen commented Mar 27, 2024 via email

mzaian left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 27, 2024

VannTen commented Mar 6, 2024 •

edited

Loading

VannTen Mar 27, 2024 •

edited

Loading

MrFreezeex Mar 27, 2024 •

edited

Loading