Register CSI CRDs as addon #70193

saad-ali · 2018-10-24T17:09:49Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Register CSI CRDs as addon when the feature gates are enabled via the addon manager.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Short-term workaround for kubernetes/enhancements#615

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

New addon in addon manager that automatically installs CSI CRDs if CSIDriverRegistry or CSINodeInfo feature gates are true.

/sig storage

saad-ali · 2018-10-30T23:54:33Z

CC @davidz627

davidz627 · 2018-10-31T18:30:17Z

Do we have guarantees on when these CRDs will be installed? Does it always happen before nodes are made schedulable? (does not affect current PR but just for my understanding)

saad-ali · 2018-11-01T20:59:08Z

Do we have guarantees on when these CRDs will be installed? Does it always happen before nodes are made schedulable? (does not affect current PR but just for my understanding)

Discussed offline. Addons are installed as part of master setup.

davidz627 · 2018-11-01T22:08:46Z

staging/src/k8s.io/csi-api/pkg/crd/crd.go

its unclear to me why we are removing this, previously the csinodeinfo.yaml was being generated from this definition when you ran the crd_test.go. That was useful for validation.

The code generated version was previously being used as the source of truth, and the YAML was generated from it. Two problems with that was 1) source of truth should be a YAML file not generated code per API machinery folks. 2) the generated yaml contained invalid fields (e.g. status), that resulted not being able to use the YAML in kubectl create -f without disabling validation.

So instead I'm moving the source of truth to YAML in crd/manifests. If any one wants a programmatic version of it, they can load it from the YAML file (see the integration test that does this).

The source of truth should be "go IDL", i.e. types.go style struct definitions. yaml is also OK, although no tools will work on it. The code getting removed here was not an acceptable format for being a source of truth, so it's good that it's getting deleted.

davidz627 · 2018-11-01T22:09:23Z

cluster/addons/storage-crds/OWNERS

reviewers too? (I could probably help with that)

davidz627 · 2018-11-01T22:10:03Z

cluster/addons/storage-crds/README.md

copy does not drift in relation to what other copy?

Reworded to clarify.

saad-ali · 2018-11-02T00:13:05Z

/test pull-kubernetes-e2e-gce

saad-ali · 2018-11-02T16:30:29Z

/test pull-kubernetes-verify
/test pull-kubernetes-integration

saad-ali · 2018-11-02T19:57:06Z

/test pull-kubernetes-e2e-kops-aws

caesarxuchao · 2018-11-02T21:45:23Z

cluster/gce/config-default.sh

Doesn't line 259 guarantee that KUBE_FEATURES_GATES is ALLAlpha=true?

I guest this block shouldn't be nested.

Yes, but if a user explicitly disables this feature, I don't want to force it on even if it is enabled by default for alpha.

caesarxuchao · 2018-11-02T21:54:58Z

staging/src/k8s.io/csi-api/pkg/crd/manifests/csidriver.yaml

Shall csiDrivers participate the spec/status convention? See https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go#L54.

I don't think I understand your question. We have a spec for the object, and will consider adding a status in the future if needed.

AishSundar · 2018-11-05T17:02:38Z

/priority important-soon

saad-ali · 2018-11-05T20:26:04Z

/test pull-kubernetes-e2e-kops-aws

saad-ali · 2018-11-05T21:14:29Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce-100-performance

mikedanese · 2018-11-05T21:26:15Z

staging/src/k8s.io/csi-api/pkg/crd/crd.go

Should these tests live next to the addon manifests?

I want the manifests dir to contain only the manifests and nothing else so that people can grab the entire directory as a source of truth and not get test files and other stuff with it. Would prefer to leave this as is.

mikedanese · 2018-11-05T21:26:50Z

staging/src/k8s.io/csi-api/pkg/crd/crd_test.go

delete this line since we are already cleaning up imports.

mikedanese · 2018-11-05T21:29:36Z

test/integration/auth/node_test.go

Question: is the conversion from yaml to json here actually necessary? I though the universal decoder handled all serialization media types automatically.

Cleaned up.

mikedanese · 2018-11-05T21:29:59Z

/approve

saad-ali · 2018-11-06T01:37:56Z

/test pull-kubernetes-e2e-kops-aws

lavalamp · 2018-11-06T19:09:07Z

