update volumeAttachment to v1 #200

cwdsuzhou · 2019-12-24T12:01:11Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
update volumeAttachment to v1

Which issue(s) this PR fixes:

Fixes #196

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Update volumeAttachment to v1

Action required: RBAC policy was updated to allow the external-attacher to patch VolumeAttachment.Status

I am not sure if we need update CSINode to V1

cwdsuzhou · 2019-12-24T12:02:02Z

/assign @jsafrane @msau42

cwdsuzhou · 2020-01-02T14:16:56Z

seem we should update the api version of k8s integration test first.

msau42 · 2020-01-02T17:10:11Z

@cwdsuzhou can you provide more details on why the test is failing? The K8s API server can support both v1beta and v1 objects so the consumers should be able to use either version successfully.

cwdsuzhou · 2020-01-03T01:56:08Z

/retest

cwdsuzhou · 2020-01-03T03:44:30Z

Jan  3 02:17:47.615: FAIL: Unexpected error:
    <*errors.errorString | 0xc00239f160>: {
        s: "expected pod \"pod-subpath-test-dynamicpv-dzfs\" success: Gave up after waiting 5m0s for pod \"pod-subpath-test-dynamicpv-dzfs\" to be \"success or failure\"",
    }
    expected pod "pod-subpath-test-dynamicpv-dzfs" success: Gave up after waiting 5m0s for pod "pod-subpath-test-dynamicpv-dzfs" to be "success or failure"
occurred

Full Stack Trace
k8s.io/kubernetes/test/e2e/framework.(*Framework).testContainerOutputMatcher(0xc00209a8c0, 0x4a944e9, 0x7, 0xc0016a8c00, 0x0, 0xc001b8b1b0, 0x1, 0x1, 0x4c7f108)
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/util.go:829 +0x1ee
k8s.io/kubernetes/test/e2e/framework.(*Framework).TestContainerOutput(...)
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:543
k8s.io/kubernetes/test/e2e/storage/testsuites.testReadFile(0xc00209a8c0, 0xc0016c99c0, 0x16, 0xc0016a8c00, 0x0)
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go:719 +0x1f5
k8s.io/kubernetes/test/e2e/storage/testsuites.(*subPathTestSuite).DefineTests.func18()
	/home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go:407 +0x23a
k8s.io/kubernetes/test/e2e.RunE2ETests(0xc002652d00)
	_output/local/go/src/k8s.io/kubernetes/test/e2e/e2e.go:110 +0x30a
k8s.io/kubernetes/test/e2e.TestE2E(0xc002652d00)
	_output/local/go/src/k8s.io/kubernetes/test/e2e/e2e_test.go:112 +0x2b
testing.tRunner(0xc002652d00, 0x4c7aad0)
	/usr/local/go/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run

logs are about timeout

msau42 · 2020-01-03T18:31:17Z

Looks like attacher sidecar successfully updated VA object:

I0103 09:13:50.426917       1 controller.go:198] Started VA processing "csi-8db4233ad3c6a8b2e98e71892654c43f63aeb919003873407101f72753f70a96"
I0103 09:13:50.426951       1 trivial_handler.go:53] Trivial sync[csi-8db4233ad3c6a8b2e98e71892654c43f63aeb919003873407101f72753f70a96] started
I0103 09:13:50.426956       1 util.go:36] Marking as attached "csi-8db4233ad3c6a8b2e98e71892654c43f63aeb919003873407101f72753f70a96"
I0103 09:13:50.430162       1 util.go:49] Marked as attached "csi-8db4233ad3c6a8b2e98e71892654c43f63aeb919003873407101f72753f70a96"
I0103 09:13:50.430187       1 trivial_handler.go:61] Marked VolumeAttachment csi-8db4233ad3c6a8b2e98e71892654c43f63aeb919003873407101f72753f70a96 as attached

However, controller manager does not seem to see the update:

I0103 09:13:50.423037       1 reconciler.go:288] attacherDetacher.AttachVolume started for volume "pvc-f6bc1fa5-cbf8-495b-a9e2-78e573614ed0" (UniqueName: "kubernetes.io/csi/hostpath.csi.k8s.io^59c001b5-2e09-11ea-aab0-525e4f5fc867") from node "csi-prow-worker" 
E0103 09:15:50.427115       1 csi_attacher.go:205] kubernetes.io/csi: attacher.WaitForAttach timeout after 2m0s [volume=59c001b5-2e09-11ea-aab0-525e4f5fc867; attachment.ID=csi-8db4233ad3c6a8b2e98e71892654c43f63aeb919003873407101f72753f70a96]

cwdsuzhou · 2020-01-04T03:18:16Z

@msau42 when it updates to v1, I find patch actually failed, but error does not return.

I will take a closer look at it! Maybe it is an issue of apiserver.

liggitt · 2020-01-04T23:46:55Z

Patching fields underneath the status field must be done via the status subresource in v1

