bin_dir: handle flatcar + non root control plane by VannTen · Pull Request #13451 · kubernetes-sigs/kubespray

VannTen · 2026-09-02T09:29:21Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Make bin_dir defaults handle

flatcar -> allow to remove custom variables handling in bootstrap_os and CI
rootless Ansible controller

(this was taken from #12937 and the rootless stuff specifically comes from here, since localhost becomes more important for download in that PR).

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
The default for bin_dir on control node is up to discussion

Does this PR introduce a user-facing change?:

bin_dir for Flatcar is now handled as a default just like for other distribution
bin_dir on the Ansible controller node now defaults to '$HOME/.cache/kubespray'

kubernetes-prow · 2026-09-02T09:29:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

kubernetes-prow · 2026-09-02T09:29:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VannTen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [VannTen]

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

VannTen · 2026-09-03T08:41:35Z

/cc @tico88612
@Payback159

VannTen · 2026-09-03T09:36:07Z

/label tide/merge-method-merge

VannTen · 2026-09-03T09:37:19Z

So the molecule change we're needed, and appears to works outside the PR context, so taking those out as well which should help with reducing the review load (a bit) on #12299

guoard · 2026-09-04T05:54:07Z

Kata molecule verify breaks after merge. Its verify.yml never gathers facts, and test_runtime.yml needs ansible_os_family now for bin_dir. CI didn't see it because kata got into the matrix after this branch.

guoard · 2026-09-04T05:57:56Z

+# TODO handle in bin_dir  when the task is delegated (should be the delegated host value)
+# while respecting defaults (control node is special cased because possibly no access to /usr/*)
+bin_dir: "{{ ('/usr/local/bin' if ansible_os_family != 'Flatcar' else '/opt/bin')
+             if inventory_hostname != 'localhost' else


A node literally named localhost now gets /root/.cache/kubespray for kubelet etc.

I think localhost can only be the ansible controller ?

Maybe instead I could use ansible_user_id == 'root' or ansible_user_id == ansible_become_user ... Wdty ?

I think localhost can only be the ansible controller ?

Yes, but in a all in one setup(localhost ansible_connection=local) localhost is also a node.

Maybe instead I could use ansible_user_id == 'root' or ansible_user_id == ansible_become_user ... Wdty ?

ansible_user_id == 'root' works for me.

I'll go with the ansible_user_id since it seems more aligned with the semantics.

(But does kubespray actually work to deploy a single node cluster on the controller ? I don't think we test that 🤔 )

I'll go with the ansible_user_id since it seems more aligned with the semantics.

The gather needs user as well as distribution.

guoard · 2026-09-04T06:02:59Z

+  - name: Get OS facts (needed for bin_dir resolution)
+    setup:
+      gather_subset:
+      - '!all,!min'


gather_subset: ['!all,!min', 'distribution'] doesn't split, ansible just gathers min. Use the string form '!all,!min,distribution'.

does ['!all', '!min', 'distribution'] works then ? Makes diffs more explicit

Yes, it works.

guoard · 2026-09-04T06:04:55Z

remove-node.yml -e reset_nodes=false breaks the same way. No facts, and pre_remove calls kubectl -> bin_dir. Delegating doesn't help, only the fact cache hides it.
Flatcar gets worse for anyone using the sample inventory. The old vars/flatcar.yml was include_vars and beat group_vars. The new default is a role default and loses to the bin_dir line in inventory/sample/group_vars/all/all.yml.
roles/recover_control_plane/control-plane/defaults/main.yml has its own bin_dir default that shadows this.
docs/operating_systems/flatcar.md still says set bin_dir to /opt/bin. CONTRIBUTING.md and .gitlab-ci/molecule.yml line 16 still point at the deleted molecule_run.sh.
Flatcar CI job is label gated and didn't run.

guoard · 2026-09-04T06:06:24Z

5. Flatcar CI job is label gated and didn't run.

/label ci-extended

VannTen · 2026-09-04T07:41:09Z

remove-node.yml -e reset_nodes=false breaks the same way. No facts, and pre_remove calls kubectl -> bin_dir. Delegating doesn't help, only the fact cache hides it.

Actually, this is currently subtly broken because kubectl (and hence bin_dir) use the value of the original host, not of the delegated to host. I didn't handle it but I should have thought of that. Fixing that + adding a control_plane[0] only facts collection in remove should negate the need for facts collection on removed but non-reset node.

guoard · 2026-09-04T09:26:42Z

Actually, this is currently subtly broken because kubectl (and hence bin_dir) use the value of the original host, not of the delegated to host. I didn't handle it but I should have thought of that. Fixing that + adding a control_plane[0] only facts collection in remove should negate the need for facts collection on removed but non-reset node.

Agreed, that's the right fix. One thing to cover while you're in there: remove-node/remove-etcd-node delegates etcdctl to groups['etcd'] | first, which isn't kube_control_plane[0] with separate etcd, so that host needs facts too.

VannTen · 2026-09-04T09:36:05Z

I have for now:

# resolve variables across delegation
#
# this should point to the correct bin_dir in all contexts,
# but when not using delegation, prefers the usual bin_dir

_delegated_vars: "{{ hostvars[ansible_delegated_vars.keys()[0]] }}"
delegated_bin_dir: "{{
  _delegated_vars['bin_dir'] |
  default(('/usr/local/bin'
            if _delegated_vars['ansible_facts']['os_family'] != 'Flatcar'
            else '/opt/bin')
          if _delegated_vars['ansible_facts']['user_id'] == (_delegated_vars['ansible_become_user'] | d('root'))
          else _delegated_vars['ansible_facts']['env'].HOME + '/.local/bin'
  )
  if ansible_delegated_vars is defined
  else
  bin_dir
}}"

