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

Fix bug in isLikelyNotMountPoint function #27054

Merged
merged 1 commit into from
Jun 19, 2016

Conversation

jingxu97
Copy link
Contributor

@jingxu97 jingxu97 commented Jun 8, 2016

In nsenter_mount.go/isLikelyNotMountPoint function, the returned output
from findmnt command misses the last letter. Modify the code to use
String.contains instead of string matching. fixes #26421 fixes #25056 fixes #22911

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 8, 2016
@luxas luxas assigned pmorie and unassigned thockin Jun 8, 2016
@luxas luxas added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 8, 2016
glog.V(5).Infof("IsLikelyNotMountPoint findmnt output for path %s: %v", file, strOut)
if strOut == file {

//TODO: This is a temperory workaround for fixing the buggy code. Track the solution with issue #26996
Copy link
Member

Choose a reason for hiding this comment

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

nit: spelling "temporary"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

On Wed, Jun 8, 2016 at 11:20 AM, Saad Ali notifications@github.com wrote:

In pkg/util/mount/nsenter_mount.go
#27054 (comment)
:

glog.V(5).Infof("IsLikelyNotMountPoint findmnt output for path %s: %v", file, strOut)
  • if strOut == file {

nit: spelling "temporary"


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/27054/files/602e5aa032deae544e8560635ad3f48d30a7260d#r66310353,
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxfRs7Rk_9i6mogZubOq-XCi7AdFlks5qJwfigaJpZM4IxMdY
.

  • Jing

@saad-ali saad-ali self-assigned this Jun 8, 2016
@saad-ali
Copy link
Member

saad-ali commented Jun 8, 2016

LGTM but I'll leave it to @pmorie for final LGTM since he's most familiar with this code.

if strOut == file {

//TODO: This is a temperory workaround for fixing the buggy code. Track the solution with issue #26996
if strings.Contains(file, strOut) {
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like this fix. I do not want it to last very long, pleeeeeeease.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some clarification about this fix.

  1. The missing last letter from the output of findmnt command: I also
    tested this command in my local environment with a simple go program. The
    result still misses the last letter. Run command line directly on the
    terminal has no issue. I also tried adding options "--notruncate", "--raw",
    but still not working. So I think the bug is not related to our kubernetes
    code.

The output from findmnt is /usr/local/google/home/jinxu/work/src/
k8s.io/jingggggggggggggggggggggggggggggggggggg //the last letter t is
missing
The file path is /usr/local/google/home/jinxu/work/src/
k8s.io/jinggggggggggggggggggggggggggggggggggggt

  1. Use strings.Contains() will not result in false positive. We pass the
    testing mountpoint path when executing the findmnt command. As long as it
    gives back a result, no matter whether some letters are missing or not, it
    means that the given path is a mountpoint. Otherwise, an error returns.

We are thinking to further refactor this code because this function
currently servers as two purposes, one is to test whether the dir exist or
not, the other is testing mountpoint. It maybe be more clean to separate
the functions. Also we are debating whether to use other ways for checking
mountpoint instead of using findmnt

Please let me know if there are some concerns about the fix.

Thanks!
Jing

On Wed, Jun 8, 2016 at 11:45 AM, Tim Hockin notifications@github.com
wrote:

In pkg/util/mount/nsenter_mount.go
#27054 (comment)
:

glog.V(5).Infof("IsLikelyNotMountPoint findmnt output for path %s: %v", file, strOut)
  • if strOut == file {

I really don't like this fix. I do not want it to last very long,
pleeeeeeease.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/27054/files/602e5aa032deae544e8560635ad3f48d30a7260d#r66315282,
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxWcEZ5i_McdGy-vyTStU0GCaNCDbks5qJw3JgaJpZM4IxMdY
.

  • Jing

Copy link
Member

Choose a reason for hiding this comment

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

Can you reproduce this as a standalone Go program? If so, we should file a
bug with either Go team or findmnt and understand what the heck is
happening.

On Wed, Jun 8, 2016 at 3:01 PM, Jing Xu notifications@github.com wrote:

In pkg/util/mount/nsenter_mount.go
#27054 (comment)
:

glog.V(5).Infof("IsLikelyNotMountPoint findmnt output for path %s: %v", file, strOut)
  • if strOut == file {

Some clarification about this fix. 1. The missing last letter from the
output of findmnt command: I also tested this command in my local
environment with a simple go program. The result still misses the last
letter. Run command line directly on the terminal has no issue. I also
tried adding options "--notruncate", "--raw", but still not working. So I
think the bug is not related to our kubernetes code. The output from
findmnt is /usr/local/google/home/jinxu/work/src/
k8s.io/jingggggggggggggggggggggggggggggggggggg //the last letter t is
missing The file path is /usr/local/google/home/jinxu/work/src/
k8s.io/jinggggggggggggggggggggggggggggggggggggt 2. Use strings.Contains()
will not result in false positive. We pass the testing mountpoint path when
executing the findmnt command. As long as it gives back a result, no matter
whether some letters are missing or not, it means that the given path is a
mountpoint. Otherwise, an error returns. We are thinking to further
refactor this code because this function currently servers as two purposes,
one is to test whether the dir exist or not, the other is testing
mountpoint. It maybe be more clean to separate the functions. Also we are
debating whether to use other ways for checking mountpoint instead of using
findmnt Please let me know if there are some concerns about the fix.
Thanks! Jing
… <#m_4990089710392983408_>
On Wed, Jun 8, 2016 at 11:45 AM, Tim Hockin _@.**> wrote: In
pkg/util/mount/nsenter_mount.go <#27054 (comment)
https://github.com/kubernetes/kubernetes/pull/27054#discussion_r66315282>
: > glog.V(5).Infof("IsLikelyNotMountPoint findmnt output for path %s: %v",
file, strOut) > - if strOut == file { > + > + //TODO: This is a temperory
workaround for fixing the buggy code. Track the solution with issue #26996
#26996 > + if
strings.Contains(file, strOut) { I *really_ don't like this fix. I do not
want it to last very long, pleeeeeeease. — You are receiving this because
you authored the thread. Reply to this email directly, view it on GitHub <
https://github.com/kubernetes/kubernetes/pull/27054/files/602e5aa032deae544e8560635ad3f48d30a7260d#r66315282>,
or mute the thread <
https://github.com/notifications/unsubscribe/ASSNxWcEZ5i_McdGy-vyTStU0GCaNCDbks5qJw3JgaJpZM4IxMdY>
.
-- - Jing


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/27054/files/602e5aa032deae544e8560635ad3f48d30a7260d#r66348569,
or mute the thread
https://github.com/notifications/unsubscribe/AFVgVCahkx_F1GeGp0oBP_t3DbUtSgvFks5qJzufgaJpZM4IxMdY
.

@jingxu97
Copy link
Contributor Author

jingxu97 commented Jun 9, 2016

@pmorie, Could you please take a look at this fix? Thanks!

@pmorie
Copy link
Member

pmorie commented Jun 9, 2016

@jingxu97 it's in my queue for tonight, but I have other stuff I have to take care of first.

@jingxu97
Copy link
Contributor Author

@pmorie Sure. Thanks a lot!
I also want to discuss with you about the approach to check the mountpoint.
I am wondering whether we can reuse the approach implemented in
mount_linux.go. Please let me know your option about it. Thanks!

Jing

On Thu, Jun 9, 2016 at 4:59 PM, Paul Morie notifications@github.com wrote:

@jingxu97 https://github.com/jingxu97 it's in my queue for tonight, but
I have other stuff I have to take care of first.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27054 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxWCNRkVOl1HC0A4bnGKgPiFx3tLTks5qKKjsgaJpZM4IxMdY
.

  • Jing

if strOut == file {

//TODO: This is a temporary workaround for fixing the buggy code. Track the solution with issue #26996
if strings.Contains(file, strOut) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is going to fix things.

What happens if file is /a/likely/mountpoint-111 and strOut is /a/likely/mountpoint-11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if file is /a/likely/mountpoint-111 and strOut is /a/likely/mountpoint-11,
then strings.Contains(file, strOut) returns true and it is a mount point.

On Thu, Jun 9, 2016 at 6:49 PM, Paul Morie notifications@github.com wrote:

In pkg/util/mount/nsenter_mount.go
#27054 (comment)
:

glog.V(5).Infof("IsLikelyNotMountPoint findmnt output for path %s: %v", file, strOut)
  • if strOut == file {

I'm not sure this is going to fix things.

What happens if file is /a/likely/mountpoint-111 and strOut is
/a/likely/mountpoint-11?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/27054/files/201576f0aa8467f75308880148973bc6618704b5#r66551038,
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxUGwbDoBrLa53g4lfUT_BGQ-s3Boks5qKMLBgaJpZM4IxMdY
.

  • Jing

@pmorie
Copy link
Member

pmorie commented Jun 10, 2016

@jingxu97 Can you clarify why the last char of the output is dropped? Can we change the arguments of findmnt so that the last letter isn't dropped?

I would like to use the same check for containerized and for not. However, the problem with adopting the code for the non-containerized case is that it relies our golang code. We could use the same code for findmnt outside containers -- at least then the behavior would be consistent.

@eparis @thockin you guys have some history here -- any thoughts?

@pmorie
Copy link
Member

pmorie commented Jun 10, 2016

For the record I think my vote is to use findmnt inside and outside the container.

@jingxu97
Copy link
Contributor Author

@pmorie I haven't figured out the root cause for this behavior. But I tried
to mount tmpfs to a local directory, and then write a simple go file to
execute the similar findmnt command, the output dropped the last char if
the file path is more than about 90 characters. I tried a few options for
the findmnt such as "--notruncate", and "--raw", but still not working.
But if findmnt is executed directly on the terminal, the output is fine.

Jing

On Thu, Jun 9, 2016 at 6:55 PM, Paul Morie notifications@github.com wrote:

@jingxu97 https://github.com/jingxu97 Can you clarify why the last char
of the output is dropped? Can we change the arguments of findmnt so that
the last letter isn't dropped?

I would like to use the same check for containerized and for not. However,
the problem with adopting the code for the non-containerized case is that
it relies our golang code. We could use the same code for findmnt outside
containers -- at least then the behavior would be consistent.

@eparis https://github.com/eparis @thockin https://github.com/thockin
you guys have some history here -- any thoughts?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27054 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxfE0O1zYp7Fqmu5fIvoc2i__O-r_ks5qKMP3gaJpZM4IxMdY
.

  • Jing

@dims
Copy link
Member

dims commented Jun 10, 2016

@jingxu97 @pmorie - I am not able to recreate the problem with a simple go script like Jing mentions. Here's my attempt (http://paste.openstack.org/show/510674/) on ubuntu.

$ uname -a
Linux ubuntu-brix 4.4.0-21-generic #37-Ubuntu SMP Mon Apr 18 18:33:37 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
$ findmnt --version
findmnt from util-linux 2.27.1

@jingxu97, Jing, can you try printing before the trim? print the byte representation etc?

@jingxu97
Copy link
Contributor Author

The following is my code

file := "/usr/local/google/home/jinxu/work/src/k8s.io/jinggggggggggggggggggggggggggggggggggggt"
cmd := exec.Command("findmnt", "-f", "-u", "-r", "-n", "-o", "target",
"--target", file)
out, err := cmd.Output()
fmt.Printf("The output from findmnt is %x\n", out)
fmt.Printf("The file path is %x\n", file)

The hex output is

The output from findmnt is
2f7573722f6c6f63616c2f676f6f676c652f686f6d652f6a696e78752f776f726b2f7372632f6b38732e696f2f6a696e6767676767676767676767676767676767676767676767676767676767676767676767670a
The file path is
2f7573722f6c6f63616c2f676f6f676c652f686f6d652f6a696e78752f776f726b2f7372632f6b38732e696f2f6a696e67676767676767676767676767676767676767676767676767676767676767676767676774

You can see findmnt output last byte is 0a(new line), and compared
with original file path it misses 74 (letter t).

$uname -a

Linux jinxu0.mtv.corp.google.com 3.13.0-85-generic #129-Ubuntu SMP Thu
Mar 17 20:50:15 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

$ findmnt --version

On Fri, Jun 10, 2016 at 1:53 PM, Davanum Srinivas notifications@github.com
wrote:

@jingxu97 https://github.com/jingxu97 @pmorie
https://github.com/pmorie - I am not able to recreate the problem with
a simple go script like Jing mentions. Here's my attempt (
http://paste.openstack.org/show/510674/) on ubuntu.

$ uname -a
Linux ubuntu-brix 4.4.0-21-generic #37-Ubuntu SMP Mon Apr 18 18:33:37 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
$ findmnt --version
findmnt from util-linux 2.27.1

@jingxu97 https://github.com/jingxu97, Jing, can you try printing
before the trim? print the byte representation etc?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27054 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxZZObJTs9it0It4F2lmxViZPRuNFks5qKc6sgaJpZM4IxMdY
.

  • Jing

@dims
Copy link
Member

dims commented Jun 10, 2016

@jingxu97 as suspected the same code works for me. So can you redirect the output from running the "findmnt" straight from the command line into a file and use a hexdump see the output? ("hexdump -C a.txt")

the 0a is definitely the \n character.

@jingxu97
Copy link
Contributor Author

Running "findmnt" straight from the command line works fine. The hexdump
output is

00000000 2f 75 73 72 2f 6c 6f 63 61 6c 2f 67 6f 6f 67 6c |/usr/local/googl|
00000010 65 2f 68 6f 6d 65 2f 6a 69 6e 78 75 2f 77 6f 72 |e/home/jinxu/wor|
00000020 6b 2f 73 72 63 2f 6b 38 73 2e 69 6f 2f 6a 69 6e |k/src/k8s.io/jin|
00000030 67 67 67 67 67 67 67 67 67 67 67 67 67 67 67 67 |gggggggggggggggg|
*
00000050 67 67 67 67 74 0a |ggggt.|
00000056

Jing

On Fri, Jun 10, 2016 at 3:01 PM, Davanum Srinivas notifications@github.com
wrote:

@jingxu97 https://github.com/jingxu97 as suspected the same code works
for me. So can you redirect the output from running the "findmnt" straight
from the command line into a file and use a hexdump see the output?
("hexdump -C a.txt")

the 0a is definitely the \n character.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27054 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxYH6QMX-eUF4vbqiyG61Pfs35s78ks5qKd6igaJpZM4IxMdY
.

  • Jing

@dims
Copy link
Member

dims commented Jun 11, 2016

@jingxu97 Jing, Am out of ideas :) Did you try reading from the pipe directly?

    cmd := exec.Command("findmnt", "-f", "-u", "-r", "-n", "-o", "target", "--target", file)
    stdout, err := cmd.StdoutPipe()
    cmd.Start()
    r := bufio.NewReader(stdout)
    line, _, err := r.ReadLine()
    fmt.Printf("The output from findmnt is %s\n", line)

@jingxu97
Copy link
Contributor Author

@dims, unfortunately, the last letter dropped too from reading the pipe.

Jing

On Fri, Jun 10, 2016 at 5:43 PM, Davanum Srinivas notifications@github.com
wrote:

@jingxu97 https://github.com/jingxu97 Jing, Am out of ideas :) Did you
try reading from the pipe directly?

cmd := exec.Command("findmnt", "-f", "-u", "-r", "-n", "-o", "target", "--target", file)
stdout, err := cmd.StdoutPipe()
cmd.Start()
r := bufio.NewReader(stdout)
line, _, err := r.ReadLine()
fmt.Printf("The output from findmnt is %s\n", line)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27054 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxdHGTWVdFWBh_6Hb-k6ZgKdZzX_nks5qKgS8gaJpZM4IxMdY
.

  • Jing

@dims
Copy link
Member

dims commented Jun 11, 2016

Jing,

So how about we change strategy and print more stuff. example print target and the file system type then we can parse the strings out and even if we lose a character in the file system type, we won't have to care.

$ findmnt -f -u -r -n -o target,fstype --target /var/lib/kubelet/pods/092fb51d-2f44-11e6-83b9-408d5ca94602/volumes/kubernetes.io~secret/default-token-ysfc4
/var/lib/kubelet/pods/092fb51d-2f44-11e6-83b9-408d5ca94602/volumes/kubernetes.io~secret/default-token-ysfc4 tmpfs

@jingxu97
Copy link
Contributor Author

Davanum,

Yes, give two output options will work. For the current fix,
string.Contains() will also work I think. As I explained earlier in my
comment, use strings.Contains() will not result in any false positive. We
pass the mountpoint path when executing the findmnt command. As long as it
gives back a result, no matter whether some letters are missing or not, it
means that the given path is a mountpoint. The output will not give you any
paths other than the path given to the command. Otherwise, an error returns.

I think both approaches should work and please let me know your option
about them. Thanks!

Jing

On Fri, Jun 10, 2016 at 6:42 PM, Davanum Srinivas notifications@github.com
wrote:

Jing,

So how about we change strategy and print more stuff. example print target
and the file system type then we can parse the strings out and even if we
lose a character in the file system type, we won't have to care.

$ findmnt -f -u -r -n -o target,fstype --target /var/lib/kubelet/pods/092fb51d-2f44-11e6-83b9-408d5ca94602/volumes/kubernetes.iosecret/default-token-ysfc4
/var/lib/kubelet/pods/092fb51d-2f44-11e6-83b9-408d5ca94602/volumes/kubernetes.io
secret/default-token-ysfc4 tmpfs


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27054 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxfEn2KvdmjwFB-jPe5R8QqsT0txdks5qKhJzgaJpZM4IxMdY
.

  • Jing

@dims
Copy link
Member

dims commented Jun 11, 2016

@jingxu97 Jing, right, i was really trying hard to use an explicit compare (avoid strings.Contains). So the printing extra stuff and splitting the strings, followed by a compare seems a better option. (But i am a newbie here!).

@jingxu97
Copy link
Contributor Author

I modified the fix based on the suggestions from @dims. Please review it. Thanks!

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 13, 2016
@@ -188,13 +190,15 @@ func (n *NsenterMounter) IsLikelyNotMountPoint(file string) (bool, error) {
// It's safer to assume that it's not a mount point.
return true, nil
}
strOut := strings.TrimSuffix(string(out), "\n")
strOut := strings.Split(string(out), " ")[0]
Copy link
Member

Choose a reason for hiding this comment

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

@jingxu97 Jing, When we split the string we won't need to do a Trim i think. but you will have to confirm what you see in your environment :) LGTM otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually here we do need a Trim. Otherwise, strOut has some empty spaces at
the end and it won't equal to file. I tested this in my environment
already. Thanks!

On Mon, Jun 13, 2016 at 1:57 PM, Davanum Srinivas notifications@github.com
wrote:

In pkg/util/mount/nsenter_mount.go
#27054 (comment)
:

@@ -188,13 +190,15 @@ func (n *NsenterMounter) IsLikelyNotMountPoint(file string) (bool, error) {
// It's safer to assume that it's not a mount point.
return true, nil
}

  • strOut := strings.TrimSuffix(string(out), "\n")
  • strOut := strings.Split(string(out), " ")[0]

@jingxu97 https://github.com/jingxu97 Jing, When we split the string we
won't need to do a Trim i think. but you will have to confirm what you see
in your environment :) LGTM otherwise.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/27054/files/4cfdf00eb68fdab213a5a28854e9876256adbe4c#r66866299,
or mute the thread
https://github.com/notifications/unsubscribe/ASSNxcA1wCVXxsEsaENWSN1yN6fuOyvdks5qLcRTgaJpZM4IxMdY
.

  • Jing

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! thanks Jing

Copy link
Member

Choose a reason for hiding this comment

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

@jingxu97 Let's make the code a little easier to follow while you're in here. Would you rename strOut to mountTarget ?

@pmorie
Copy link
Member

pmorie commented Jun 13, 2016

Just a couple nits to make the code easier to understand, LGTM otherwise. @jingxu97

In nsenter_mount.go/isLikelyNotMountPoint function, the returned output
from findmnt command misses the last letter. Modify the code to make sure
that output has the full target path. fix kubernetes#26421 kubernetes#25056 kubernetes#22911
@erictune erictune added this to the v1.3 milestone Jun 15, 2016
@jingxu97 jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2016
@jingxu97
Copy link
Contributor Author

Fix the nits suggested by @pmorie. Attach LGTM label to merge.

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2016

GCE e2e build/test passed for commit 809dae1.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 19, 2016

GCE e2e build/test passed for commit 809dae1.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
10 participants