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 job containers package #903

Merged
merged 1 commit into from
Mar 8, 2021
Merged

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Dec 5, 2020

  • Add JobContainer and JobProcess types as the two types to represent a job container
    and a process in a job container.
  • Add logic to find the executable being asked to run for a job container.
  • Logic to launch the container as specific user.
  • Logic to mount the containers scratch space on the host to a directory.

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

internal/jobcontainers/oci.go Outdated Show resolved Hide resolved
internal/jobcontainers/jobcontainer.go Outdated Show resolved Hide resolved
internal/jobcontainers/utils.go Outdated Show resolved Hide resolved
internal/jobcontainers/jobcontainer.go Show resolved Hide resolved
test/go.mod Outdated Show resolved Hide resolved
@dcantah dcantah force-pushed the jobcontainers branch 2 times, most recently from bf96e1f to 1878f1e Compare December 15, 2020 01:11
@dcantah dcantah force-pushed the jobcontainers branch 4 times, most recently from 7cf2907 to 2f7eb8b Compare December 23, 2020 02:44
internal/oci/uvm.go Outdated Show resolved Hide resolved
test/cri-containerd/main.go Outdated Show resolved Hide resolved
@dcantah dcantah force-pushed the jobcontainers branch 3 times, most recently from 26c51a4 to d7df52c Compare March 2, 2021 11:36
@kevpar
Copy link
Member

kevpar commented Mar 3, 2021

How does this integrate into the shim? Are things like the pod/task/exec types for job containers coming in a future PR?

@dcantah
Copy link
Contributor Author

dcantah commented Mar 3, 2021

@kevpar Yep right after this. The HCS exec/task types are re-used is how I have it now. If the annotation is set (or one can imagine if the OCI spec field is set later on) we just do jobcontainer.Create instead of hcsoci.CreateContainer. JobContainer implements cow.Container so you can just pass it along as is to all of the task/exec types.

@kevpar
Copy link
Member

kevpar commented Mar 3, 2021

@kevpar Yep right after this. The HCS exec/task types are re-used is how I have it now. If the annotation is set (or one can imagine if the OCI spec field is set later on) we just do jobcontainer.Create instead of hcsoci.CreateContainer. JobContainer implements cow.Container so you can just pass it along as is to all of the task/exec types.

IMO we should have that PR out as soon as it is ready, given the short timeline we have to get this feature in. Worst case some of the interface for calling into the job containers package would have to change, which doesn't seem like a big deal.

@dcantah
Copy link
Contributor Author

dcantah commented Mar 3, 2021

IMO we should have that PR out as soon as it is ready, given the short timeline we have to get this feature in. Worst case some of the interface for calling into the job containers package would have to change, which doesn't seem like a big deal.

It's ready, it just uses work in this PR. I can add just the commit with the shim work to a branch and PR that and rebase on this when it gets in

@kevpar
Copy link
Member

kevpar commented Mar 3, 2021

IMO we should have that PR out as soon as it is ready, given the short timeline we have to get this feature in. Worst case some of the interface for calling into the job containers package would have to change, which doesn't seem like a big deal.

It's ready, it just uses work in this PR. I can add just the commit with the shim work to a branch and PR that and rebase on this when it gets in

I don't think we need to block to get that PR out is my point :) If this PR is b1 -> master then you should be able to publish another PR that is b2 -> b1 and I think it will work properly.

@dcantah
Copy link
Contributor Author

dcantah commented Mar 3, 2021

IMO we should have that PR out as soon as it is ready, given the short timeline we have to get this feature in. Worst case some of the interface for calling into the job containers package would have to change, which doesn't seem like a big deal.

It's ready, it just uses work in this PR. I can add just the commit with the shim work to a branch and PR that and rebase on this when it gets in

I don't think we need to block to get that PR out is my point :) If this PR is b1 -> master then you should be able to publish another PR that is b2 -> b1 and I think it will work properly.

I thought this one wasn't far off is why but that didn't turn out as planned 😆, I'll push the shim PR.

@dcantah
Copy link
Contributor Author

dcantah commented Mar 3, 2021

@kevpar Didn't come to mind but as the branch is on my fork it's not as easy as that actually. I opened a PR against my fork dcantah#1 if we want to review it here and merge into this branch

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

* Add `JobContainer` and `JobProcess` types as the two types to represent a job container
and a process in a job container.
* Add logic to find the executable being asked to run for a job container.
* Logic to launch the container as specific user.
* Logic to mount the containers scratch space on the host to a directory.
* Small subset of tests added to jobobject package

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
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.

6 participants