fix: Swap roles kubespray-defaults & bootstrap-os

[huangkevin404]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

When using a Flatcar distribution, Python is not installed by default. This is taken care of by the bootstrap-os role.
We need to have Python to successfully execute the kubespray-defaults role.

This PR makes sure role bootstrap-os executes before role kubespray-defaults. The facts that are set by kubespray-defaults are not used by bootstrap-os, but some variables defined in kubespray-defaults/defaults/main might (the doc hints at this, but I did not manage to find any var that are used). If that is the case, we might need to split kubespray-defaults into two roles, one role that only sets vars, then we run bootstrap-os to install Python, and then execute the second role to set the facts (this needs Python).

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Make sure kubespray-defaults can be executed successfully by executing bootstrap-os first

[k8s-ci-robot]

Hi @huangkevin404. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[yankay]

/ok-to-test

[huangkevin404]

If we are confident that the automated tests have sufficient coverage, I updated the docs to reflect that bootstrap-os no longer needs kubespray-defaults.

[VannTen]

I have done quite a bit of refactoring of bootstrap-os some time ago, so it's possible the requirements is gone without we noticing.
Re-reading the bootstap role, I don't why it should not work, indeed.

OTOH:

We need to have Python to successfully execute the kubespray-defaults role.

This should not be the case because of this in the bootstrap role

kubespray/roles/kubespray-defaults/tasks/main.yaml

Lines 1 to 19 in 1798989

	---
	- name: Set facts variables
	# do not run gather facts when bootstrap-os in roles
	when: >
	ansible_play_role_names |
	intersect(['bootstrap-os', 'kubernetes_sigs.kubespray.bootstrap-os']) |
	length == 0
	tags:
	- always
	block:
	- name: Set fallback_ips
	import_tasks: fallback_ips.yml
	when: fallback_ips is not defined
	- name: Set no_proxy
	import_tasks: no_proxy.yml
	when:
	- http_proxy is defined or https_proxy is defined
	- no_proxy is not defined

I'm not sure what exactly led to that implementation, but it's not ideal.

Could you add a commit to remove the check for boostrap-os in kubespray-defaults, since it should not be needed after the reordering ?

[huangkevin404]

Could you add a commit to remove the check for boostrap-os in kubespray-defaults, since it should not be needed after the reordering ?

Are you suggesting that we should remove this check ?

kubespray/roles/kubespray-defaults/tasks/main.yaml

Lines 4 to 7 in 1798989

	when: >
	ansible_play_role_names |
	intersect(['bootstrap-os', 'kubernetes_sigs.kubespray.bootstrap-os']) |
	length == 0

From what I understand, this check could still be useful if someone ever calls the kubespray-defaults role without calling bootstrap-os (which does seems to happen, for example in the recover_control_plane playbook). But you might be more familiar with this check than me.

[VannTen]

Actually, this check if for avoiding doing stuff which require that bootstrap-os has been run before, and it's only needed once (because all other invocation of kubespray-defaults are afterwards, and bootstrap has already run).

Since bootstrap does not need kubespray-defaults AFAICT, reordering means than kubespray defaults will always runs after bootstrap, thus we can drop the check.

EDIT: I suggest you specify that you squash your two commits together, and explain in the commit message that the kubespray-defaults requirements of bootstrap-os is now gone.

[huangkevin404]

Thanks for the explanation @VannTen !
Pushed the new version, waiting to see if CI catches anything.

[VannTen]

Can you properly separate the title of your commit from the message please ?
The splitted phrase between the title and the commit body makes for a pretty bad experience when using git log or blame

[VannTen]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huangkevin404, 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 [VannTen]

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

[VannTen]

/retest-failed

[VannTen]

/retest-failed

[VannTen]

/retest