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

Deprecation notice of the storage-versions flag #68080

Merged
merged 1 commit into from Sep 5, 2018

Conversation

@caesarxuchao
Copy link
Member

caesarxuchao commented Aug 30, 2018

This PR deprecates the --storage-versions flag of kube-apiserver. The flag allows user to change the API version the content in etcd will be serialized to.

The flag provides unnecessary flexibility, with the side-effect of making kube-apiserver upgrades/downgrades hard to reason about. Specifically, Kubernetes follows the 4-step API version deprecation policy, which makes upgrades/downgrades across minor versions safe as long as all the data in etcd is encoded to the default storage versions. However, if users can specify their own storage versions, then the safety of each upgrade/downgrade needs to be analyzed case-by-case.

Action required: The --storage-versions flag of kube-apiserver is deprecated. Please omit this flag to ensure the default storage versions are used. Otherwise the cluster is not safe to upgrade to a version newer than 1.12. This flag will be removed in 1.13.

/assign @deads2k @liggitt @lavalamp @enj
/sig api-machinery

@@ -96,6 +96,8 @@ func (s *StorageSerializationOptions) AddFlags(fs *pflag.FlagSet) {
// Note: the weird ""+ in below lines seems to be the only way to get gofmt to
// arrange these text blocks sensibly. Grrr.
fs.StringVar(&s.StorageVersions, "storage-versions", s.StorageVersions, ""+
"DEPRECATED. Please avoid using this flag. "+

This comment has been minimized.

@enj

enj Aug 30, 2018

Member

Needs fs.MarkDeprecated(...)

This comment has been minimized.

@caesarxuchao

caesarxuchao Aug 30, 2018

Author Member

Thanks! PTAL.

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 30, 2018

#67678 removes the functionality of the flag. I still think we can skip the normal deprecation process (see #67678 (comment)) for the --storage-versions flag, and it makes users' clusters safer. However, it seems no one has the bandwidth to consider #67678 and code freeze is imminent, so I just follow the process and deprecate the flag in this release.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:deprecation-notice-storage-versions branch from 80a753c to 25a5390 Aug 30, 2018

@@ -103,6 +103,9 @@ func (s *StorageSerializationOptions) AddFlags(fs *pflag.FlagSet) {
"You only need to pass the groups you wish to change from the defaults. "+
"It defaults to a list of preferred versions of all known groups.")

fs.MarkDeprecated("storage-versions", "Please avoid using this flag. "+

This comment has been minimized.

@enj

enj Aug 30, 2018

Member

This message is not particularly helpful, though I am not sure what we can say here that will be actionable.

This comment has been minimized.

@liggitt

liggitt Aug 30, 2018

Member

something like "omit this flag to use the default storage versions. this flag will be removed in 1.13." might be better

This comment has been minimized.

@enj

enj Aug 30, 2018

Member

But if they have been using this flag, they need to run some form of storage migration to fix it before upgrading...

This comment has been minimized.

@liggitt

liggitt Aug 30, 2018

Member

But if they have been using this flag, they need to run some form of storage migration to fix it before upgrading...

not really... whatever versions they were storing before must be able to be encoded/decoded by the apiserver, and we haven't removed the ability of the apiserver to decode any types it could decode in 1.11

This comment has been minimized.

@liggitt

liggitt Aug 30, 2018

Member

the point of removing this is so that we can know what is being encoded, so that in a future release we can tell them to run a storage migration tool, know versions x,y,z are stored, then safely remove versions a,b,c

@enj

This comment has been minimized.

Copy link
Member

enj commented Aug 30, 2018

Is anything other than a release note + flag deprecation required?

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Aug 30, 2018

Is anything other than a release note + flag deprecation required?

I don't think so. The server will be able to read and write the storage once it is gone. Eventually we'll want to sweep with the new tool that chao is working on, but it doesn't need to be immediate and the problem isn't new

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Aug 30, 2018

/lgtm

/hold

hold for @lavalamp and potentially making that release note action-required.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:deprecation-notice-storage-versions branch from 25a5390 to d7767ae Aug 30, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 30, 2018

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:deprecation-notice-storage-versions branch from d7767ae to 8cf77c6 Aug 30, 2018

fs.MarkDeprecated("storage-versions", ""+
"Please omit this flag to use the default storage versions. "+
"Otherwise the cluster is not safe to upgrade to a version newer than 1.12. "+
"This flag will be removed in 1.13.")

This comment has been minimized.

@caesarxuchao

caesarxuchao Aug 30, 2018

Author Member

Integrated Jordan's suggestion to make the message clearer.

@enj

This comment has been minimized.

Copy link
Member

enj commented Aug 30, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 30, 2018

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 30, 2018

/lgtm

(wanted to say retest..)

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Aug 30, 2018

@caesarxuchao: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 30, 2018

/retest

@@ -103,6 +103,11 @@ func (s *StorageSerializationOptions) AddFlags(fs *pflag.FlagSet) {
"You only need to pass the groups you wish to change from the defaults. "+
"It defaults to a list of preferred versions of all known groups.")

fs.MarkDeprecated("storage-versions", ""+
"Please omit this flag to use the default storage versions. "+

This comment has been minimized.

@lavalamp

lavalamp Aug 30, 2018

Member

even clearer: "Please omit this flag to ensure the default storage versions are used."

This comment has been minimized.

@caesarxuchao

caesarxuchao Aug 31, 2018

Author Member

Done. PTAL, thanks.

@lavalamp lavalamp removed the lgtm label Aug 30, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Aug 30, 2018

Yes, please mark it action required. lgtm with a minor wording change.

/unhold

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:deprecation-notice-storage-versions branch from 8cf77c6 to 1fb6b5a Aug 31, 2018

@lavalamp lavalamp added this to the v1.12 milestone Sep 4, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Sep 4, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 4, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 4, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, deads2k, enj, lavalamp

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

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Sep 4, 2018

/hold cancel

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Sep 5, 2018

This PR is lgtm'ed before the code freeze started (Sep.4 EOD). I'll manually apply the priority/critical-urgent label to get it merged.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Sep 5, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Sep 5, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit ad1721e into kubernetes:master Sep 5, 2018

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation caesarxuchao authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.