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.

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

Block volumes Support: iSCSI plugin update #54752

Merged

Conversation

mtanino
Copy link
Member

@mtanino mtanino commented Oct 28, 2017

What this PR does / why we need it:

Add interface changes to iSCSI volume plugin to enable block volumes support feature.

Which issue this PR fixes:
Based on this proposal (kubernetes/community#805 & kubernetes/community#1265) and this feature issue: kubernetes/enhancements#351

Special notes for your reviewer:

This PR temporarily includes following changes except iSCSI plugin change for reviewing purpose.
These changes will be removed from the PR once they are merged.

There are another PRs related to this functionality.
(#50457) API Change
(#53385) VolumeMode PV-PVC Binding change
(#51494) Container runtime interface change, volumemanager changes, operationexecutor changes
(#55112) Block volume: Command line printer update
Plugins
(#51493) Block volumes Support: FC plugin update
(#54752) Block volumes Support: iSCSI plugin update

Release note:

NONE

@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. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 28, 2017
@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 28, 2017
@mtanino
Copy link
Member Author

mtanino commented Oct 28, 2017

/retest

2 similar comments
@mtanino
Copy link
Member Author

mtanino commented Oct 30, 2017

/retest

@mtanino
Copy link
Member Author

mtanino commented Oct 30, 2017

/retest

@mtanino mtanino changed the title [WIP] Block volumes Support: iSCSI plugin update Block volumes Support: iSCSI plugin update Nov 3, 2017
@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 Nov 3, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2017
@mtanino mtanino force-pushed the pr/BlockVolumesSupport-iscsi branch from b7f419f to 5dd3639 Compare November 8, 2017 15:17
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2017
@mtanino
Copy link
Member Author

mtanino commented Nov 8, 2017

/retest

2 similar comments
@mtanino
Copy link
Member Author

mtanino commented Nov 8, 2017

/retest

@mtanino
Copy link
Member Author

mtanino commented Nov 8, 2017

/retest

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017
@mtanino mtanino force-pushed the pr/BlockVolumesSupport-iscsi branch from 0787946 to 3676f66 Compare November 9, 2017 23:38
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 9, 2017
@rootfs
Copy link
Member

rootfs commented Nov 13, 2017

@mtanino is it ready for review?

@mtanino
Copy link
Member Author

mtanino commented Nov 13, 2017

@rootfs yes, ready to review. thanks.

func (detacher *iscsiDetacher) volumeSpecToUnmounter(mounter mount.Interface) *iscsiDiskUnmounter {
exec := detacher.host.GetExec(iscsiPluginName)
func VolumeSpecToUnmounter(mounter mount.Interface, host volume.VolumeHost) *iscsiDiskUnmounter {
exec := host.GetExec(iscsiPluginName)
Copy link
Member

Choose a reason for hiding this comment

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

this is already done above

Copy link
Member Author

Choose a reason for hiding this comment

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

Which line?

@mtanino mtanino force-pushed the pr/BlockVolumesSupport-iscsi branch from 76ba461 to d715960 Compare December 2, 2017 00:29
@mtanino
Copy link
Member Author

mtanino commented Dec 2, 2017

@rootfs @jsafrane
I think I need feedback for iscsi block volume support. PTAL.

This PR adds

@mtanino
Copy link
Member Author

mtanino commented Dec 2, 2017

/unassign @erictune
/assign @jsafrane

@k8s-ci-robot k8s-ci-robot assigned jsafrane and unassigned erictune Dec 2, 2017
@mtanino mtanino force-pushed the pr/BlockVolumesSupport-iscsi branch 2 times, most recently from d997124 to 35baf9c Compare December 4, 2017 20:41
@mtanino
Copy link
Member Author

mtanino commented Dec 4, 2017

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 4, 2017

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

Test name Commit Details Rerun command
pull-kubernetes-cross 756a957694ade26a7f415d0ec93fe4863ca86650 link /test pull-kubernetes-cross

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.

@mtanino
Copy link
Member Author

mtanino commented Dec 4, 2017

/retest

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017
@mtanino mtanino force-pushed the pr/BlockVolumesSupport-iscsi branch from 35baf9c to 86d27cd Compare January 9, 2018 00:11
@kubernetes kubernetes deleted a comment from k8s-github-robot Jan 9, 2018
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2018
iface, initiatorNamePtr, err := getISCSIInitiatorInfo(spec)
if err != nil {
return nil, err
func (plugin *iscsiPlugin) NewBlockVolumeMapper(spec *volume.Spec, pod *v1.Pod, _ volume.VolumeOptions) (volume.BlockVolumeMapper, error) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: GoDoc

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@@ -212,25 +176,87 @@ func (plugin *iscsiPlugin) newUnmounterInternal(volName string, podUID types.UID
}, nil
}

func (plugin *iscsiPlugin) NewBlockVolumeUnmapper(volName string, podUID types.UID) (volume.BlockVolumeUnmapper, error) {
Copy link
Member

Choose a reason for hiding this comment

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

GoDoc

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@mtanino mtanino force-pushed the pr/BlockVolumesSupport-iscsi branch from 86d27cd to 3e066d6 Compare January 9, 2018 21:02
@rootfs
Copy link
Member

rootfs commented Jan 10, 2018

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2018
Mitsuhiro Tanino added 2 commits January 10, 2018 11:38
This patch adds block volume support to iSCSI volume plugin.
@mtanino mtanino force-pushed the pr/BlockVolumesSupport-iscsi branch from 3e066d6 to 96509d4 Compare January 10, 2018 16:44
@rootfs
Copy link
Member

rootfs commented Jan 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtanino, rootfs

Associated issue: #805

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

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

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
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 54230, 58100, 57861, 54752). If you want to cherry-pick this change to another branch, please follow the instructions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants