Skip to content

CSINodeInfo and CSIDriver Controller Changes#74283

Merged
k8s-ci-robot merged 7 commits intokubernetes:masterfrom
xing-yang:csi_crd_controller
Mar 5, 2019
Merged

CSINodeInfo and CSIDriver Controller Changes#74283
k8s-ci-robot merged 7 commits intokubernetes:masterfrom
xing-yang:csi_crd_controller

Conversation

@xing-yang
Copy link
Copy Markdown
Contributor

@xing-yang xing-yang commented Feb 20, 2019 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR includes the controller side changes to move CSINodeInfo and CSIDriver to v1beta1 core storage APIs. It depends on the PR for API changes:
#73883

Which issue(s) this PR fixes:

Fixes #

kubernetes/enhancements#770

kubernetes/enhancements#603

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Transition CSINodeInfo and CSIDriver alpha CRDs to in-tree CSINode and CSIDriver core storage v1beta1 APIs.
ACTION REQUIRED: the alpha CRDs are no longer used and drivers will need to be updated to use the beta APIs.
The support for `_` in the CSI driver name will be dropped as the CSI Spec does not allow that.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 20, 2019
@k8s-ci-robot k8s-ci-robot added area/apiserver area/kubelet kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 20, 2019
@fedebongio
Copy link
Copy Markdown
Contributor

fedebongio commented Feb 21, 2019

/cc @cheftako

This was referenced Feb 25, 2019
Closed
Merged
Closed
@xing-yang xing-yang force-pushed the csi_crd_controller branch 4 times, most recently from ded02f8 to 5c1bffc Compare February 28, 2019 15:55
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2019
@k8s-ci-robot 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/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 4, 2019
msau42
msau42 reviewed Mar 4, 2019
View reviewed changes
pkg/volume/csi/nodeinfomanager/nodeinfomanager.go Outdated
Copy link
Copy Markdown
Member

@msau42 msau42 Mar 4, 2019

Choose a reason for hiding this comment

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

Can you also add a test case for this?

Copy link
Copy Markdown
Contributor Author

@xing-yang xing-yang Mar 4, 2019

Choose a reason for hiding this comment

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

Sure

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2019
@davidz627
Copy link
Copy Markdown
Contributor

davidz627 commented Mar 4, 2019

/hold cancel
/lgtm cancel
until test is added. Xing, please just squash commits as they were before after adding the test as well.

@k8s-ci-robot k8s-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 4, 2019
@xing-yang xing-yang force-pushed the csi_crd_controller branch from fbd14c6 to a8a5190 Compare March 4, 2019 23:32
@msau42
Copy link
Copy Markdown
Member

msau42 commented Mar 4, 2019

Can you also clarify in the release notes, that the it's the CSINodeInfo and CSIDriver alpha CRDs.

Also add "ACTION REQUIRED" at the beginning and mention that the alpha CRDs are no longer used and drivers will need to be updated to use the beta APIs.

@k8s-ci-robot 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 Mar 4, 2019
@xing-yang
Copy link
Copy Markdown
Contributor Author

xing-yang commented Mar 5, 2019

/test pull-kubernetes-integration

@msau42
Copy link
Copy Markdown
Member

msau42 commented Mar 5, 2019

/retest

xing-yang added 3 commits March 4, 2019 16:41
@xing-yang
CSINodeInfo/CSIDriver controller changes
6265f4f 
This is the 2nd PR to move CSINodeInfo/CSIDriver APIs to
v1beta1 core storage APIs. It includes controller side changes.
It depends on the PR with API changes:
kubernetes#73883
@xing-yang
Modify node admission and node authorizer
85867e5
@xing-yang
Enable CSI test suite and fix test failures
d69e0ff
@xing-yang xing-yang force-pushed the csi_crd_controller branch from a8a5190 to d69e0ff Compare March 5, 2019 00:42
@msau42
Copy link
Copy Markdown
Member

msau42 commented Mar 5, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2019
@msau42
Copy link
Copy Markdown
Member

msau42 commented Mar 5, 2019

/test pull-kubernetes-integration

@msau42
Copy link
Copy Markdown
Member

msau42 commented Mar 5, 2019

/test pull-kubernetes-e2e-gce

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Mar 5, 2019 •
edited
Loading

@xing-yang: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-csi-serial 286372d99280c496fcc8f059dd0ff538c6c4e887 link /test pull-kubernetes-e2e-gce-csi-serial

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.

Details

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.

@fejta-bot
Copy link
Copy Markdown

fejta-bot commented Mar 5, 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 6c31101 into kubernetes:master Mar 5, 2019
This was referenced Mar 6, 2019
Closed
Closed
@deleteriousEffect deleteriousEffect mentioned this pull request Mar 26, 2019
Closed
@fatih fatih mentioned this pull request Apr 23, 2019
Merged
3 tasks
@jia-jerry jia-jerry mentioned this pull request Apr 24, 2019
Merged
@liggitt liggitt mentioned this pull request Oct 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thockin thockin thockin left review comments

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@david-mcmahon david-mcmahon Awaiting requested review from david-mcmahon

@cheftako cheftako Awaiting requested review from cheftako

+3 more reviewers

@bertinatto bertinatto bertinatto left review comments

@davidz627 davidz627 davidz627 left review comments

@msau42 msau42 msau42 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@davidz627 davidz627

@msau42 msau42

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@xing-yang @fedebongio @msau42 @thockin @fejta-bot @davidz627 @k8s-ci-robot @bertinatto