Remove storage versions flag #67678

caesarxuchao · 2018-08-21T21:18:39Z

This pull follows up #68080. The --storage-versions flag of kube-apiserver is removed.

The --storage-versions flag of kube-apiserver is removed. The storage versions will always be the default value built-in the kube-apiserver binary.

caesarxuchao · 2018-08-21T21:18:56Z

/sig api-machinery

sttts · 2018-08-23T14:59:01Z

/cc @enj

staging/src/k8s.io/apiserver/pkg/server/storage/resource_encoding_config.go

liggitt · 2018-08-23T16:13:06Z

staging/src/k8s.io/apiserver/pkg/server/storage/resource_encoding_config.go

@@ -112,7  100,7 @@ func (o *DefaultResourceEncodingConfig) InMemoryEncodingFor(resource schema.Grou

 	resourceOverride, resourceExists := groupEncoding.InternalResourceEncodings[resource.Resource]
 	if !resourceExists {
-		return groupEncoding.DefaultInternalEncoding, nil
+		return schema.GroupVersion{Group: resource.Group, Version: runtime.APIVersionInternal}, nil


why was this changed?

liggitt · 2018-08-23T16:13:17Z

staging/src/k8s.io/apiserver/pkg/server/storage/resource_encoding_config.go

@@ -94,7  81,8 @@ func (o *DefaultResourceEncodingConfig) StorageEncodingFor(resource schema.Group

 	resourceOverride, resourceExists := groupEncoding.ExternalResourceEncodings[resource.Resource]
 	if !resourceExists {
-		return groupEncoding.DefaultExternalEncoding, nil
+		// return the most preferred external version for the group
+		return o.scheme.PrioritizedVersionsForGroup(resource.Group)[0], nil


same here, didn't expect this

liggitt · 2018-08-23T16:13:45Z

/cc @deads2k

liggitt · 2018-08-23T16:14:31Z

the etcd_storage integration test would be the most interesting test to ensure still works after this

I also expected the flag to be announced/released as deprecated prior to removing it

caesarxuchao · 2018-08-23T17:32:38Z

I'll try to finish this PR and remove the "WIP" today, please do not review the details yet.

I'll take another look at the etcd_storage integration test.

liggitt · 2018-08-24T01:39:50Z

I'll try to finish this PR and remove the "WIP" today, please do not review the details yet.

if the first step is to mark the flag deprecated, then wait for the deprecation period, it seems like the first PR wouldn't be able to remove all of this yet.

caesarxuchao · 2018-08-24T17:28:24Z

/retest

caesarxuchao · 2018-08-24T17:34:02Z

if the first step is to mark the flag deprecated

Can we keep the flag, but make it non-functional, and mark it as deprecated?

It shouldn't break regular users, unless someone reads etcd directly.

If we keep this flag functioning for two more releases, storage migration won't be safe unless users follow instructions to disable this flag.

caesarxuchao · 2018-08-24T17:34:54Z

Tests are passing, especially the etcd_storage_path_test. Removing WIP.

caesarxuchao · 2019-01-11T18:00:19Z

/retest

caesarxuchao · 2019-01-11T18:51:38Z

Rebased. @liggitt PTAL, thanks.

liggitt · 2019-01-11T19:29:56Z

I'll try one more workaround for the storage version migrator e2e test and report back.

was this not successful?

caesarxuchao · 2019-01-11T21:03:04Z

It was successful enough, kubernetes-sigs/kube-storage-version-migrator#16 :)
So this pull was the same as it used to be, just a rebase.

liggitt · 2019-01-11T21:12:35Z

It was successful enough, kubernetes-sigs/kube-storage-version-migrator#16 :)
So this pull was the same as it used to be, just a rebase.

excellent, thanks

liggitt · 2019-01-11T21:14:08Z

/approve cancel
/lgtm cancel

actually, I'm not sure this got reviewed. will queue it up, glad it can progress for 1.14

caesarxuchao · 2019-01-11T21:16:22Z

No, it wasn't reviewed thoroughly.

I'll squash after it's reviewed.

caesarxuchao · 2019-01-29T18:28:11Z

@liggitt PTAL. Thanks.

k8s-ci-robot · 2019-01-29T23:04:36Z

@caesarxuchao: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	b07003f3f71d162d58036a892470480b49943736	link	`/test pull-kubernetes-e2e-kops-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

The storage version now is solely decided by the scheme.PrioritizedVersionsForGroup(). For cohabitating resources, the storage version will be that of the overriding group as returned by storageFactory.getStorageGroupResource().

is either decided by the schema's version priority, or by the per resource override. This fixes a bug where the "batch" group is encoded in v1beta1, which was hidden when --storage-versions is a valid flag.

liggitt

one comment on the variable names. looks reasonable otherwise. leaning heavily on the etcd storage path test (no change needed there is a really good sign)

liggitt · 2019-02-11T20:21:12Z

staging/src/k8s.io/apiserver/pkg/server/storage/resource_encoding_config.go

-	DefaultInternalEncoding   schema.GroupVersion
-	InternalResourceEncodings map[string]schema.GroupVersion
+type OverridingResourceEncoding struct {
+	ExternalResourceEncodings schema.GroupVersion


nit: ExternalResourceEncoding

liggitt · 2019-02-11T20:21:19Z

staging/src/k8s.io/apiserver/pkg/server/storage/resource_encoding_config.go

-	InternalResourceEncodings map[string]schema.GroupVersion
+type OverridingResourceEncoding struct {
+	ExternalResourceEncodings schema.GroupVersion
+	InternalResourceEncodings schema.GroupVersion


nit: InternalResourceEncoding

…up override at all

caesarxuchao · 2019-02-11T23:11:08Z

Fixed the nits. PTAL. Thanks.

liggitt · 2019-02-11T23:14:37Z

/lgtm
/approve

k8s-ci-robot · 2019-02-11T23:14:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, liggitt

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:

~~cmd/kube-apiserver/OWNERS~~ [caesarxuchao,liggitt]
~~hack/jenkins/OWNERS~~ [liggitt]
~~pkg/kubeapiserver/OWNERS~~ [liggitt]
~~pkg/master/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

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

caesarxuchao self-assigned this Aug 21, 2018

k8s-ci-robot requested review from liggitt and sttts August 21, 2018 21:18

caesarxuchao mentioned this pull request Aug 21, 2018

[KEP] storage api version migration kubernetes/community#2524

Merged

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Aug 22, 2018

sttts assigned liggitt and deads2k Aug 22, 2018

caesarxuchao force-pushed the remove-storage-versions-flag branch 2 times, most recently from 0e39962 to 5a84234 Compare August 22, 2018 20:11

k8s-ci-robot requested a review from enj August 23, 2018 14:59

liggitt reviewed Aug 23, 2018

View reviewed changes

staging/src/k8s.io/apiserver/pkg/server/storage/resource_encoding_config.go Show resolved Hide resolved

liggitt reviewed Aug 23, 2018

View reviewed changes

k8s-ci-robot requested a review from deads2k August 23, 2018 16:13

caesarxuchao changed the title ~~[WIP] Remove storage versions flag~~ Remove storage versions flag Aug 24, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 11, 2019

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 11, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2019

caesarxuchao force-pushed the remove-storage-versions-flag branch from 9279333 to b041fdb Compare January 29, 2019 18:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2019

caesarxuchao force-pushed the remove-storage-versions-flag branch from b041fdb to b07003f Compare January 29, 2019 22:23

Chao Xu added 4 commits January 30, 2019 13:28

Remove the --storage-versions flag from kube-apiserver.

1281243

The storage version now is solely decided by the scheme.PrioritizedVersionsForGroup(). For cohabitating resources, the storage version will be that of the overriding group as returned by storageFactory.getStorageGroupResource().

Remove unnecessary group storage version defaults. The storage version

98a1003

is either decided by the schema's version priority, or by the per resource override. This fixes a bug where the "batch" group is encoded in v1beta1, which was hidden when --storage-versions is a valid flag.

generated BUILD files

bed7696

don't run test-update-storage-objects.sh

a76c266

liggitt reviewed Feb 11, 2019

View reviewed changes

Simplify the resource_encoding_config.go, since we don't need per gro…

4ea0708

…up override at all

caesarxuchao force-pushed the remove-storage-versions-flag branch from b07003f to 4ea0708 Compare February 11, 2019 23:09

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2019

k8s-ci-robot merged commit 0ae81c9 into kubernetes:master Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove storage versions flag #67678

Remove storage versions flag #67678

caesarxuchao commented Aug 21, 2018 •

edited

Loading

caesarxuchao commented Aug 21, 2018

sttts commented Aug 23, 2018

liggitt Aug 23, 2018

liggitt Aug 23, 2018

liggitt commented Aug 23, 2018

liggitt commented Aug 23, 2018

caesarxuchao commented Aug 23, 2018

liggitt commented Aug 24, 2018

caesarxuchao commented Aug 24, 2018

caesarxuchao commented Aug 24, 2018

caesarxuchao commented Aug 24, 2018

caesarxuchao commented Jan 11, 2019

caesarxuchao commented Jan 11, 2019

liggitt commented Jan 11, 2019

caesarxuchao commented Jan 11, 2019

liggitt commented Jan 11, 2019 •

edited

Loading

liggitt commented Jan 11, 2019

caesarxuchao commented Jan 11, 2019

caesarxuchao commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

liggitt left a comment

liggitt Feb 11, 2019

liggitt Feb 11, 2019

caesarxuchao commented Feb 11, 2019

liggitt commented Feb 11, 2019

k8s-ci-robot commented Feb 11, 2019

Remove storage versions flag #67678

Remove storage versions flag #67678

Conversation

caesarxuchao commented Aug 21, 2018 • edited Loading

caesarxuchao commented Aug 21, 2018

sttts commented Aug 23, 2018

liggitt Aug 23, 2018

Choose a reason for hiding this comment

liggitt Aug 23, 2018

Choose a reason for hiding this comment

liggitt commented Aug 23, 2018

liggitt commented Aug 23, 2018

caesarxuchao commented Aug 23, 2018

liggitt commented Aug 24, 2018

caesarxuchao commented Aug 24, 2018

caesarxuchao commented Aug 24, 2018

caesarxuchao commented Aug 24, 2018

caesarxuchao commented Jan 11, 2019

caesarxuchao commented Jan 11, 2019

liggitt commented Jan 11, 2019

caesarxuchao commented Jan 11, 2019

liggitt commented Jan 11, 2019 • edited Loading

liggitt commented Jan 11, 2019

caesarxuchao commented Jan 11, 2019

caesarxuchao commented Jan 29, 2019

k8s-ci-robot commented Jan 29, 2019

liggitt left a comment

Choose a reason for hiding this comment

liggitt Feb 11, 2019

Choose a reason for hiding this comment

liggitt Feb 11, 2019

Choose a reason for hiding this comment

caesarxuchao commented Feb 11, 2019

liggitt commented Feb 11, 2019

k8s-ci-robot commented Feb 11, 2019

caesarxuchao commented Aug 21, 2018 •

edited

Loading

liggitt commented Jan 11, 2019 •

edited

Loading