I did not review the shell changes exhaustively, other than that lgtm.

/approve

k8s-ci-robot · 2018-11-06T19:09:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, mikedanese, saad-ali

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cluster/OWNERS~~ [lavalamp,mikedanese]
~~staging/src/k8s.io/csi-api/Godeps/OWNERS~~ [lavalamp,saad-ali]
~~staging/src/k8s.io/csi-api/pkg/crd/OWNERS~~ [lavalamp]
~~test/OWNERS~~ [lavalamp,mikedanese,saad-ali]

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

davidz627 · 2018-11-06T21:47:42Z

/retest

saad-ali · 2018-11-06T22:46:56Z

/test pull-kubernetes-e2e-gce

msau42 · 2018-11-07T01:19:20Z

/lgtm

fejta-bot · 2018-11-07T05:21:32Z

/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 comment for consistent failures.

k8s-ci-robot requested review from jingax10 and nikhiljindal October 24, 2018 17:11

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Oct 24, 2018

saad-ali force-pushed the csiCRDAddon branch from de380b5 to 7a0831a Compare October 24, 2018 17:55

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 25, 2018

saad-ali force-pushed the csiCRDAddon branch from 6ff08ce to d73b8e5 Compare October 25, 2018 19:02

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 29, 2018

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 1, 2018

saad-ali force-pushed the csiCRDAddon branch from 2e4c0ed to 198d79a Compare November 1, 2018 21:58

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

davidz627 reviewed Nov 1, 2018

View reviewed changes

saad-ali force-pushed the csiCRDAddon branch from 198d79a to f5f0616 Compare November 1, 2018 22:24

saad-ali force-pushed the csiCRDAddon branch 3 times, most recently from 99b988b to 5c97b26 Compare November 2, 2018 01:49

saad-ali changed the title ~~WIP: Register CSI CRDs as addon~~ Register CSI CRDs as addon Nov 2, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 2, 2018

saad-ali force-pushed the csiCRDAddon branch from 5c97b26 to 7603dfd Compare November 2, 2018 16:21

caesarxuchao reviewed Nov 2, 2018

View reviewed changes

msau42 mentioned this pull request Nov 5, 2018

volume A/D controller: move CRDs to yaml #68524

Closed

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 5, 2018

saad-ali force-pushed the csiCRDAddon branch from 7603dfd to b46dd6d Compare November 5, 2018 19:34

mikedanese reviewed Nov 5, 2018

View reviewed changes

Register CSI CRDs as addon

a679486

saad-ali force-pushed the csiCRDAddon branch from b46dd6d to a679486 Compare November 6, 2018 18:58

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2018

k8s-ci-robot assigned msau42 Nov 7, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2018

k8s-ci-robot merged commit 7fe5916 into kubernetes:master Nov 7, 2018

msau42 mentioned this pull request Nov 7, 2018

CSI alpha tests are failing #70760

Closed

cofyc mentioned this pull request Nov 8, 2018

Update urls in docs kubernetes-csi/docs#73

Closed

pohly mentioned this pull request Nov 8, 2018

local-up-cluster.sh: install CSI CRDs #70791

Closed

Register CSI CRDs as addon #70193

Register CSI CRDs as addon #70193

Uh oh!

Conversation

saad-ali commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saad-ali commented Oct 30, 2018

Uh oh!

davidz627 commented Oct 31, 2018

Uh oh!

saad-ali commented Nov 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saad-ali commented Nov 2, 2018

Uh oh!

saad-ali commented Nov 2, 2018

Uh oh!

saad-ali commented Nov 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AishSundar commented Nov 5, 2018

Uh oh!

saad-ali commented Nov 5, 2018

Uh oh!

saad-ali commented Nov 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikedanese commented Nov 5, 2018

Uh oh!

saad-ali commented Nov 6, 2018

Uh oh!

lavalamp commented Nov 6, 2018

Uh oh!

k8s-ci-robot commented Nov 6, 2018

Uh oh!

davidz627 commented Nov 6, 2018

Uh oh!

saad-ali commented Nov 6, 2018

Uh oh!

msau42 commented Nov 7, 2018

Uh oh!

fejta-bot commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

saad-ali commented Oct 24, 2018 •

edited

Loading