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

implement optional ControllerPublishVolume #85

Open
wants to merge 1 commit into
base: master
from

Conversation

@zhucan
Copy link
Member

commented Aug 14, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
To test ControllerPublishVolume and ControllerUnpublishVolume could be called
Which issue(s) this PR fixes:
Fixes #80

Special notes for your reviewer:
@pohly

Does this PR introduce a user-facing change?:

implement optional ControllerPublishVolume
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhucan
To complete the pull request process, please assign saad-ali
You can assign the PR to them by writing /assign @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found 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 requested review from msau42 and saad-ali Aug 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/M label Aug 14, 2019
@zhucan

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

/assign @pohly

@@ -36,7 +36,8 @@ var (
ephemeral = flag.Bool("ephemeral", false, "publish volumes in ephemeral mode even if kubelet did not ask for it (only needed for Kubernetes 1.15)")
showVersion = flag.Bool("version", false, "Show version.")
// Set by the build process
version = ""
version = ""
controller = flag.Bool("controller", false, "Open ControllerPublishVolume and ControllerUnpublishVolume")

This comment has been minimized.

Copy link
@msau42

msau42 Aug 16, 2019

Collaborator

i would make the name more specific, like "controllerPublish". and the description "implements PUBLISH_UNPUBLISH_VOLUME ControllerServiceCapability"

This comment has been minimized.

Copy link
@zhucan

zhucan Aug 16, 2019

Author Member

copy that, thanks

if ephemeral {
return &controllerServer{caps: getControllerServiceCapabilities(nil)}
}

if controller {
return &controllerServer{

This comment has been minimized.

Copy link
@msau42

msau42 Aug 16, 2019

Collaborator

Instead of duplicating the array, I would append to the default array if the flag is enabled

This comment has been minimized.

Copy link
@zhucan

zhucan Aug 16, 2019

Author Member

copy that, thanks

@@ -234,11 +247,13 @@ func (cs *controllerServer) ValidateVolumeCapabilities(ctx context.Context, req
}

func (cs *controllerServer) ControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) {
return nil, status.Error(codes.Unimplemented, "")
glog.V(4).Infof("ControllerPublishVolume %v", req)

This comment has been minimized.

Copy link
@msau42

msau42 Aug 16, 2019

Collaborator

In the issue, @pohly suggested that we persist some metadata here so that in NodePublishVolume, we can verify that the ControllerPublishVolume was called.

This comment has been minimized.

Copy link
@zhucan

zhucan Aug 16, 2019

Author Member

for example? @msau42

This comment has been minimized.

Copy link
@zhucan

zhucan Aug 16, 2019

Author Member

Check request? @msau42 @pohly

This comment has been minimized.

Copy link
@zhucan

zhucan Aug 16, 2019

Author Member

we can know whether ControllerPublishVolume was called through the print of logs

This comment has been minimized.

Copy link
@msau42

msau42 Aug 20, 2019

Collaborator

I think the suggestion patrick had was to actually write some metadata to a file and then during NodePublishVolume, we can programatically verify it.

This comment has been minimized.

Copy link
@hoyho

hoyho Oct 9, 2019

Member

I think the suggestion patrick had was to actually write some metadata to a file and then during NodePublishVolume, we can programatically verify it.

How about set a/some k-v in ControllerPublishVolumeResponse field PublishContext
e.g.

	return &csi.ControllerPublishVolumeResponse{
		PublishContext: map[string]string{
			"ControllerPublished":"true",
		},
	}, nil

During NodePublishVolume, we can verify it by checking parameter NodePublishVolumeRequest.PublishContext

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.