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

Replace `busybox mount` with direct read of /proc/mounts #6608

Merged
merged 3 commits into from Jun 13, 2019

Conversation

@smacfarlane
Copy link
Contributor

commented Jun 3, 2019

Closes #6591
Closes #6577

This replaces $bb mount with a direct read of /proc/mounts to check if a filesystem is mounted. See #6591 for details.

@smacfarlane smacfarlane requested review from baumanj and eeyun as code owners Jun 3, 2019

@chef-expeditor

This comment has been minimized.

Copy link

commented Jun 3, 2019

Hello smacfarlane! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@smacfarlane smacfarlane force-pushed the sm/fix-studio-umount branch from 1742f35 to 11d3f59 Jun 3, 2019


$bb mount | $bb grep -q "on $_mount_point type"

# Work around bug in musl's implementation of getmntent_r.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

Is this the bug? It would be good to reference it here so we can remove our workaround when the underlying issue is fixed.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jun 6, 2019

Contributor

Should we update this comment in light of #6591 (comment)?

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 12, 2019

Author Contributor

That's a related bug, #6591 (comment) is the specific case we're hitting. I've added a link to the issue as a comment.

cleanup() {
sudo umount "$mnt_path" || true
sudo rm -rf "$mnt_path"
( cd "$tmpdir"/studio && $STUDIO_COMMAND rm )

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 5, 2019

Member

Does $STUDIO_COMMAND have to be an env var? If it were an argument to this script, I think that would be safer since unquoted, $STUDIO_COMMAND is subject to globbing, which we don't want:

Suggested change
( cd "$tmpdir"/studio && $STUDIO_COMMAND rm )
( cd "$tmpdir"/studio && "$@" rm )

or add this at the beginning:

studio_command=("$@")

and then

Suggested change
( cd "$tmpdir"/studio && $STUDIO_COMMAND rm )
( cd "$tmpdir"/studio && "${studio_command[@]}" rm )

A check for at least one argument might be good too.

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 12, 2019

Author Contributor

I prefer the argument-style invocation as well, this was done for consistency with how the expect studio-enter.exp script is invoked. Since the argument is one or two args, depending on what we're testing (hab studio vs /path/to/hab-studio.sh) the env var was a much more straight forward solution.

I'm doing some investigation to see if I can make it work as an argument to the expect script, that would be preferred. If not, I think I would like to keep consistency in how we invoke the tests.

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 12, 2019

Member

I'll leave it to your discretion

smacfarlane added some commits Jun 3, 2019

Add test case to detect studio rm failure on long mount entry
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
Fix is_fs_mounted() to work around musl's getmntent_r bug
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

@smacfarlane smacfarlane force-pushed the sm/fix-studio-umount branch from 83e3308 to 5c84cec Jun 13, 2019

# scenario.

# NOTE(SM): This makes this studio implementation Linux specific.
$bb cut -d' ' -f2 /proc/mounts | $bb grep -q "$_mount_point"

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 13, 2019

Author Contributor

I think I need to do a little more work here. The grep previously matched on $mount type so was guaranteed to only match one item. We're splitting on whitespace from /proc/mount now so there is the potential for multiple matches. If busybox grep supports it, I'm going to switch this to a literal match and verify it works.

cc @baumanj

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jun 13, 2019

Author Contributor

By way of example:

[23][default:/src:130]# cut -d' ' -f2 /proc/mounts |grep -x /dev
/dev
[24][default:/src:0]# cut -d' ' -f2 /proc/mounts |grep /dev
/dev
/dev/pts

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 13, 2019

Member

That seems reasonable. The -x flag is part of POSIX for grep and busybox does seem to support it

Use -x to match full line only
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>

@smacfarlane smacfarlane merged commit 856c27a into master Jun 13, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2361 passed (49 minutes, 5 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2732 passed (34 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

chef-ci added a commit that referenced this pull request Jun 13, 2019

Update CHANGELOG.md with details from pull request #6608
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.