liggitt · 2020-01-04T23:48:05Z

pkg/controller/csi_handler.go

@@ -724,7 +725,7 @@ func (h *csiHandler) patchVA(va, clone *storage.VolumeAttachment) (*storage.Volu
 		return va, err
 	}

-	newVa, err := h.client.StorageV1beta1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)
+	newVa, err := h.client.StorageV1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)


if this is changing status fields, it must submit the patch to the status subresource

liggitt · 2020-01-04T23:48:27Z

pkg/controller/csi_handler.go

@@ -316,7 +317,7 @@ func (h *csiHandler) prepareVANodeID(va *storage.VolumeAttachment, nodeID string
 }

 func (h *csiHandler) saveVA(va *storage.VolumeAttachment, patch []byte) (*storage.VolumeAttachment, error) {
-	newVA, err := h.client.StorageV1beta1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)
+	newVA, err := h.client.StorageV1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)


same comment as https://github.com/kubernetes-csi/external-attacher/pull/200/files#r363060972

liggitt · 2020-01-04T23:49:27Z

pkg/controller/util.go

@@ -83,7 +84,7 @@ func markAsDetached(client kubernetes.Interface, va *storage.VolumeAttachment) (
 	if err != nil {
 		return va, err
 	}
-	newVA, err := client.StorageV1beta1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)
+	newVA, err := client.StorageV1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)


same comment as https://github.com/kubernetes-csi/external-attacher/pull/200/files#r363060972

liggitt · 2020-01-04T23:49:38Z

pkg/controller/util.go

@@ -41,7 +42,7 @@ func markAsAttached(client kubernetes.Interface, va *storage.VolumeAttachment, m
 	if err != nil {
 		return va, err
 	}
-	newVA, err := client.StorageV1beta1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)
+	newVA, err := client.StorageV1().VolumeAttachments().Patch(va.Name, types.MergePatchType, patch)


same comment as https://github.com/kubernetes-csi/external-attacher/pull/200/files#r363060972

msau42 · 2020-01-07T20:41:41Z

deploy/kubernetes/rbac.yaml

@@ -34,6 +34,9 @@ rules:
  - apiGroups: ["storage.k8s.io"]
    resources: ["volumeattachments"]
    verbs: ["get", "list", "watch", "update", "patch"]
+  - apiGroups: ["storage.k8s.io"]


Hm new rbac is going to require us to bump major versions again. @jsafrane are we ready for this?

We need change e2e test of k8s ?

I don't remember any other deprecation that would require version bump. It would be nice to include also v1 of CSINode and VolumeAttachment and thus bumping of min. k8s version.

I created #212 not to forget to bump major version and go module.

I think we can do 3.0 in this cycle.

@jsafrane are we ready to do 3.0 now? This is one of the items preventing us from adding CSI tests to conformance, as conformance cluster only enables v1 objects.

cwdsuzhou · 2020-01-18T11:45:07Z

/hold

jsafrane · 2020-04-16T11:24:01Z

@cwdsuzhou, I think it's time for 3.0 release (together with Kubernetes 1.19). Can you please rebase the PR?

cwdsuzhou · 2020-04-17T12:11:04Z

@cwdsuzhou, I think it's time for 3.0 release (together with Kubernetes 1.19). Can you please rebase the PR?

sure, thanks

cwdsuzhou · 2020-04-17T13:17:06Z

/hold cc

cwdsuzhou · 2020-04-17T13:17:49Z

/unhold

jsafrane · 2020-04-17T14:15:44Z

lgtm-ish.
Please edit the release note in the first comment - in addition to what is there now, it should also say "Action required: RBAC policy was updated to allow the external-attacher to patch VolumeAttachment.Status" (or something like that).

cwdsuzhou · 2020-04-17T14:39:25Z

lgtm-ish.
Please edit the release note in the first comment - in addition to what is there now, it should also say "Action required: RBAC policy was updated to allow the external-attacher to patch VolumeAttachment.Status" (or something like that).

Updated

jsafrane · 2020-04-17T14:58:46Z

pkg/controller/util.go

@@ -116,7 +119,7 @@ func GetFinalizerName(driver string) string {
 }

 // GetNodeIDFromCSINode returns nodeID from CSIDriverInfoSpec
