-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: spelling "temporary"
There was a problem hiding this comment.
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 {
- //TODO: This is a temperory workaround for fixing the buggy code. Track the solution with issue isLikelyNotMountPoint() function needs refactored #26996
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
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- 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
- 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 {
- //TODO: This is a temperory workaround for fixing the buggy code. Track the solution with issue isLikelyNotMountPoint() function needs refactored #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
There was a problem hiding this comment.
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 {
- //TODO: This is a temperory workaround for fixing the buggy code. Track the solution with issue isLikelyNotMountPoint() function needs refactored #26996
- if strings.Contains(file, strOut) {
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
.
@pmorie, Could you please take a look at this fix? Thanks! |
@jingxu97 it's in my queue for tonight, but I have other stuff I have to take care of first. |
@pmorie Sure. Thanks a lot! Jing On Thu, Jun 9, 2016 at 4:59 PM, Paul Morie notifications@github.com wrote:
|
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) { |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 {
- //TODO: This is a temporary workaround for fixing the buggy code. Track the solution with issue isLikelyNotMountPoint() function needs refactored #26996
- if strings.Contains(file, strOut) {
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
@jingxu97 Can you clarify why the last char of the output is dropped? Can we change the arguments of 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? |
For the record I think my vote is to use |
@pmorie I haven't figured out the root cause for this behavior. But I tried Jing On Thu, Jun 9, 2016 at 6:55 PM, Paul Morie notifications@github.com wrote:
|
@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.
@jingxu97, Jing, can you try printing before the trim? print the byte representation etc? |
The following is my code file := "/usr/local/google/home/jinxu/work/src/k8s.io/jinggggggggggggggggggggggggggggggggggggt" The hex output is The output from findmnt is You can see findmnt output last byte is 0a(new line), and compared $uname -a Linux jinxu0.mtv.corp.google.com 3.13.0-85-generic #129-Ubuntu SMP Thu $ findmnt --version On Fri, Jun 10, 2016 at 1:53 PM, Davanum Srinivas notifications@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. |
Running "findmnt" straight from the command line works fine. The hexdump 00000000 2f 75 73 72 2f 6c 6f 63 61 6c 2f 67 6f 6f 67 6c |/usr/local/googl| Jing On Fri, Jun 10, 2016 at 3:01 PM, Davanum Srinivas notifications@github.com
|
@jingxu97 Jing, Am out of ideas :) Did you try reading from the pipe directly?
|
@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
|
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.
|
Davanum, Yes, give two output options will work. For the current fix, I think both approaches should work and please let me know your option Jing On Fri, Jun 10, 2016 at 6:42 PM, Davanum Srinivas notifications@github.com
|
@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!). |
I modified the fix based on the suggestions from @dims. Please review it. Thanks! |
@@ -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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! thanks Jing
There was a problem hiding this comment.
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
?
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
Fix the nits suggested by @pmorie. Attach LGTM label to merge. |
GCE e2e build/test passed for commit 809dae1. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 809dae1. |
Automatic merge from submit-queue |
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