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 use of alpha initializers #72972

Merged
merged 5 commits into from Jan 24, 2019

Conversation

@liggitt
Copy link
Member

liggitt commented Jan 16, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

  • Marks the alpha metadata.Initializers field deprecated and removes all in-tree use of it
  • Removes the admissionregistration.k8s.io/v1alpha1 API group, containing only the InitializationConfiguration type
  • Removes the IncludeUninitialized field from *Options
  • Removes the alpha Initializers admission plugin
  • Removes use of Initializers fields from the cloud-controller-manager persistent volume labeler controller (stubs the needsInitialization() method to return false, and the markInitialized() method to no-op). This controller will either need to work off of a different attribute (like an annotation), or become an admission webhook.

Which issue(s) this PR fixes:
Fixes #67113

Special notes for your reviewer:

There are no plans to graduate the initializers feature, so removing it sooner rather than later sends a clearer signal to the community that new features should not be built anticipating its promotion.

Does this PR introduce a user-facing change?:

The alpha Initializers feature, `admissionregistration.k8s.io/v1alpha1` API version, `Initializers` admission plugin, and use of the `metadata.initializers` API field have been removed. Discontinue use of the alpha feature and delete any existing `InitializerConfiguration` API objects before upgrading. The `metadata.initializers` field will be removed in a future release.

/sig api-machinery
/cc @lavalamp @deads2k @smarterclayton @caesarxuchao

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 16, 2019

added to the sig-api-machinery meeting agenda on 1/16 for discussion

@liggitt liggitt force-pushed the liggitt:remove-alpha-initializers branch from e731e22 to c657236 Jan 16, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Jan 16, 2019

Did we want to keep the field so that we don't break downstream and remove it in a future release?

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 16, 2019

Did we want to keep the field so that we don't break downstream and remove it in a future release?

Current state:

  • feature is disabled by default
  • when the feature is disabled, any metadata.initializers sent to the apiserver are ignored and not persisted
  • if a local manifest has a metadata.initializers field present, kubectl client-side validation will not complain because the field appears in the openapi schema

We can consider keeping the field in metadata and unconditionally clearing it server-side, but I don't see what would make it safer to remove in a future release than it is now

I'd prefer to remove it from the schema to avoid generating misleading clients

@liggitt liggitt force-pushed the liggitt:remove-alpha-initializers branch from c657236 to 18018ed Jan 16, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 16, 2019

@kubernetes/sig-cloud-provider-pr-reviews
for impact on PV label controller which was built on the alpha initializers feature

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Jan 16, 2019

@liggitt is this targeting v1.14?

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 16, 2019

yes

liggitt added some commits Jan 17, 2019

@liggitt liggitt force-pushed the liggitt:remove-alpha-initializers branch from 6f3d973 to 1a15d80 Jan 23, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 23, 2019

/retest

// see if the PVL is the next in line.
return initializers.Pending[0].Name == name
func needsInitialization(vol *v1.PersistentVolume) bool {
// TODO: determine whether initialization is required based on a different attribute,

This comment has been minimized.

@andrewsykim

andrewsykim Jan 23, 2019

Member

Just a note for reference: this should ideally be done for v1.14 (in a future PR though)

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 24, 2019

this is ready, local-e2e is red for an unrelated reason

@caesarxuchao

This comment has been minimized.

Copy link
Member

caesarxuchao commented Jan 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 24, 2019

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 24, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 24, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit e28c757 into kubernetes:master Jan 24, 2019

17 of 18 checks passed

pull-kubernetes-local-e2e Job triggered.
Details
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
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-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
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-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 24, 2019

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

Test name Commit Details Rerun command
pull-kubernetes-local-e2e 1a15d80 link /test pull-kubernetes-local-e2e

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.

@liggitt liggitt deleted the liggitt:remove-alpha-initializers branch Jan 25, 2019

@akutz

This comment has been minimized.

Copy link
Member

akutz commented Jan 25, 2019

FYI, this has impacted all VMware ci/latest conformance tests. I feel that such breaking changes should be more broadly advertised, Alpha or not. This feature has been part of example config since 1.10, and at that point calling something Alpha has the same effect as the Winamp or ICQ betas :)

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 25, 2019

Thanks for the feedback, I'll send an announcement to kubernetes-dev as well, and bump the release note to action-required

edit: posted to https://groups.google.com/forum/#!topic/kubernetes-dev/6gO_CxEOn5s

@akutz

This comment has been minimized.

Copy link
Member

akutz commented Jan 25, 2019

Hi @liggitt,

Thank you. I feel like I'm the Alpha-thorn in your side :(

FWIW, I used this config bit because of Kelsey's guide. I'll file a PR with his repo as well.

Thank you Jordan!

--
-a

akutz added a commit to akutz/kubernetes-the-hard-way that referenced this pull request Jan 25, 2019

Remove Initializers from API plug-ins
This patch removes the `Initializers` from the list of the API server's
admissions plug-ins in order to be compatible with
kubernetes/kubernetes#72972.

akutz added a commit to akutz/kubernetes-the-hard-way that referenced this pull request Jan 25, 2019

Remove Initializers from API plug-ins
This patch removes the `Initializers` from the list of the API server's
admissions plug-ins in order to be compatible with
kubernetes/kubernetes#72972.

akutz added a commit to akutz/kubernetes-the-hard-way that referenced this pull request Jan 25, 2019

Remove Initializers from API plug-ins
This patch removes `Initializers` from the list of the API server's
admissions plug-ins in order to be compatible with
kubernetes/kubernetes#72972.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment