Add V1beta1 VolumeAttachment API #58462

NickrenREN · 2018-01-18T14:58:12Z

What this PR does / why we need it:
Add V1beta1 VolumeAttachment API, co-existing with Alpha API object

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #58461

Special notes for your reviewer:

Release note:

Add V1beta1 VolumeAttachment API, co-existing with Alpha API object

NickrenREN · 2018-01-23T11:00:45Z

/assign @saad-ali @jsafrane @caesarxuchao

NickrenREN · 2018-01-23T11:01:07Z

/unassign @gnufied @yifan-gu

NickrenREN · 2018-01-23T11:04:11Z

Finally..... All checks turn green.
Many thanks for @jsafrane and @caesarxuchao 's help

NickrenREN · 2018-01-23T12:54:43Z

/retest

saad-ali

Largely LGTM. A few comments.

saad-ali · 2018-01-24T02:07:20Z

pkg/features/kube_features.go

@@ -255,7 +255,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
 	ServiceNodeExclusion:                        {Default: false, PreRelease: utilfeature.Alpha},
 	MountContainers:                             {Default: false, PreRelease: utilfeature.Alpha},
 	VolumeScheduling:                            {Default: false, PreRelease: utilfeature.Alpha},
-	CSIPersistentVolume:                         {Default: false, PreRelease: utilfeature.Alpha},
+	CSIPersistentVolume:                         {Default: false, PreRelease: utilfeature.Beta},


Let's turn it on by default, now that it is alpha.

Right, beta not alpha, brain fart.

saad-ali · 2018-01-24T02:14:26Z

pkg/kubeapiserver/authorizer/config.go

