Skip to content

In the case of remounting with changed data, need to call mount - #34077

Merged
cpuguy83 merged 1 commit into
moby:masterfrom
justincormack:mount_remount_opts
Jul 14, 2017
Merged

cpuguy83 merged 1 commit into
moby:masterfrom
justincormack:mount_remount_opts

Conversation

@justincormack

Copy link
Copy Markdown
Contributor

The case where we are trying to do a remount with changed filesystem specific options was missing,
we need to call mount as well here to change those options.

See #33844 for where we need this, as we change tmpfs options.

Signed-off-by: Justin Cormack justin.cormack@docker.com

@justincormack

Copy link
Copy Markdown
Contributor Author

cc @cyli can you test if this resolves your issue?

@cyli

cyli commented Jul 12, 2017

Copy link
Copy Markdown
Contributor

@justincormack Yes, this does correctly remount the secrets dir, thanks!

@shouze

shouze commented Jul 12, 2017

Copy link
Copy Markdown
Contributor

Lacks some unit test or so to avoid future regressions, no?

@thaJeztah

Copy link
Copy Markdown
Member

ping @cpuguy83 @estesp PTAL

@justincormack

Copy link
Copy Markdown
Contributor Author

@shouze yes, the current test setup does not really work for this, only for bind mounts, I will add some other tests.

@justincormack

Copy link
Copy Markdown
Contributor Author

Added a test that will cover this case.

The case where we are trying to do a remount with changed filesystem specific options was missing,
we need to call `mount` as well here to change those options.

See moby#33844 for where we need this, as we change `tmpfs` options.

Signed-off-by: Justin Cormack <justin.cormack@docker.com>

@estesp estesp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sorry I didn't see the original issue; I would have guessed first it was about changing mount options as I fought with that so many times when first doing the userns implementation :-]

@justincormack

Copy link
Copy Markdown
Contributor Author

cc @cpuguy83

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants