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

Remove storage versions flag #67678

Merged

Conversation

@caesarxuchao
Copy link
Member

caesarxuchao commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 21, 2018

/sig api-machinery

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

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Aug 23, 2018

/cc @enj

@k8s-ci-robot k8s-ci-robot requested a review from enj Aug 23, 2018

@@ -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

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 23, 2018

Member

why was this changed?

@@ -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

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 23, 2018

Member

same here, didn't expect this

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Aug 23, 2018

/cc @deads2k

@k8s-ci-robot k8s-ci-robot requested a review from deads2k Aug 23, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Aug 23, 2018

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

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 23, 2018

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

This comment has been minimized.

Copy link
Member

liggitt commented Aug 24, 2018

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

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 24, 2018

/retest

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 24, 2018

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

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Aug 24, 2018

Tests are passing, especially the etcd_storage_path_test. Removing WIP.

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

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Jan 11, 2019

Rebased. @liggitt PTAL, thanks.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 11, 2019

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

was this not successful?

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Member

liggitt commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Member

liggitt commented Jan 11, 2019

/approve cancel
/lgtm cancel

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

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Jan 11, 2019

No, it wasn't reviewed thoroughly.

I'll squash after it's reviewed.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:remove-storage-versions-flag branch from 9279333 to b041fdb Jan 29, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Jan 29, 2019

@liggitt PTAL. Thanks.

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:remove-storage-versions-flag branch from b041fdb to b07003f Jan 29, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 29, 2019

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws b07003f 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.

caesarxuchao added some commits Aug 21, 2018

Remove the --storage-versions flag from kube-apiserver.
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
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
Copy link
Member

liggitt left a comment

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)

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

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 11, 2019

Member

nit: ExternalResourceEncoding

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

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 11, 2019

Member

nit: InternalResourceEncoding

@caesarxuchao caesarxuchao force-pushed the caesarxuchao:remove-storage-versions-flag branch from b07003f to 4ea0708 Feb 11, 2019

@caesarxuchao

This comment has been minimized.

Copy link
Member Author

caesarxuchao commented Feb 11, 2019

Fixed the nits. PTAL. Thanks.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 11, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 11, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 11, 2019

[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:

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

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

17 checks passed

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-godeps Job succeeded.
Details
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
pull-publishing-bot-validate Skipped
tide In merge pool.
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.