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

Add volume mount support for job containers #1057

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Jul 1, 2021

This adds basic directory mount support for job containers. As any path on the host
is already accessible from the container, the concept of volume mounts is a bit funny
for job containers. However, it still makes sense to treat the volume mount point where
the container image is mounted as where most things should be found regarding the container.

The manner in which this is done is by appending the container mount path for the volume to
where the rootfs volume is mounted on the host and then symlinking it.

So:
Container rootfs volume path = "C:\C\123456789abcdefgh"

Example #1

{
"host_path": "C:\mydir"
"container_path": "\dir\in\container"
}

"C:\mydir" would be symlinked to "C:\C\123456789abcdefgh\dir\in\container"

Example #2

Drive letters will be stripped
{
"host_path": "C:\mydir"
"container_path": "C:\dir\in\container"
}
"C:\mydir" would be symlinked to "C:\C\123456789abcdefgh\dir\in\container"

Signed-off-by: Daniel Canter dcanter@microsoft.com

@dcantah dcantah requested a review from a team as a code owner July 1, 2021 13:31
@dcantah
Copy link
Contributor Author

dcantah commented Jul 1, 2021

Need to add a test here, will do shortly

@dcantah
Copy link
Contributor Author

dcantah commented Jul 1, 2021

@jsturtevant fyi

@perithompson
Copy link

I have added the necessary PR in k/k to allow this code to work @jsturtevant @dcantah PTAL kubernetes/kubernetes#103434

@dcantah dcantah force-pushed the jobcontainer-volume branch 2 times, most recently from 35c9d7c to 260ea4f Compare July 6, 2021 22:00
@dcantah dcantah force-pushed the jobcontainer-volume branch from 260ea4f to 56e057f Compare July 9, 2021 00:29
@dcantah
Copy link
Contributor Author

dcantah commented Jul 9, 2021

@anmaxvl Removed the k8s workaround mentioned in the last round as it's no longer needed

This adds basic directory mount support for job containers. As any path on the host
is already accessible from the container, the concept of volume mounts is a bit funny
for job containers. However, it still makes sense to treat the volume mount point where
the container image is mounted as where most things should be found regarding the container.

The manner in which this is done is by appending the container mount path for the volume to
where the rootfs volume is mounted on the host and then symlinking it.

So:
Container rootfs volume path = "C:\C\123456789abcdefgh\"

Example #1
--------------
{
    "host_path": "C:\mydir"
    "container_path": "\dir\in\container"
}

"C:\mydir" would be symlinked to "C:\C\123456789abcdefgh\dir\in\container"

Example #2
---------------
Drive letters will be stripped
{
    "host_path": "C:\mydir"
    "container_path": "C:\dir\in\container"
}
"C:\mydir" would be symlinked to "C:\C\123456789abcdefgh\dir\in\container"

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah dcantah force-pushed the jobcontainer-volume branch from 56e057f to 90a193c Compare July 9, 2021 22:52
@dcantah
Copy link
Contributor Author

dcantah commented Jul 9, 2021

@katiewasnothere updated, ptal

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

lgtm

return errors.Wrap(err, "failed to make directory for job container mount")
}

if err := os.Symlink(mount.Source, fullCtrPath); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an expectation based on standard container mount behavior that a mount over an existing path will mask the existing path? os.Symlink will fail if the target exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct yea. At the bindflt level I'd assume there's nothing stopping a shadowing of whatever was there, but I'm not sure if hcs has any logic to disallow this. Let me give it a whirl, that's a good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msscotb Yea the behavior on Windows (and Linux but I knew this part) is to mask whatever is there. I don't know how we could mimic this without bindflt :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msscotb Was this a blocking comment or just a "curious what happens in a normal container scenario"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msscotb ping on this

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a better option since binding as we do for standard containers is not possible with the process isolated design current.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants