agent: Simplify mount point creation #2428
Conversation
dgibson
requested review from
Tim-Zhang,
marcel-apf,
devimc,
yyyeerbo,
lifupan and
egernst
dgibson
force-pushed
the
simplify-mount-storage
branch
from
c1b3431
to
672ee48
Compare
dgibson
added
the
wip
|
/test |
|
Hrm, AFAICT the metrics and vfio test failures aren't related to this patch (I've seen them on unrelated PRs). Seems to be some problem setting up the environment there. |
dgibson
force-pushed
the
simplify-mount-storage
branch
from
672ee48
to
5efd883
Compare
dgibson
removed
the
wip
|
/test |
|
/retest-vfio |
|
/test-ubuntu-metrics |
|
I'm reviewing now (thanks!) Reviewing now. We'll want to make sure Fupan get's a chance to review as well. |
I just added the |
|
@dgibson - thanks for this PR. I was looking to use I ended up just writing my own as a result. Kudos for digging a bit more and seeing the function doesn't appear necessary. This LGTM. @lifupan WDYT? |
dgibson
force-pushed
the
simplify-mount-storage
branch
from
5efd883
to
4ac4b10
Compare
|
/test |
dgibson
force-pushed
the
simplify-mount-storage
branch
from
bd2c1b1
to
9c59629
Compare
It is a bug fix, if it's ever possible to get to this path with a bind mount of a directory. I suspect not, since a directory volume would be a virtiofs mount instead, but I'm not certain. In any case, what would I need to do for such a backport. |
Once this one gets merged, please, open another PR targetting the |
|
/test |
dgibson
force-pushed
the
simplify-mount-storage
branch
5 times, most recently
from
86d0b3c
to
bba0419
Compare
|
Setting do-not-merge label temporarily while I debug some weird failures on the static checks. |
dgibson
force-pushed
the
simplify-mount-storage
branch
3 times, most recently
from
9b06480
to
86d0b3c
Compare
|
Bother, found the problem. Fixing a lint name which corrects a clippy failure run locally appears to break the CI, I think because it's tripping Rust bug rust-lang/rust-clippy#7422 |
dgibson
force-pushed
the
simplify-mount-storage
branch
from
86d0b3c
to
fa4f036
Compare
|
Rebased on #2634 to address the bogus clippy warning issues. |
BareMount::mount does some complicated marshalling and uses unsafe code to call into the mount(2) system call. However, we're already using the nix crate which provides a more Rust-like wrapper for mount(2). We're even already using nix::mount::umount and nix::mount::MsFlags from the same module. In the same way, we can replace the direct usage of libc::umount() with nix::mount::umount() in one of the tests. Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
struct Baremount contains the information necessary to make a new mount. As a datastructure, however, it's pointless, since every user just constructs it, immediately calls the BareMount::mount() method then discards the structure. Simplify the code by making this a direct function call baremount(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
…ts() ensure_destination_exists() can create either a directory or a regular file depending on the arguments. This patch extracts the regular file specific option into its own helper: ensure_destination_file_exists(). This: - Avoids doing some steps in the directory case (they're already handled by create_dir_all()) - Enables some further future cleanups Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
mount_storage() first makes sure the mount point for the storage volume exists. It uses fs::create_dir_all() in the case of 9p or virtiofs volumes otherwise ensure_destination_exists(). But.. ensure_destination_exists() boils down to an fs::create_dir_all() in most cases anyway. The only case it doesn't is for a bind fstype, where it creates a file instead of a directory. But, that's not correct anyway because we need to create either a file or a directory depending on the source of the bind mount, which ensure_destination_exists() doesn't know. The 9p/virtiofs paths also check if the mountpoint exists before calling fs::create_dir_all(), which is unnecessary (fs::create_dir_all already handles that case). mount_storage() does have the information to know what we need to create, so have it explicitly call ensure_destination_file_exists() for the bind mount to a non-directory case, and fs::create_dir_all() in all other cases. fixes kata-containers#2390 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The only remaining callers of ensure_destination_exists() are in its own unit tests. So, just remove it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
dgibson
force-pushed
the
simplify-mount-storage
branch
from
fa4f036
to
9d3cd98
Compare
|
/test |
|
/retest-metrics |
|
/retest-ubuntu-metrics |
mount_storage() first makes sure the mount point for the storage volume
exists. It uses fs::create_dir_all() in the case of 9p or virtiofs volumes
otherwise ensure_destination_exists(). But.. ensure_destination_exists()
boils down to an fs::create_dir_all() in most cases anyway. The only case
it doesn't is for a bind fstype, where it creates a file instead of a
directory. But..
need to create either a file or a directory depending on the source of
the bind mount, which ensure_destination_exists() doesn't know about.
Furthermore the 9p/virtiofs path checks if the mountpoint exists before
calling fs::create_dir_all(), which is unnecessary, because create_dir_all
has an equivalent test built in.
Signed-off-by: David Gibson david@gibson.dropbear.id.au