Use local PX endpoint for mount, unmount, detach and attach calls #48898

harsh-px · 2017-07-13T19:47:33Z

What this PR does / why we need it:
This PR fixes an issue with Setup and TearDown of Portworx volumes which has side-effects such a Pod using a Portworx volume not being able to start on the minion.

Which issue this PR fixes: fixes #49034
This PR addresses an issue that fails to mount, attach, unmount or detach a volume when Kubernetes sends these requests to Portworx when it's API server on that particular minion is down.

Portworx mount, unmount, attach and detach requests need to be received on the minion where the pod is running. So these calls need to talk to the Portworx API server running locally on that node (and NOT to the Portworx k8s service since it may route the request to any node in the cluster). This PR explicitly makes such requests local only.

Release note:

Fix Pods using Portworx volumes getting stuck in ContainerCreating phase.

k8s-ci-robot · 2017-07-13T19:47:40Z

Hi @harsh-px. Thanks for your PR.

I'm waiting for a kubernetes 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.

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. I understand the commands that are listed here.

harsh-px · 2017-07-13T19:59:40Z

/assign @gnufied

gnufied

Some minor nits - mostly lgtm

gnufied · 2017-07-13T20:01:39Z

pkg/volume/portworx/portworx.go

@@ -266,7 +266,7 @@ func (b *portworxVolumeMounter) SetUp(fsGroup *int64) error {
 // SetUpAt attaches the disk and bind mounts to the volume path.
 func (b *portworxVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 	notMnt, err := b.mounter.IsLikelyNotMountPoint(dir)
-	glog.V(4).Infof("Portworx Volume set up: %s %v %v", dir, !notMnt, err)
+	glog.Infof("Portworx Volume set up. Dir: %s %v %v", dir, !notMnt, err)


Do we really want to log these messages always in the log files? Looks to me that, older logging priority was more accurate.

We have found tracking down events for volume setup and teardown very important in debugging field issues and hence I've changed the logging level. SetupAt and TearDownAt get invoked only when pods get scheduled on a new node (new pod or existing pod getting re-scheduled). So these logs entries won't be very chatty.

gnufied · 2017-07-13T20:05:50Z

pkg/volume/portworx/portworx_util.go

 	return volumeID, requestGB, nil, err
 }

 // DeleteVolume deletes a Portworx volume
 func (util *PortworxVolumeUtil) DeleteVolume(d *portworxVolumeDeleter) error {
-	driver, err := util.getPortworxDriver(d.plugin.host)
+	driver, err := util.getPortworxDriver(d.plugin.host, false)


Everywhere we add a naked true or false argument - we should also add a comment of the form true /*localOnly*/

Done in incremental diff

gnufied · 2017-07-13T20:06:36Z

pkg/volume/portworx/portworx_util.go

@@ -181,13 +186,24 @@ func createDriverClient(hostname string) (*osdclient.Client, error) {
 	}
 }

-func (util *PortworxVolumeUtil) getPortworxDriver(volumeHost volume.VolumeHost) (volumeapi.VolumeDriver, error) {
+func (util *PortworxVolumeUtil) getPortworxDriver(volumeHost volume.VolumeHost, localOnly bool) (volumeapi.VolumeDriver, error) {


Can you document - what that localOnly flag does? Why certain calls supply this as true while others as false ?

Added a function doc in the incremental

I read the comment - and I still don't understand why CreateDisk and DeleteDisk pass this flag as false while others as true.

Feel free to link to some external docs if the context is large enough to not fit in a code comment. We frequently do that in AWS and other volume plugins (or we should be doing that).

Please see if the update comment gives more context. I have given a high-level context on why create and delete calls can use localOnly as false while other's cannot.

Currently, we don't have external docs describing our internal algorithms (most of them are proprietary.)

gnufied · 2017-07-13T20:07:07Z

pkg/volume/portworx/portworx_util.go

+		if err != nil {
+			return nil, err
+		} else {
+			glog.Infof("Using portworx local service at: %v as api endpoint", volumeHost.GetHostName())


IMO - this log message should only appear if verbosity is set to a lower level, not always.

Changed to glog.V(4) in the incremental

gnufied · 2017-07-14T01:42:52Z

@k8s-bot ok to test

harsh-px · 2017-07-14T16:44:42Z

Re-run of the failing test succeeded.

…orxDriver function

…ction

gnufied · 2017-07-14T20:43:27Z

/lgtm

harsh-px · 2017-07-15T00:33:32Z

@gnufied Thanks for reviewing this. I am guessing you are the only who adds the approved label?

gnufied · 2017-07-17T14:57:10Z

/approve

k8s-github-robot · 2017-07-17T14:57:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gnufied, harsh-px

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

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

~~pkg/volume/OWNERS~~ [gnufied]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

gnufied · 2017-07-17T14:57:57Z

@harsh-px can you open a github issue and link this PR there?

harsh-px · 2017-07-17T17:33:23Z

@gnufied Created issue and linked here.

k8s-github-robot · 2017-07-17T20:23:51Z

Automatic merge from submit-queue (batch tested with PRs 48997, 48595, 48898, 48711, 48972)

harsh-px · 2017-07-17T21:11:35Z

@gnufied I need to cherry-pick this into the next 1.7 and 1.6 release. However, the cherry-pick PR would need the milestone label set for these releases in this PR.

What is the process for getting these milestone labels?

gnufied · 2017-07-17T21:36:55Z

@harsh-px you open the PRs against the branches. Since change here is mostly a bug fix, it would be cherry picked by release manager of those branches.

harsh-px · 2017-07-18T15:57:27Z

/release-note Fixes an issue where a pod using a Portworx volume gets stuck in ContainerCreating phase.

…98-upstream-release-1.7 Automated cherry pick of #48898 upstream release 1.7

…98-upstream-release-1.6 Automatic merge from submit-queue Automated cherry pick of #48898 upstream release 1.6 **What this PR does / why we need it**: This PR fixes an issue with Setup and TearDown of Portworx volumes which has side-effects such a Pod using a Portworx volume not being able to start on the minion. **Which issue this PR fixes**: This PR addresses an issue that fails to mount, attach, unmount or detach a volume when Kubernetes sends these requests to Portworx when it's API server on that particular minion is down. Portworx mount, unmount, attach and detach requests need to be received on the minion where the pod is running. So these calls need to talk to the Portworx API server running locally on that node (and NOT to the Portworx k8s service since it may route the request to any node in the cluster). This PR explicitly makes such requests local only. Cherrypick of #48898

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 13, 2017

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 Jul 13, 2017

k8s-github-robot assigned deads2k and gnufied Jul 13, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 13, 2017

harsh-px force-pushed the fix-px-volume-calls branch from 76a8c74 to a0d6aa1 Compare July 13, 2017 19:56

gnufied suggested changes Jul 13, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 14, 2017

Harsh Desai added 3 commits July 14, 2017 13:20

Use local PX endpoint for mount, unmount, detach and attach calls

8d0dec7

Fix logging levels in Portworx volume driver and add doc for getPortw…

7268e2e

…orxDriver function

Add more detailed comment for localOnly flag in getPortworxDriver fun…

90919e3

…ction

harsh-px force-pushed the fix-px-volume-calls branch from 8e4f1f8 to 90919e3 Compare July 14, 2017 20:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2017

harsh-px mentioned this pull request Jul 17, 2017

A pod using a Portworx volume driver PVC gets stuck in ContainerCreating phase #49034

Closed

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2017

k8s-github-robot merged commit 2c1c33d into kubernetes:master Jul 17, 2017

harsh-px mentioned this pull request Jul 18, 2017

Automated cherry pick of #48898 upstream release 1.7 #49067

Merged

harsh-px mentioned this pull request Jul 18, 2017

Automated cherry pick of #48898 upstream release 1.6 #49068

Merged

enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 18, 2017

wojtek-t added a commit that referenced this pull request Jul 19, 2017

Merge pull request #49067 from portworx/automated-cherry-pick-of-#488…

d44672e

…98-upstream-release-1.7 Automated cherry pick of #48898 upstream release 1.7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use local PX endpoint for mount, unmount, detach and attach calls #48898

Use local PX endpoint for mount, unmount, detach and attach calls #48898

harsh-px commented Jul 13, 2017 •

edited by enisoc

Loading

k8s-ci-robot commented Jul 13, 2017

harsh-px commented Jul 13, 2017

gnufied left a comment •

edited

Loading

gnufied Jul 13, 2017

harsh-px Jul 13, 2017

gnufied Jul 13, 2017

harsh-px Jul 13, 2017

gnufied Jul 13, 2017

harsh-px Jul 13, 2017

gnufied Jul 14, 2017 •

edited

Loading

gnufied Jul 14, 2017 •

edited

Loading

harsh-px Jul 14, 2017

gnufied Jul 13, 2017

harsh-px Jul 13, 2017

gnufied commented Jul 14, 2017

harsh-px commented Jul 14, 2017

gnufied commented Jul 14, 2017

harsh-px commented Jul 15, 2017

gnufied commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

gnufied commented Jul 17, 2017

harsh-px commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

harsh-px commented Jul 17, 2017

gnufied commented Jul 17, 2017

harsh-px commented Jul 18, 2017

Use local PX endpoint for mount, unmount, detach and attach calls #48898

Use local PX endpoint for mount, unmount, detach and attach calls #48898

Conversation

harsh-px commented Jul 13, 2017 • edited by enisoc Loading

k8s-ci-robot commented Jul 13, 2017

harsh-px commented Jul 13, 2017

gnufied left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

gnufied Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Jul 14, 2017

harsh-px commented Jul 14, 2017

gnufied commented Jul 14, 2017

harsh-px commented Jul 15, 2017

gnufied commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

gnufied commented Jul 17, 2017

harsh-px commented Jul 17, 2017

k8s-github-robot commented Jul 17, 2017

harsh-px commented Jul 17, 2017

gnufied commented Jul 17, 2017

harsh-px commented Jul 18, 2017

harsh-px commented Jul 13, 2017 •

edited by enisoc

Loading

gnufied left a comment •

edited

Loading

gnufied Jul 14, 2017 •

edited

Loading

gnufied Jul 14, 2017 •

edited

Loading