Cleanup master terminology (start fixing issue #11353)

[bogd]

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 cleanup

What this PR does / why we need it:
Cleanup old "master" terminology in kubespray

Which issue(s) this PR fixes:

Special notes for your reviewer:
As per comments on issue #11353 , I have not yet touched any potentially user-facing components (tags, variables, etc). They will eventually need modifying as well, in a future step.

When that happens, it will require a more detailed release note, but I do not know yet if that can be done in the same PR or will need a new one.
Any potentially breaking changes were kept back for a future PR.

Does this PR introduce a user-facing change?:

Cleanup older terminology, replace "master" with "control plane"

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bogd / name: Bogdan Sass (345eec3)

[k8s-ci-robot]

Welcome @bogd!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @bogd. 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

[bogd]

Regarding tags, I would like to proceed as suggested in the issue comment ( #11353 ) - add control-plane tags while keeping the master tag intact (for people who reference the tag manually).

However, I am not sure how to proceed here.

The line is { role: kubernetes/control-plane, tags: master, upgrade_cluster_setup: true }, but there seem to (no longer?) be any tasks in kubernetes/control-plane with the master tag.

[ Edit: I misunderstood what the tag does here - it applies to the include_role task, not to the role itself. So we are in the situation described in the issue discussion - and back to "not sure how to proceed in fixing this". :)

Replacing the tag might impact people who call kubespray playbooks with the tag set.
Maybe we could call the include twice, once for each tag (master and control-plane) until we deprecate the old tag? ]

[mzaian]

Regarding tags, I would like to proceed as suggested in the issue comment ( #11353 ) - add control-plane tags while keeping the master tag intact (for people who reference the tag manually).

However, I am not sure how to proceed here.

The line is { role: kubernetes/control-plane, tags: master, upgrade_cluster_setup: true }, but there seem to (no longer?) be any tasks in kubernetes/control-plane with the master tag.

[ Edit: I misunderstood what the tag does here - it applies to the include_role task, not to the role itself. So we are in the situation described in the issue discussion - and back to "not sure how to proceed in fixing this". :)

Replacing the tag might impact people who call kubespray playbooks with the tag set. Maybe we could call the include twice, once for each tag (master and control-plane) until we deprecate the old tag? ]

Hello @bogd

We could just highlight this in the release notes by adding a deprecation warning. People should take care of it when they use the new release.

[bogd]

Thank you, @mzaian !

Since this PR already touches multiple files, I would like to stick to "no user-facing changes" for it.

My plan is to change as much as possible without any potentially breaking changes, and keep those changes for separate PR(s) - smaller ones, easier to follow.

This means that I will not touch variables and tag references (yet). Once this is merged, I will start work on the more... troublesome changes in a separate PR. In fact, this is why the PR is marked as "start fixing issue" :)

As before, please let me know if I am missing something, or if you think I should go about this a different way.

[mzaian]

Thank you, @mzaian !

Since this PR already touches multiple files, I would like to stick to "no user-facing changes" for it.

My plan is to change as much as possible without any potentially breaking changes, and keep those changes for separate PR(s) - smaller ones, easier to follow.

This means that I will not touch variables and tag references (yet). Once this is merged, I will start work on the more... troublesome changes in a separate PR. In fact, this is why the PR is marked as "start fixing issue" :)

As before, please let me know if I am missing something, or if you think I should go about this a different way.

I really like this approach small PRs for critical things. I have no comments but Thank you for taking care of this :)

[VannTen]

The recommendation survives under sig-architecture [here](https://github.com/kubernetes/community/blob/master/sig-architecture/naming/recommendations/001-master-control-plane.md), but I am not sure how to refer to this in a commit message.

Putting that link into the commit message is perfectly acceptable (permalink preferably)

[bogd]

Quick question - if g in group_names is preferred, can't this check also be replaced?

Original:

 when:
    - inventory_hostname in groups['etcd'] | union(groups['kube_control_plane']) | unique | sort

Alternative:

 when:
    - ('etcd' in group_names) or ('kube_control_plane' in group_names)

There are multiple inventory_hostname in groups[] checks, but this one stood out in particular.

I am not trying to expand the scope of this PR (it is already big enough :) ), I am just trying to keep consistency throughout the files.

[VannTen]

Yeah it's fine. I usually do stuff like this : group_names | intersect([<list of group_names>] | length != 0 but your way is perfectly fine as well, and certainly better than the inventory_hostname one.

[VannTen]

No need for parens here (or anywhere else if the check is standalone). Check operators have a pretty high precedence, I think.

[bogd]

The parentheses are there for another reason - to "escape" the quotation (otherwise, it is interpreted as a YAML quote, and the YAML parser fails to interpret the value).

This is the way I normally work around this "quote at start of YAML value" problem. If you know a better way to do it, I will implement it.

I know it is still a problem because in a previous commit I forgot to add it, and all the CI tests started failing :)

[VannTen]

Oh that's why.
I've seen mostly using the other kind of quote for that ("'etcd' in group_names") but parens look fine to me ; it can be confused precedence override, but OTOH the quotes muck up syntax highlighting, so that's a pick your poison scenario I guess.

[bogd]

For me, using two kinds of quotes is a bit less readable - I am the kind of person that always prefers explicit parentheses to relying on operator precedence, so probably that is why :)

(I am also the kind of person that often edits YAMLs in vim without syntax highlighting, but I guess that says more about my age than about anything else)

[VannTen]

same for kube_control_plane on each occurence.

[bogd]

Same reason as above - if I do not do something about the quote, the YAML parser will fail when interpreting the line.

[VannTen]

I'm fine with the content.
Can you reformat your commit message though ? Not strictly respecting git recommended title length is fine, but you should still separate the tiltle of your commit from the body with a blank line.
I suggest dropping the (part of #11353), ref to issues and PR are not super useful in commit message.

Thanks for the work 👍

[VannTen]

@yankay @mzaian I'm fine with putting this in 2.26 if you want, but it's a rather large change, do you prefer to defer until after the release ?

[mzaian]

@yankay @mzaian I'm fine with putting this in 2.26 if you want, but it's a rather large change, do you prefer to defer until after the release ?

I think no one tested this with different scenarios. It should be tested and then maybe go with the future 2.27 it's a bit late for 2.26

[VannTen]

Yeah that seems reasonable. /hold (I'll remove this once 2.26 is cut, we can still review until then)

[VannTen]

Thanks !
/lgtm

[bogd]

Thanks ! /lgtm

Thank you for the help, and the guidance in getting a new contributor up to speed :)

[VannTen]

/hold cancel