Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubeadm: add support for patches in v1beta3; deprecate --experimental-patches #103063

Merged

Conversation

neolit123
Copy link
Member

What this PR does / why we need it:

The feature of "patches" in kubeadm has been in Alpha for a few
releases. It has not received major bug reports from users.
Deprecate the --experimental-patches flag and add --patches.

Both flags are allowed to be mixed with --config.
Add {Init|Join}Configuration.Patches, which is a structure that
contains patch related options. Currently it only has the "Directory"
field which is the same option as the existing --experimental-patches
flag.

The flags --[experimental-]patches value override this value
if both a flag and config is passed during "init" or "join".

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#2046
xref kubernetes/kubeadm#1796

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ACTION REQUIRED: the flag --experimental-patches is now deprecated and will be removed in a future release. You can migrate to using the new flag --patches. Add a new field {Init|Join}Configuration.patches.directory that can be used for the same purpose. For "init" and "join" it is now recommended that you migrate to configure patches via {Init|Join}Configuration.patches.directory. For the time being, these flags can be mixed with --config, but that might change in the future. On a command line, the last *patches flag takes precedence over previous flags and the value in config. "kubeadm upgrade" --patches will continue to be the only available option, since "upgrade" does not support a configuration file yet.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-cluster-lifecycle/kubeadm/1739-customization-with-patches
- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-cluster-lifecycle/kubeadm/970-kubeadm-config

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 21, 2021
@neolit123 neolit123 changed the title kubeadm: add support for patches via confg; deprecate --experimental-patches kubeadm: add support for patches in v1beta3; deprecate --experimental-patches Jun 21, 2021
@k8s-ci-robot k8s-ci-robot requested review from pacoxu and RA489 June 21, 2021 19:38
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 21, 2021
@neolit123
Copy link
Member Author

/kind feature
/priority important-soon
/triage accepted

/cc @fabriziopandini

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 21, 2021
@neolit123 neolit123 mentioned this pull request Jun 21, 2021
16 tasks
@neolit123
Copy link
Member Author

/hold for review

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2021
Copy link
Member

@pacoxu pacoxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ExperimentalPatches override Patches here?
https://github.com/kubernetes/kubernetes/pull/103063/files#diff-40e2eaae5e3828005feeb02733c99a5f90e56a43e249388ee94a893c2141cf42R104

I cannot leave a message inline(github 500😓).

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2021
@neolit123
Copy link
Member Author

Does ExperimentalPatches override Patches here?
https://github.com/kubernetes/kubernetes/pull/103063/files#diff-40e2eaae5e3828005feeb02733c99a5f90e56a43e249388ee94a893c2141cf42R104

I cannot leave a message inline(github 500).

no, basically the order of adding flags does not matter in the code. it matters on the command line.
if both --foo=x --bar=y are passed, and both flags write to the same Go variable, the value of the last flag on the command line --bar will be used. this is mentioned in the release note.

Copy link
Member

@pacoxu pacoxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, pacoxu

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:

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

The feature of "patches" in kubeadm has been in Alpha for a few
releases. It has not received major bug reports from users.
Deprecate the --experimental-patches flag and add --patches.

Both flags are allowed to be mixed with --config.
Add {Init|Join}Configuration.Patches, which is a structure that
contains patch related options. Currently it only has the "Directory"
field which is the same option as the existing --experimental-patches
flag.

The flags --[experimental-]patches value override this value
if both a flag and config is passed during "init" or "join".
@neolit123 neolit123 force-pushed the 1.22-add-patches-to-v1beta3 branch from b84d592 to 70a5246 Compare June 23, 2021 19:24
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 23, 2021
@neolit123
Copy link
Member Author

/cc @SataQiu @pacoxu

i think @fabriziopandini might be on PTO until next week. the 8th of July is code freeze and we should get this merged before then. do you have any more comments here?

@k8s-ci-robot k8s-ci-robot requested review from pacoxu and SataQiu June 29, 2021 14:22
@pacoxu
Copy link
Member

pacoxu commented Jun 30, 2021

According to kubernetes/kubeadm#2046,

  • add --patches
  • mark --experimental-patches as deprecated
  • disallow mixture of --patches and --config
  • add Init|JoinConfiguration.patches.directory to v1beta3

disallow mixture of --patches and --config is not implemented in this PR. However, it can be fixed standalone if needed.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2021
@neolit123
Copy link
Member Author

neolit123 commented Jun 30, 2021 via email

@pacoxu
Copy link
Member

pacoxu commented Jun 30, 2021

We may disallow it in 1.23 for compatibility.

LGTM then.

@pacoxu
Copy link
Member

pacoxu commented Jul 1, 2021

/hold cancel
/retest

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2021
@pacoxu
Copy link
Member

pacoxu commented Jul 1, 2021

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants