UPSTREAM: 49118: Allow unmounting bind-mounted directories. #15294

adelton · 2017-07-18T13:56:35Z

For files, we cannot use path/..;
we could use filepath.Dir but for bind-mounted, isNotMounted
which calls IsLikelyNotMountPoint would not work anyway.
Let's just have the driver do the work.

Addressing

Error: UnmountVolume.TearDown failed for volume "..." (volume.spec.Name: "...") pod "..." (UID: "...") with: lstat /path/.../test-flex/..: not a directory

adelton · 2017-07-18T13:56:57Z

Cc @simo5, @openshift/security.

php-coder · 2017-07-18T14:04:40Z

@adelton When you're editing files inside vendor/k8s.io/kubernetes directory then 1) you must follow some special conventions for commit's description 2) in many cases you should submit the similar fix to the Kubernetes itself (or even only to the Kubernetes if it's not critical to OpenShift).

pweil- · 2017-07-18T14:06:07Z

@openshift/storage

php-coder · 2017-07-18T14:11:28Z

you must follow some special conventions for commit's description

I see that Travis already has failing the build with the error about that:

===== Verifying UPSTREAM Commits =====

The following commits contain Godeps changes but aren't declared as UPSTREAM:

[de66f0e] Allow unmounting bind-mounted files.

  - vendor/k8s.io/kubernetes/pkg/volume/flexvolume/unmounter.go

Unfortunately it doesn't mention what are these conventions. Here is the link that I'm using:

origin/tools/rebasehelpers/commitchecker/validate.go

Lines 19 to 38 in 7d661a2

    
           UPSTREAM commit summaries should look like: 
        
             UPSTREAM: [non-kube-repo/name: ]<PR number|carry|drop>: description 
        
           UPSTREAM commits which revert previous UPSTREAM commits should look like: 
        
             UPSTREAM: revert: <sha>: <normal upstream format> 
        
           UPSTREAM commits are validated against the following regular expression: 
        
             {{ .Pattern }} 
        
           Examples of valid summaries: 
        
             UPSTREAM: 12345: A kube fix 
        
             UPSTREAM: coreos/etcd: 12345: An etcd fix 
        
             UPSTREAM: <carry>: A carried kube change 
        
             UPSTREAM: <drop>: A dropped kube change 
        
             UPSTREAM: revert: abcd123: coreos/etcd: 12345: An etcd fix 
        
             UPSTREAM: k8s.io/heapster: 12345: A heapster fix

pweil- · 2017-07-18T14:14:35Z

Unfortunately it doesn't mention what are these conventions

See https://github.com/openshift/origin/blob/master/HACKING.md#using-hackcherry-pick

liggitt · 2017-07-18T14:15:53Z

this should be opened upstream for review and approval first.

we only pick changes into origin if they are fixing release-blocking bugs, otherwise we wait to pick up the kube level with upstream improvements

adelton · 2017-07-18T14:28:11Z

Filed upstream now: kubernetes/kubernetes#49118. Let's see.

jsafrane · 2017-07-25T07:51:26Z

@adelton, please update the commit message.

https://github.com/openshift/origin/blob/master/HACKING.md#using-hackcherry-pick says:

All upstream commits should have a commit message where the first line is:
UPSTREAM: <PR number|drop|carry>: <short description>

Check git log for examples.

adelton · 2017-07-25T07:51:26Z

The kubernetes/kubernetes#49118 was merged. Do we want to get that change in to OpenShift Origin now, making testing with bind-mounted directories (and files) easier, or wait for rebase and drop this pull request?

For bind-mounted directories, the isNotMounted which calls IsLikelyNotMountPoint fails because the filesystem of the mounted location and the parent directory are the same. Addressing: unmounter.go:59] Warning: Path: /path/.../test-dir already unmounted

adelton · 2017-07-25T08:10:37Z

Updated commit message, rebased on master -> 139e0d5.

jsafrane · 2017-07-25T09:58:46Z

[test]

openshift-bot · 2017-07-25T10:07:42Z

Evaluated for origin test up to 139e0d5

openshift-bot · 2017-07-25T12:23:08Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3477/) (Base Commit: 970dc61) (PR Branch Commit: 139e0d5)

jsafrane · 2017-07-25T14:14:39Z

[merge]

openshift-bot · 2017-07-25T14:15:48Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 2

openshift-bot · 2017-07-25T14:15:49Z

Evaluated for origin merge up to 139e0d5

jsafrane · 2017-07-31T12:17:46Z

/approve no-issue

adelton · 2017-08-07T08:12:42Z

What can be done to get this merged? That "Submit Queue" check does not really give much clue about what it feels is wrong with the change.

jsafrane · 2017-08-09T08:40:55Z

/test all

jsafrane · 2017-08-09T14:34:10Z

/lgtm

openshift-merge-robot · 2017-08-09T14:34:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adelton, jsafrane

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

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

~~vendor/k8s.io/kubernetes/pkg/volume/OWNERS~~ [jsafrane]

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

jsafrane · 2017-08-09T14:34:41Z

/test ci/openshift-jenkins/extended_templates

jsafrane · 2017-08-09T14:35:09Z

/test extended_templates

openshift-merge-robot · 2017-08-10T01:57:33Z

/test all [submit-queue is verifying that this PR is safe to merge]

jsafrane · 2017-08-10T11:31:24Z

/retest

jsafrane · 2017-08-10T12:48:12Z

/retest

adelton · 2017-08-10T14:00:24Z

Those https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_install_update/3880/ failures don't seem like something this patch could have caused. What is the best way to report / escalate those, @jsafrane ?

openshift-bot · 2017-08-11T00:16:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

jsafrane · 2017-08-11T07:27:13Z

/retest

openshift-merge-robot · 2017-08-11T12:19:22Z

/test all [submit-queue is verifying that this PR is safe to merge]

jsafrane · 2017-08-11T13:07:34Z

/retest

jsafrane · 2017-08-11T15:16:19Z

/retest

openshift-bot · 2017-08-12T02:29:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2017-08-12T18:37:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-08-12T20:04:13Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-12T21:58:24Z

Automatic merge from submit-queue

adelton changed the title ~~Allow unmounting bind-mounted files.~~ Allow unmounting bind-mounted files. [pending review upstream] Jul 18, 2017

openshift-merge-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 24, 2017

jsafrane changed the title ~~Allow unmounting bind-mounted files. [pending review upstream]~~ Allow unmounting bind-mounted files. Jul 25, 2017

adelton force-pushed the flex-bind-mount branch from de66f0e to 139e0d5 Compare July 25, 2017 08:08

adelton changed the title ~~Allow unmounting bind-mounted files.~~ Allow unmounting bind-mounted directories. Jul 25, 2017

adelton changed the title ~~Allow unmounting bind-mounted directories.~~ UPSTREAM: 49118: Allow unmounting bind-mounted directories. Jul 25, 2017

stevekuznetsov assigned sttts Jul 25, 2017

sttts assigned derekwaynecarr and unassigned sttts Jul 26, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2017

openshift-ci-robot assigned jsafrane Aug 9, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2017

openshift-merge-robot merged commit ef7d658 into openshift:master Aug 12, 2017

adelton deleted the flex-bind-mount branch September 8, 2017 06:56

UPSTREAM: 49118: Allow unmounting bind-mounted directories. #15294

UPSTREAM: 49118: Allow unmounting bind-mounted directories. #15294

Conversation

adelton commented Jul 18, 2017

adelton commented Jul 18, 2017

php-coder commented Jul 18, 2017 • edited Loading

pweil- commented Jul 18, 2017

php-coder commented Jul 18, 2017

pweil- commented Jul 18, 2017

liggitt commented Jul 18, 2017 • edited Loading

adelton commented Jul 18, 2017

jsafrane commented Jul 25, 2017

adelton commented Jul 25, 2017 • edited Loading

adelton commented Jul 25, 2017

jsafrane commented Jul 25, 2017

openshift-bot commented Jul 25, 2017

openshift-bot commented Jul 25, 2017

jsafrane commented Jul 25, 2017

openshift-bot commented Jul 25, 2017 • edited Loading

openshift-bot commented Jul 25, 2017

jsafrane commented Jul 31, 2017

adelton commented Aug 7, 2017

jsafrane commented Aug 9, 2017

jsafrane commented Aug 9, 2017

openshift-merge-robot commented Aug 9, 2017

jsafrane commented Aug 9, 2017

jsafrane commented Aug 9, 2017

openshift-merge-robot commented Aug 10, 2017

jsafrane commented Aug 10, 2017

jsafrane commented Aug 10, 2017

adelton commented Aug 10, 2017

openshift-bot commented Aug 11, 2017

jsafrane commented Aug 11, 2017

openshift-merge-robot commented Aug 11, 2017

jsafrane commented Aug 11, 2017

jsafrane commented Aug 11, 2017

openshift-bot commented Aug 12, 2017

openshift-bot commented Aug 12, 2017

openshift-merge-robot commented Aug 12, 2017

openshift-merge-robot commented Aug 12, 2017

php-coder commented Jul 18, 2017 •

edited

Loading

liggitt commented Jul 18, 2017 •

edited

Loading

adelton commented Jul 25, 2017 •

edited

Loading

openshift-bot commented Jul 25, 2017 •

edited

Loading