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.

Sign up for GitHub

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

Jump to bottom

Upgraded CSI spec to v1.2.0 #169

Merged
merged 1 commit into from Jun 1, 2020
Merged

Upgraded CSI spec to v1.2.0 #169

merged 1 commit into from Jun 1, 2020

Conversation

jqmichael
Copy link
Contributor

What is this PR about? / Why do we need it?
Upgraded CSI Spec to the latest. None of the change applies to EFS code base.

Breaking changes/Deprecations:

None (CSI 1.x releases are backwards compatible).
Additions:

spec (PR #372): Add optional secrets field to ListSnapshotsRequest.
spec (PR #374): Add optional (but recommended) VolumeStatus.published_node_ids field to ListVolumesResponse (to enable COs to detect and reconcile when a volume is unpublished out of band).
spec (PR #390): Add optional (but recommended) staging_path to NodeGetVolumeStatsRequest and NodeExpandVolumeRequest.
spec (PR #381): Add optional volume_capability to ControllerExpandVolumeRequest and NodeExpandVolumeRequest (to enable SPs to differentiate between block and file).
Clarifications/Corrections/Fixes:

spec (PR #347): Clarify DeleteVolume behavior with snapshots.
spec (PR #375): Clarify SP is allowed to return OK for ControllerUnpublishResponse if node or volume not found.
spec (PR #383): Relax recovery behavior for NOT_FOUND error code for ControllerUnpublishVolume from MUST to SHOULD.
Dependencies:

lib/go (PR #378) Update protoc to v3.9.1, protoc-gen-go to v1.3.2.

https://github.com/container-storage-interface/spec/releases

What testing is done?
Tested manually.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 20, 2020
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 20, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @jqmichael. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 20, 2020
leakingtapan
leakingtapan requested changes May 25, 2020
View reviewed changes
Copy link
Contributor

@leakingtapan leakingtapan left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any new APIs from 1.2.0 spec that recommend to be implemented? I don't see there is any new functionalities implemented along with the new spec.

And please update the README also

deploy/kubernetes/base/node.yaml Outdated Show resolved Hide resolved
deploy/kubernetes/base/node.yaml Show resolved Hide resolved
deploy/kubernetes/base/node.yaml Show resolved Hide resolved
deploy/kubernetes/base/node.yaml Show resolved Hide resolved
@leakingtapan
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 25, 2020
@jqmichael
Copy link
Contributor Author

Is there any new APIs from 1.2.0 spec that recommend to be implemented? I don't see there is any new functionalities implemented along with the new spec.

And please update the README also

There's no new API introduced. All changes from v1.2.0 are listed in the PR description. There's none that applies to EKS CSI Driver today.

Will update the README.

@jqmichael
Copy link
Contributor Author

/retest

@k8s-ci-robot 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 May 27, 2020
jqmichael
jqmichael commented May 27, 2020
View reviewed changes
go.mod
github.com/kubernetes-csi/csi-test v2.2.0+incompatible
github.com/onsi/ginkgo v1.12.2
github.com/onsi/gomega v1.10.1
google.golang.org/grpc v1.26.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to use an version prior to v1.27.0 until we pick up etcd-io/etcd#11564 (had trouble picking that commit up with go mod)

@jqmichael
Copy link
Contributor Author

/retest

@jqmichael
Copy link
Contributor Author

/test pull-aws-efs-csi-driver-unit

@jqmichael
Copy link
Contributor Author

@leakingtapan Please take a look. Thanks

@jqmichael jqmichael force-pushed the CSISpec1-2 branch 2 times, most recently from c85e37e to 9a5b760 Compare May 29, 2020 23:15
@jqmichael
Copy link
Contributor Author

/retest

1 similar comment
@jqmichael
Copy link
Contributor Author

/retest

leakingtapan
leakingtapan reviewed May 31, 2020
View reviewed changes
docs/README.md Outdated
### CSI Specification Compatibility Matrix
| AWS EFS CSI Driver \ CSI Spec Version | v0.3.0| v1.1.0 | v1.2.0 |
|----------------------------------------|-------|--------|--------|
| master branch | no | yes | yes |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v1.1.0 should be no for master branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Fixed

@wongma7
Copy link
Contributor

wongma7 commented Jun 1, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2020
@wongma7
Copy link
Contributor

wongma7 commented Jun 1, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jqmichael, wongma7

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit ac8fd12 into kubernetes-sigs:master Jun 1, 2020
@leakingtapan leakingtapan mentioned this pull request Jun 9, 2020
Closed
@jqmichael jqmichael mentioned this pull request Sep 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leakingtapan leakingtapan leakingtapan requested changes

@d-nishi d-nishi Awaiting requested review from d-nishi

@wongma7 wongma7 Awaiting requested review from wongma7

Assignees

@wongma7 wongma7

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jqmichael @k8s-ci-robot @leakingtapan @wongma7