Add kubeadm option to etcd_deployment_type to replace the etcd_kubeadm_enabled variable

[necatican]

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #8302

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add kubeadm option to `etcd_deployment_type` to replace the `etcd_kubeadm_enabled` variable (ADD NOTE `etcd_kubeadm_enabled` is deprecated. You can set `etcd_deployment_type` to `kubeadm` to get the same behaviour.")

[k8s-ci-robot]

Hi @necatican. 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/test-infra repository.

[necatican]

There are two issues where I require some guidance :)

1. Where to put etcd_kubeadm_enabled
   If we declare this variable in group_vars/etcd.yml, we need to read it on the kubespray-defaults role. I went ahead and placed it in group_vars/all/all.yml, but that doesn't feel right either. Any ideas on how we should implement it?

Maybe we could place it in the etcd.yml and read it on kubespray-defaults.

2. Should we deprecate or directly remove etcd_kubeadm_enabled
   In my initial commit, I took some initiative and added a small block to roles/kubespray-defaults/tasks/main.yaml. I know Kubespray doesn't follow semver. However, I am not sure if we should introduce a breaking change just like that, even though the method itself is experimental.

/cc cristicalin

[cristicalin]

Overall this looks good, thanks for the effort @necatican !

Where to put etcd_kubeadm_enabled
If we declare this variable in group_vars/etcd.yml, we need to read it on the kubespray-defaults role. I went ahead and placed it in group_vars/all/all.yml, but that doesn't feel right either. Any ideas on how we should implement it?

You don't need to define etcd_kubeadm_enabled anymore in order to facilitate its deprecation. My approach would be a to add a compatibility rule that sets etcd_deployment_type=kubeadm when etcd_kubeadm_enabled is defined and set to True so we don't break existing deployments and issue a deprecation warning in https://github.com/kubernetes-sigs/kubespray/blob/master/roles/kubernetes/preinstall/tasks/0020-verify-settings.yml when etcd_kubeadm_enabled is still defined so deployers get a hint of changes they need to make to their inventory vars.

Should we deprecate or directly remove etcd_kubeadm_enabled
In my initial commit, I took some initiative and added a small block to roles/kubespray-defaults/tasks/main.yaml. I know Kubespray doesn't follow semver. However, I am not sure if we should introduce a breaking change just like that, even though the method itself is experimental.

I think the answer above covers this point as well.

Since we are making a change to the public env vars I would ask you to document this in https://github.com/kubernetes-sigs/kubespray/blob/master/docs/upgrades.md and take the opportunity to describe the etcd deployment types in https://github.com/kubernetes-sigs/kubespray/blob/master/docs/etcd.md

Another point I want to make is that we should get things ready for tagging 2.18 and this feels a bit on a short notice so I would hold it off until we tag 2.18 and merge it after.

/cc @floryut @oomichi what do you think?

[cristicalin]

/ok-to-test

[floryut]

Agree, I plan on drafting the release note (which should be easier as I'll use the k8s release note generator) soon, so let's hold after 2.18 is tag
/hold

[necatican]

Where to put etcd_kubeadm_enabled

I made a typo here, I moved etcd_deployment_type to group_vars/all/all.yml, not the etcd_kubeadm_enabled 🤦‍♂️

I will document the changes and etcd deployment types 😄 Thanks for the feedback!

[cristicalin]

I made a typo here, I moved etcd_deployment_type to group_vars/all/all.yml, not the etcd_kubeadm_enabled man_facepalming

Since it pertains to etcd you can move it to group_vars/all/etcd.yml since this applies to the all group but it points out that functionally it is an etcd related value.

[necatican]

/retest

[cristicalin]

/hold cancel

[oomichi]

Thanks for doing this.
It is nice to merge 2 options into a single one, that is helpful for users also.
Just one question in the line.
The other part seems good for me.

[oomichi]

Why here needs to specify ignore_errors: true?
I could not find cases require it.

[necatican]

Hello,
Thanks for the review! ❤️

I didn't want to fail the playbook for this change. The current implementation warns the user and moves on by setting etcd_deployment_type.

I initially put the ignore_errors statement below the failed_when task, but that didn't make ansible-lint happy.

roles/kubespray-defaults/tasks/main.yaml:28: ignore-errors Use failed_when and specify error conditions instead of using ignore_errors

Should I warn the user and fail the playbook run instead?

[oomichi]

Even if setting etcd_kubeadm_enabled by users and outputting the warning message of line 30, I guess any expected errors don't happen.
I might miss something.

[cristicalin]

The warning message would be better addressed in roles/kubernetes/preinstall/tasks/0020-verify-settings.yml rather than here and have this block just do the set_fact step for backward compatibility.

You can add a #todo comment to clean this when we drop backward compatibility support.

[necatican]

Thank you for the reviews. It makes sense to put the check into verify settings. I was torn between choosing a place but haven't thought about putting those tasks into separate files. 😅

I'm trying to find a better way to alert the user. Throwing a fail stops the playbook, but if I don't fail the task and ignore it, the warning is easy to miss due to being green. I can think of two possible ways and would like to get some guidance on how to resolve this. I couldn't find a good way to issue warnings using Ansible. ☹️

This one is just a simple debug task, it shows up as yellow due to changed_when

- name: Warn the user if they are still using `etcd_kubeadm_enabled`
  debug:
    msg: "WARNING! => `etcd_kubeadm_enabled` is deprecated and will be removed in a future release. You can set `etcd_deployment_type` to `kubeadm` instead of setting `etcd_kubeadm_enabled` to `true`."
  changed_when: etcd_kubeadm_enabled
  run_once: yes
  when: etcd_kubeadm_enabled is defined

We can use the following assert task in the future when we decide to drop the compatibility support.

- name: Warn the user if they are still using `etcd_kubeadm_enabled`
  assert:
    that: not etcd_kubeadm_enabled
    msg: "WARNING! => `etcd_kubeadm_enabled` is deprecated and will be removed in a future release. You can set `etcd_deployment_type` to `kubeadm` instead of setting `etcd_kubeadm_enabled` to `true`."
  changed_when: true
  failed_when: false
  run_once: yes
  when: etcd_kubeadm_enabled is defined

Or should we drop the compatibility support right now and force users to change the variable? It could mess up with someone's automation but should not be a huge issue overall.

cc: @cristicalin @oomichi (Sorry for the second ping. I somehow submitted the previous comment as a review. GitHub deleted the comment itself when I tried to delete the review. 😅)

[cristicalin]

Personally I would not drop backward compatibility at this stage (see the mess I caused with docker to containerd as an example of the fallout).

You can assert any incompatible setting which should stop the playbook like etcd_kubeadm_enabled set to True and etcd_deployment_type != kubeadm. And then output a warning with debug if etcd_kubeadm_enabled is still, unfortunately this is an ansible limitation and I'm not aware of a better way to output deprecation messages in ansible than just using debug. Happy to learn though.

[necatican]

Thanks again ❤️
I have added an assertion to check incompatible settings. It will lay dormant for a while though since we run kubespray-defaults before assertions. However, it might be useful in the future, when we decide to deprecate etcd_kubeadm_enabled.

I also added changed_when: true to debug task, so it will be a bit more noticeable.

[oomichi]

Thanks for updating, now looks good to me.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: necatican, oomichi

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 [oomichi]

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

[oomichi]

/lgtm