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

CSI: skip attach for non-attachable drivers #67955

Merged
merged 6 commits into from
Sep 5, 2018
Merged

CSI: skip attach for non-attachable drivers #67955

merged 6 commits into from
Sep 5, 2018

Conversation

jsafrane
Copy link
Member

@jsafrane jsafrane commented Aug 28, 2018 •
edited
Loading

What this PR does / why we need it:
This is implementation of kubernetes/community#2523. CSI volumes that don't need attach/detach now don't need external attacher running.

WIP:

/sig storage

cc: @saad-ali @vladimirvivien @verult @msau42 @gnufied @davidz627

Release note:

CSI volume plugin does not need external attacher for non-attachable CSI volumes.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2018
@jsafrane jsafrane changed the title Csi skip attach saad WIP: CSI: skip attach for non-attachable drivers Aug 28, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 28, 2018
@jsafrane jsafrane force-pushed the csi-skip-attach-saad branch 2 times, most recently from 17b5578 to 66dc7e8 Compare August 28, 2018 15:51
@gnufied
Copy link
Member

gnufied commented Aug 28, 2018

/assign

@jsafrane jsafrane force-pushed the csi-skip-attach-saad branch from 66dc7e8 to f7f414f Compare August 29, 2018 11:27
@jsafrane
Copy link
Member Author

Added e2e test

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2018
@saad-ali
Copy link
Member

saad-ali commented Sep 1, 2018

@gnufied @jsafrane @childsb

#67803 has merged. Please rebase.

@saad-ali saad-ali changed the title WIP: CSI: skip attach for non-attachable drivers CSI: skip attach for non-attachable drivers Sep 4, 2018
@k8s-ci-robot 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 Sep 4, 2018
@gnufied
Copy link
Member

gnufied commented Sep 4, 2018

This has same conflicts as #67945 PR and likely it will conflict again when #67945 merges first.

@gnufied gnufied force-pushed the csi-skip-attach-saad branch from f7f414f to 27d9913 Compare September 4, 2018 23:31
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 4, 2018
@gnufied gnufied force-pushed the csi-skip-attach-saad branch from 27d9913 to fc2ddd5 Compare September 4, 2018 23:40
@gnufied
Copy link
Member

gnufied commented Sep 4, 2018

@saad-ali rebased and updated the PR. PTAL.

@saad-ali
Copy link
Member

saad-ali commented Sep 5, 2018

/lgtm
/approve

/assign @liggitt
for the small auth changes

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

liggitt commented Sep 5, 2018

/approve
for policy change

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2018
@saad-ali
Copy link
Member

saad-ali commented Sep 5, 2018

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 5, 2018
@jsafrane @gnufied
Add feature for skipping attachment of non-attachable CSI volumes
21adce3
@jsafrane @gnufied
Add new RBAC rules for CSIDriver
4e7eca7 
A/D controller and nodes need to watch CSIDrivers to know if they should send pod information
in NodePublish.
@jsafrane @gnufied
Add CSIDriver lister to CSI plugin
7c1311b 
So we don't instantiate a new lister in every SetUpAt() call.
@jsafrane @gnufied
Skip attach for non-attachable CSI volumes
c6c74d6
@jsafrane @gnufied
Add unit tests for skipping attach
f474b54
@gnufied gnufied force-pushed the csi-skip-attach-saad branch from 96f483a to d34a5bf Compare September 5, 2018 14:15
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@childsb
Copy link
Contributor

childsb commented Sep 5, 2018

re-adding
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@gnufied gnufied force-pushed the csi-skip-attach-saad branch from d34a5bf to f1cef9b Compare September 5, 2018 14:36
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 5, 2018 •
edited
Loading

@jsafrane: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized f7f414fde79b1938809c31613805b182c072f826 link /test pull-kubernetes-local-e2e-containerized

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.

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.

@gnufied
Copy link
Member

gnufied commented Sep 5, 2018

kops failures looks unrelated.

/test pull-kubernetes-e2e-kops-aws

@childsb
Copy link
Contributor

childsb commented Sep 5, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: childsb, jsafrane, liggitt, saad-ali

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

@childsb childsb added this to the v1.12 milestone Sep 5, 2018
@childsb
Copy link
Contributor

childsb commented Sep 5, 2018

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 5, 2018
@childsb
Copy link
Contributor

childsb commented Sep 5, 2018

/milestone v1.12

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 68161, 68023, 67909, 67955, 67731). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit 19c2538 into kubernetes:master Sep 5, 2018
@krzysied krzysied mentioned this pull request Sep 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendandburns brendandburns Awaiting requested review from brendandburns

@enisoc enisoc Awaiting requested review from enisoc

Assignees

@gnufied gnufied

@childsb childsb

@liggitt liggitt

@saad-ali saad-ali

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.
7 participants
@jsafrane @gnufied @saad-ali @liggitt @childsb @k8s-ci-robot @k8s-github-robot