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.

Sign up for GitHub

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

Jump to bottom

Remove (or deprecate) --feature-gates=HighAvailability, SelfHosting, CertsInSecrets #1058

Closed
fabriziopandini opened this issue Aug 15, 2018 · 10 comments
Closed

Remove (or deprecate) --feature-gates=HighAvailability, SelfHosting, CertsInSecrets #1058

fabriziopandini opened this issue Aug 15, 2018 · 10 comments
Assignees
@timothysc @fabriziopandini
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone
v1.12

Comments

@fabriziopandini
Copy link
Member

In the context of Checklist for kubeadm join --control-plane implementation we are going to remove following feature-gates flags HighAvailability, SelfHosting, CertsInSecrets (this Tracking issue for "Config to v1beta1" will benefit from this simplification as well)

We should decide how to do this:

  • remove feature-gates flags from v1.12 or deprecate flags in v1.12 for kubeadm init
  • how to manage in kubeadm updates (preserve flags or change flags/the cluster; how to inform users about this)
  • when to cleanup code
  • other implications

/kind feature
/kind cleanup
/priority important-soon
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 15, 2018
@fabriziopandini fabriziopandini added this to the v1.12 milestone Aug 15, 2018
@stewart-yu
Copy link
Contributor

/cc

@neolit123
Copy link
Member

remove feature-gates flags from v1.12 or deprecate flags in v1.12 for kubeadm init

deprecated state:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go#L72

but a feature gate is also a CLI flag so i'm unsure about the policy:
https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-flag-or-cli

given these FGs are ALPHA it says "0 releases", which means we can deprecate right away?

@chuckha chuckha added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Aug 15, 2018
@neolit123
Copy link
Member

neolit123 commented Aug 15, 2018 •
edited
Loading

as confirmed by api-machinery folks, we can remove these without a deprecation policy since these are alpha. a release note would suffice.

@riverzhang riverzhang mentioned this issue Aug 17, 2018
Closed
@fabriziopandini fabriziopandini mentioned this issue Aug 17, 2018
Closed
11 tasks
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Aug 17, 2018 •
edited
Loading

Summing up comments above, what discussed the 14 of august in sig meeting and the 15 in kubeadm office hours, and my deep dive on the topic I propose the following solution with the goal to:

  • block usage of feature-gates flags HighAvailability, SelfHosting, CertsInSecrets for new clusters starting from v1.12
  • give a warning to users during upgrades to v1.12 of clusters with SelfHosting, CertsInSecrets, but not block; instead upgrades to v1.13 of such clusters will be blocked

Implementation plan:

v1.12 milestone (1 PR)

  • Mark feature-gates flags HighAvailability, SelfHosting, CertsInSecrets as Hidden (not visible in help)
  • Add a new DeprecationMode attribute to feature gates struct for defining how deprecation will happen during upgrades
  • Set DeprecationMode = SilentlyRemove for feature gate HighAvailability, and make this happen during upgrades here
  • Set DeprecationMode = Warning for feature gate SelfHosting, CertsInSecrets, and generate the warning during upgrade plan and upgrade apply here
  • Block usage of feature gates flags with DeprecationMode != None in all CLI commands here
  • clean-up implementation code for HighAvailability (if any)

v1.13 milestone (1 PR)

  • remove feature gate flag HighAvailability
  • Set DeprecationMode = Block for feature gate SelfHosting, CertsInSecrets, and make this happen during upgrades here
  • clean-up implementation code for SelfHosting, CertsInSecrets

v1.14 milestone (1 PR)

  • remove feature gate flags SelfHosting, CertsInSecrets

@neolit123 neolit123 mentioned this issue Aug 17, 2018
Closed
28 tasks
@neolit123
Copy link
Member

neolit123 commented Aug 17, 2018 •
edited
Loading

@fabriziopandini
there is already a deprecated state in the FeatureSpec as far as i can see:
https://github.com/kubernetes/kubernetes//blob/master/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go#L72

so i think we can do this:
PreRelease: utilfeature.Deprecated

and a warning should be printed:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go#L180

but as outlined in my previous commend and the office hours discussion we understood that we don't need to have a deprecation period for alpha gates:

as confirmed by api-machinery folks, we can remove these without a deprecation policy since these are alpha. a release note would suffice.

do you think that the new DeprecationMode and the deprecation policy here is only needed only because we also have to handle upgrades?

i will reserve on voting now, because i don't understand the need for the deprecation yet.
this complicates things for us, but if you really think this is is needed we can go with it, i guess.

@chuckha @liztio @stealthybox

@fabriziopandini
Copy link
Member Author

@neolit123 my only concern is too mitigate impacts on users, and even if we can remove alpha features without advice I think that in case of self hosting we should give proper advice + grace period
For the implementation, the proposed solution consider that kubeadm has its own implementation of feature gates :-( ...

@neolit123
Copy link
Member

my only concern is too mitigate impacts on users, and even if we can remove alpha features without advice I think that in case of self hosting we should give proper advice + grace period

yep, understood.
there is always someone that depends on an alpha feature and will complain if we remove it without notice. it's just that this is extra work for us and someone has to do it.

For the implementation, the proposed solution consider that kubeadm has its own implementation of feature gates :-( ...

/grumble :\

@fabriziopandini
Copy link
Member Author

As for SIG discussion (see meeting notes - August 21 - 2018) we are going to block usage of feature gates and block updates on existing clusters in V1.12

@fabriziopandini
Copy link
Member Author

/lifecycle active

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Aug 22, 2018
@fabriziopandini fabriziopandini mentioned this issue Aug 23, 2018
Merged
hh pushed a commit to ii/kubernetes that referenced this issue Aug 24, 2018
Merge pull request kubernetes#67786 from fabriziopandini/kubeadm-depr…
078961f 
…ecate-featureflags

Automatic merge from submit-queue (batch tested with PRs 67776, 67503, 67679, 67786, 67830). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm - deprecate feature-gates HighAvailability, SelfHosting, CertsInSecrets

**What this PR does / why we need it**:
As for sig discussion (see meeting notes - August 22 - 2018) we are going to block usage of feature gates HighAvailability, SelfHosting, CertsInSecrets for new clusters and block updates to v1.12 of existing clusters using such features.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` 
Fixes # kubernetes/kubeadm#1058

**Special notes for your reviewer**:
I'm going to open issue to track code cleanup in v1.13 

**Release note**:
```release-note
kubeadm - feature-gates HighAvailability, SelfHosting, CertsInSecrets are now deprecated and can't be used anymore for new clusters. Update of cluster using above feature-gates flag is not supported
```
/sig cluster-lifecycle
/kind feature
/kind cleanup
/assign @timothysc
/cc
@timothysc
Copy link
Member

Closing, the deprecation PR has merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timothysc timothysc

@fabriziopandini fabriziopandini

Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Milestone
v1.12
Development

No branches or pull requests
6 participants
@chuckha @timothysc @neolit123 @fabriziopandini @k8s-ci-robot @stewart-yu