This seems to work, but well... It ain't pretty, to say the least ☹️

- Handle the Flatcar specific bin_dir directly in the default definition of Flatcar instead of having special cases in bootstrap_os and tests/common_vars - Non-root handling is mostly for the Ansible controller, which might run as non-root and without sudo privileges. It's doubtful it works for anything else.

The molecule_run.sh isn't really needed since the switch to gitlab-ci matrix runs. Also use the inventory variables pattern for kata-containers.

VannTen · 2026-09-04T13:25:19Z

Ouch. Apparently the _var_ns stuff does not work before ansible-core 2.19 and the templating rewrite, vars is fully evaluated on templating :/

kubernetes-prow Bot requested review from guoard and mzaian September 2, 2026 09:29

kubernetes-prow Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 2, 2026

kubernetes-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2026

VannTen force-pushed the cleanup/flatcar_specifics branch from c8c72b2 to bbc7021 Compare September 3, 2026 08:38

VannTen marked this pull request as ready for review September 3, 2026 08:41

kubernetes-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2026

kubernetes-prow Bot requested a review from ErikJiang September 3, 2026 08:41

kubernetes-prow Bot requested a review from tico88612 September 3, 2026 08:41

kubernetes-prow Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 3, 2026

kubernetes-prow Bot added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Sep 3, 2026

guoard reviewed Sep 4, 2026

View reviewed changes

kubernetes-prow Bot added the ci-extended Run additional tests label Sep 4, 2026

VannTen marked this pull request as draft September 4, 2026 07:41

kubernetes-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2026

VannTen force-pushed the cleanup/flatcar_specifics branch from 764442b to 519f4a5 Compare September 4, 2026 08:13

VannTen added 2 commits September 4, 2026 15:07

CI/molecule: use common vars as well and simplify run

cf4a322

The molecule_run.sh isn't really needed since the switch to gitlab-ci matrix runs. Also use the inventory variables pattern for kata-containers.

VannTen force-pushed the cleanup/flatcar_specifics branch from 519f4a5 to cf4a322 Compare September 4, 2026 13:09

kubernetes-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 4, 2026

Uh oh!

Conversation

VannTen commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kubernetes-prow Bot commented Sep 2, 2026

Uh oh!

kubernetes-prow Bot commented Sep 2, 2026

Uh oh!

VannTen commented Sep 3, 2026

Uh oh!

VannTen commented Sep 3, 2026

Uh oh!

VannTen commented Sep 3, 2026

Uh oh!

guoard commented Sep 4, 2026

Uh oh!

guoard Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VannTen Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

guoard Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

VannTen Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

guoard Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

guoard Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

VannTen Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

guoard Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

guoard commented Sep 4, 2026

Uh oh!

guoard commented Sep 4, 2026

Uh oh!

VannTen commented Sep 4, 2026

Uh oh!

guoard commented Sep 4, 2026

Uh oh!

VannTen commented Sep 4, 2026

Uh oh!

VannTen commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VannTen commented Sep 2, 2026 •

edited

Loading

guoard Sep 4, 2026 •

edited

Loading