@@ -82,7 +82,7 @@ func (config AuthorizationConfig) New() (authorizer.Authorizer, authorizer.RuleR
 				graph,
 				config.InformerFactory.Core().InternalVersion().Pods(),
 				config.InformerFactory.Core().InternalVersion().PersistentVolumes(),
-				config.VersionedInformerFactory.Storage().V1alpha1().VolumeAttachments(),
+				config.VersionedInformerFactory.Storage().V1beta1().VolumeAttachments(),


NodeAuthorizer should handle BOTH V1alpha1().VolumeAttachments and V1beta1().VolumeAttachments not just one or the other.

Since we can get both versions VolumeAttachment objects by V1beta1 or V1alpha1 API, so any API version here will do ? Not sure. cc @liggitt

NodeAuthorizer should handle BOTH V1alpha1().VolumeAttachments and V1beta1().VolumeAttachments not just one or the other.

No, it will only watch one to populate the authorizer graph (it uses the info to authorize requests for either version). The authorizer runs in process in the apiserver, so it is never version skewed. I'd expect it to watch v1beta1

Thanks for the clarification, disregard this comment @NickrenREN.

saad-ali · 2018-01-24T02:17:57Z

cmd/kube-apiserver/app/server.go

@@ -574,7 +574,7 @@ func BuildStorageFactory(s *options.ServerRunOptions) (*serverstorage.DefaultSto
 		// FIXME (soltysh): this GroupVersionResource override should be configurable
 		[]schema.GroupVersionResource{
 			batch.Resource("cronjobs").WithVersion("v1beta1"),
-			storage.Resource("volumeattachments").WithVersion("v1alpha1"),
+			storage.Resource("volumeattachments").WithVersion("v1beta1"),


What happens if we leave this v1alpha1? If we add new things to v1alpha1 but not to the v1beta1 will this handle it correctly?

There could only be one storage version. You have to keep the v1alpha1 and v1beta1 roundtrippable. It's an api requirement in the api conventions doc.

If you want to test new things in alpha, you can create a new struct and specify here to store it as v1alpha1.

If you store in v1alpha1, you have the option to downgrade and have VolumeAttachments keep working in alpha form, but you have alpha data in etcd, which means you cannot drop the alpha types until storage migration is solved.

If you store in beta, you are better positioned to drop alpha types sooner (since the only people who would have alpha data were people who upgraded from 1.9 with the alpha flag enabled)

I'd probably expect to store in v1beta1

Ok. I'm slightly concerned that doing so will require us to add new features to both v1beta1 but v1alpha1, but, we'll leave storage as v1beta1.

liggitt · 2018-01-24T04:31:34Z

test/integration/auth/node_test.go

@@ -107,7 +107,7 @@ func TestNodeAuthorizer(t *testing.T) {
 	masterConfig.GenericConfig.AdmissionControl = nodeRestrictionAdmission

 	// enable testing volume attachments
-	masterConfig.ExtraConfig.APIResourceConfigSource.(*serverstorage.ResourceConfig).EnableVersions(storagev1alpha1.SchemeGroupVersion)
+	masterConfig.ExtraConfig.APIResourceConfigSource.(*serverstorage.ResourceConfig).EnableVersions(storagev1beta1.SchemeGroupVersion)


Should be able to remove this line if the beta version is enabled by default

liggitt · 2018-01-24T04:32:46Z

test/integration/etcd/etcd_storage_path_test.go

@@ -294,13 +294,21 @@ var etcdStorageData = map[schema.GroupVersionResource]struct {
 	},
 	// --

-	// k8s.io/kubernetes/pkg/apis/storage/v1alpha1


For minimal diff, keep alpha first and add beta below?

saad-ali · 2018-01-31T06:52:17Z

/lgtm
/approve

/assign @liggitt for API approval

k8s-ci-robot · 2018-01-31T06:52:18Z

@saad-ali: GitHub didn't allow me to assign the following users: API, approval, for.

Note that only kubernetes members and repo collaborators can be assigned.

In response to this:

/lgtm
/approve

/assign @liggitt for API approval

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.

NickrenREN · 2018-01-31T06:55:35Z

/assign @thockin @liggitt

NickrenREN · 2018-01-31T07:27:38Z

just rebase. @saad-ali

saad-ali · 2018-01-31T17:46:15Z

/lgtm

k8s-ci-robot · 2018-02-02T00:46:41Z

@NickrenREN: GitHub didn't allow me to request PR reviews from the following users: brendanburns.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @brendanburns

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.

NickrenREN · 2018-02-02T00:47:26Z

/cc @brendandburns

brendandburns · 2018-02-02T00:57:41Z

/approve

k8s-ci-robot · 2018-02-02T00:57:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, NickrenREN, saad-ali

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

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [brendandburns]
~~cmd/kube-apiserver/OWNERS~~ [brendandburns]
~~docs/api-reference/OWNERS~~ [brendandburns]
~~pkg/OWNERS~~ [brendandburns]
~~plugin/pkg/auth/OWNERS~~ [brendandburns]
~~staging/src/k8s.io/api/OWNERS~~ [brendandburns]
~~staging/src/k8s.io/client-go/OWNERS~~ [brendandburns]
~~test/OWNERS~~ [brendandburns,saad-ali]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-02T02:07:42Z

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

k8s-github-robot · 2018-02-02T02:50:24Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned gnufied and yifan-gu Jan 18, 2018

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

NickrenREN force-pushed the va-to-beta branch from d66f001 to 3b82e22 Compare January 19, 2018 05:38

k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 19, 2018

NickrenREN force-pushed the va-to-beta branch 4 times, most recently from 989985a to 38a214b Compare January 23, 2018 08:12

NickrenREN changed the title ~~WIP: Move VolumeAttachment API to Beta~~ Move VolumeAttachment API to Beta Jan 23, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2018

k8s-ci-robot assigned caesarxuchao, jsafrane and saad-ali Jan 23, 2018

k8s-ci-robot unassigned gnufied and yifan-gu Jan 23, 2018

NickrenREN force-pushed the va-to-beta branch from 38a214b to fc88fe5 Compare January 23, 2018 11:31

saad-ali reviewed Jan 24, 2018

View reviewed changes

NickrenREN force-pushed the va-to-beta branch from fc88fe5 to 644a9db Compare January 24, 2018 04:26

liggitt reviewed Jan 24, 2018

View reviewed changes

NickrenREN force-pushed the va-to-beta branch from 644a9db to f45ca86 Compare January 24, 2018 04:55

k8s-ci-robot assigned liggitt Jan 31, 2018

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

k8s-ci-robot assigned thockin Jan 31, 2018

NickrenREN force-pushed the va-to-beta branch from 76032ee to 9f44ed4 Compare January 31, 2018 07:16

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2018

Add Beta VolumeAttachment API

d45a418

NickrenREN force-pushed the va-to-beta branch from 9f44ed4 to 84baa6a Compare January 31, 2018 10:14

NickrenREN added 3 commits January 31, 2018 18:45

update all

8915e27

Use v1beta1 VolumeAttachment

7b9d2c0

Change feature gate PreRelease to Beta

d7ffadd

NickrenREN force-pushed the va-to-beta branch from 84baa6a to d7ffadd Compare January 31, 2018 10:46

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

k8s-ci-robot requested a review from brendandburns February 2, 2018 00:47

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

k8s-github-robot merged commit 8363129 into kubernetes:master Feb 2, 2018

NickrenREN deleted the va-to-beta branch February 2, 2018 04:59

vladimirvivien mentioned this pull request Feb 10, 2018

CSI - Update CSIPersistentVolume feature gate spec to Beta #59675

Closed

kokhang mentioned this pull request Feb 28, 2018

Update Rook roadmap rook/rook#1537

Merged

2 tasks

yue9944882 mentioned this pull request Aug 9, 2018

[Umbrella Issue] Authz & Core admission controller clientset/informer refactor internal->external #66680

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add V1beta1 VolumeAttachment API #58462

Add V1beta1 VolumeAttachment API #58462

NickrenREN commented Jan 18, 2018 •

edited

NickrenREN commented Jan 23, 2018

NickrenREN commented Jan 23, 2018

NickrenREN commented Jan 23, 2018 •

edited

NickrenREN commented Jan 23, 2018

saad-ali left a comment

saad-ali Jan 24, 2018

liggitt Jan 24, 2018

saad-ali Jan 31, 2018

saad-ali Jan 24, 2018

NickrenREN Jan 24, 2018 •

edited

liggitt Jan 24, 2018 •

edited

saad-ali Jan 31, 2018

NickrenREN Jan 31, 2018

saad-ali Jan 24, 2018

caesarxuchao Jan 24, 2018 •

edited

liggitt Jan 24, 2018

liggitt Jan 24, 2018

saad-ali Jan 31, 2018

liggitt Jan 24, 2018

NickrenREN Jan 24, 2018

liggitt Jan 24, 2018

NickrenREN Jan 24, 2018

saad-ali commented Jan 31, 2018

k8s-ci-robot commented Jan 31, 2018

NickrenREN commented Jan 31, 2018

NickrenREN commented Jan 31, 2018

saad-ali commented Jan 31, 2018

k8s-ci-robot commented Feb 2, 2018

NickrenREN commented Feb 2, 2018

brendandburns commented Feb 2, 2018

k8s-ci-robot commented Feb 2, 2018

k8s-github-robot commented Feb 2, 2018

k8s-github-robot commented Feb 2, 2018

Add V1beta1 VolumeAttachment API #58462

Add V1beta1 VolumeAttachment API #58462

Conversation

NickrenREN commented Jan 18, 2018 • edited

NickrenREN commented Jan 23, 2018

NickrenREN commented Jan 23, 2018

NickrenREN commented Jan 23, 2018 • edited

NickrenREN commented Jan 23, 2018

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickrenREN Jan 24, 2018 • edited

Choose a reason for hiding this comment

liggitt Jan 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao Jan 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Jan 31, 2018

k8s-ci-robot commented Jan 31, 2018

NickrenREN commented Jan 31, 2018

NickrenREN commented Jan 31, 2018

saad-ali commented Jan 31, 2018

k8s-ci-robot commented Feb 2, 2018

NickrenREN commented Feb 2, 2018

brendandburns commented Feb 2, 2018

k8s-ci-robot commented Feb 2, 2018

k8s-github-robot commented Feb 2, 2018

k8s-github-robot commented Feb 2, 2018

NickrenREN commented Jan 18, 2018 •

edited

NickrenREN commented Jan 23, 2018 •

edited

NickrenREN Jan 24, 2018 •

edited

liggitt Jan 24, 2018 •

edited

caesarxuchao Jan 24, 2018 •

edited