-func GetNodeIDFromCSINode(driver string, csiNode *storage.CSINode) (string, bool) {
+func GetNodeIDFromCSINode(driver string, csiNode *storagev1beat1.CSINode) (string, bool) {


typo: storagev1beat1

jsafrane · 2020-04-17T15:45:30Z

/lgtm
/approve
Thanks a lot for a quick rebase!

k8s-ci-robot · 2020-04-17T15:45:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cwdsuzhou, jsafrane

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:

~~OWNERS~~ [jsafrane]

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

d24254f6 Merge pull request kubernetes-csi#202 from xing-yang/kind_0.14.0 0faa3fc7 Update to Kind v0.14.0 images ef4e1b2b Merge pull request kubernetes-csi#201 from xing-yang/add_1.24_image 4ddce251 Add 1.24 Kind image 7fe51491 Merge pull request kubernetes-csi#200 from pohly/bump-kubernetes-version 70915a8e prow.sh: update snapshotter version 31a3f38b Merge pull request kubernetes-csi#199 from pohly/bump-kubernetes-version 7577454a prow.sh: bump Kubernetes to v1.22.0 git-subtree-dir: release-tools git-subtree-split: d24254f6aa780bb6ba36a946973ee01df5633f6b

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 24, 2019

k8s-ci-robot requested review from lpabon and msau42 December 24, 2019 12:01

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 24, 2019

k8s-ci-robot assigned jsafrane and msau42 Dec 24, 2019

cwdsuzhou force-pushed the Dec/update_va2v1 branch 2 times, most recently from ad86ff4 to ec029c5 Compare January 3, 2020 08:37

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 3, 2020

cwdsuzhou force-pushed the Dec/update_va2v1 branch 2 times, most recently from 67f481e to e457386 Compare January 3, 2020 08:54

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2020

cwdsuzhou mentioned this pull request Jan 4, 2020

Patch status of volumeAttachment(StorageV1) does not work kubernetes/kubernetes#86835

Closed

liggitt reviewed Jan 4, 2020

View reviewed changes

cwdsuzhou force-pushed the Dec/update_va2v1 branch from e457386 to a8abecd Compare January 5, 2020 03:46

msau42 reviewed Jan 7, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2020

jsafrane mentioned this pull request Jan 31, 2020

Bump version to 3.0.0 #212

Closed

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

cwdsuzhou force-pushed the Dec/update_va2v1 branch from 4dd4e4f to 2797c50 Compare April 17, 2020 13:14

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 17, 2020

jsafrane reviewed Apr 17, 2020

View reviewed changes

Update volumeAttachment to v1

9956286

cwdsuzhou force-pushed the Dec/update_va2v1 branch from 2797c50 to 9956286 Compare April 17, 2020 15:04

cwdsuzhou requested a review from jsafrane April 17, 2020 15:05

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2020

k8s-ci-robot merged commit 3d81eb3 into kubernetes-csi:master Apr 17, 2020

msau42 mentioned this pull request Apr 20, 2020

Failed to remove finalizer on VolumeAttachment #225

Closed

xing-yang mentioned this pull request Aug 23, 2022

Update release tools #370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update volumeAttachment to v1 #200

update volumeAttachment to v1 #200

cwdsuzhou commented Dec 24, 2019 •

edited

Loading

cwdsuzhou commented Dec 24, 2019

cwdsuzhou commented Jan 2, 2020

msau42 commented Jan 2, 2020

cwdsuzhou commented Jan 3, 2020

cwdsuzhou commented Jan 3, 2020

msau42 commented Jan 3, 2020

cwdsuzhou commented Jan 4, 2020

liggitt commented Jan 4, 2020

liggitt Jan 4, 2020

liggitt Jan 4, 2020

liggitt Jan 4, 2020

liggitt Jan 4, 2020

msau42 Jan 7, 2020

cwdsuzhou Jan 13, 2020

jsafrane Jan 13, 2020

jsafrane Jan 31, 2020

cwdsuzhou Feb 7, 2020

msau42 Apr 10, 2020

cwdsuzhou commented Jan 18, 2020

jsafrane commented Apr 16, 2020

cwdsuzhou commented Apr 17, 2020

cwdsuzhou commented Apr 17, 2020

cwdsuzhou commented Apr 17, 2020

jsafrane commented Apr 17, 2020 •

edited

Loading

cwdsuzhou commented Apr 17, 2020

jsafrane Apr 17, 2020

cwdsuzhou Apr 17, 2020

jsafrane commented Apr 17, 2020

k8s-ci-robot commented Apr 17, 2020

update volumeAttachment to v1 #200

update volumeAttachment to v1 #200

Conversation

cwdsuzhou commented Dec 24, 2019 • edited Loading

cwdsuzhou commented Dec 24, 2019

cwdsuzhou commented Jan 2, 2020

msau42 commented Jan 2, 2020

cwdsuzhou commented Jan 3, 2020

cwdsuzhou commented Jan 3, 2020

msau42 commented Jan 3, 2020

cwdsuzhou commented Jan 4, 2020

liggitt commented Jan 4, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwdsuzhou commented Jan 18, 2020

jsafrane commented Apr 16, 2020

cwdsuzhou commented Apr 17, 2020

cwdsuzhou commented Apr 17, 2020

cwdsuzhou commented Apr 17, 2020

jsafrane commented Apr 17, 2020 • edited Loading

cwdsuzhou commented Apr 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Apr 17, 2020

k8s-ci-robot commented Apr 17, 2020

cwdsuzhou commented Dec 24, 2019 •

edited

Loading

jsafrane commented Apr 17, 2020 •

edited

Loading