Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

agent: add support for custom OCI hooks inside guest#347

Closed
eguzman3 wants to merge 1 commit intokata-containers:masterfrom
eguzman3:add-custom-hooks
Closed

agent: add support for custom OCI hooks inside guest#347
eguzman3 wants to merge 1 commit intokata-containers:masterfrom
eguzman3:add-custom-hooks

Conversation

@eguzman3
Copy link
Copy Markdown
Contributor

In order to allow OCI hooks to be executed during the lifecycle of the container inside the guest the path "/usr/share/oci/hooks/" has been designated to allow for 'drop-in' hooks.

Signed-off-by: Edward Guzman eguzman@nvidia.com

@eguzman3 eguzman3 force-pushed the add-custom-hooks branch 2 times, most recently from 14ac102 to af39f5e Compare August 30, 2018 20:25
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 30, 2018

Codecov Report

Merging #347 into master will decrease coverage by 3.16%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
- Coverage   47.21%   44.05%   -3.17%     
==========================================
  Files          15       15              
  Lines        2442     2454      +12     
==========================================
- Hits         1153     1081      -72     
- Misses       1140     1228      +88     
+ Partials      149      145       -4

@caoruidong
Copy link
Copy Markdown
Member

Who will put the hooks into /usr/share/oci/hooks/ ?

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Sep 4, 2018

@eguzman3 Same question as @caoruidong. Can you explain in your commit how these hooks are placed in the said location as well as the use case you are trying to solve that makes it necessary to run hooks in the guest.
Will the hooks be passed to the guest(9p mounted) or do you assume the hooks to be present on the container image?

@eguzman3
Copy link
Copy Markdown
Contributor Author

eguzman3 commented Sep 5, 2018

@caoruidong @amshinde These hooks are intended to be executed on the guest and are separate from those executed on the host. I am assuming the hooks will be added to that directory at the time of rootfs image creation. The use case for this is trying to leverage this existing OCI hook: https://github.com/NVIDIA/nvidia-container-runtime

@flx42
Copy link
Copy Markdown
Contributor

flx42 commented Sep 5, 2018

Just chiming in to point out that this drop-in hook mechanism was inspired by what Project Atomic's fork of Docker is doing:
https://github.com/projectatomic/docker/tree/docker-1.13.1-rhel#add-dockerhooks-exec-custom-hooks-for-prestartpoststop-containerspatch
And also libpod (for CRI-O):
https://github.com/containers/libpod/blob/v0.8.5/pkg/hooks/docs/oci-hooks.5.md

As @eguzman3 said, we want to ship one or multiple pre-start hooks in our guest rootfs.

@bergwolf
Copy link
Copy Markdown
Member

@eguzman3 Could you combine your two PRs into one? I see that they already share some common change. They are in fact adding the same functionality to the agent. There is no need to separate them. Within one PR, you can still have multiple commits.

return nil
}

func finishCreateContainer(a *agentGRPC, ctr *container, req *pb.CreateContainerRequest, config *configs.Config) (resp *gpb.Empty, err error) {
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.

The same function is also added in #346. Please try to combine your two PRs and apply the two commits one on top of the other.

grpc.go Outdated
NoPivotRoot: a.sandbox.noPivotRoot,
})
// Add any custom OCI hooks to the spec
err = addHooks(ociSpec)
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.

The hooks are static afaics. So it is more efficient to just scan once during agent startup, and do the OCI spec/hook dance only when there are real hooks to add.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants