cli: add guest hook path option (v2) #834
Conversation
Gopkg.toml
Outdated
| @@ -24,7 +24,7 @@ | |||
|
|
|||
| [[constraint]] | |||
| name = "github.com/opencontainers/runtime-spec" | |||
| revision = "4e3b9264a330d094b0386c3703c5f379119711e8" | |||
| revision = "5806c35637336642129d03657419829569abc5aa" | |||
There was a problem hiding this comment.
In the introduced spec changes, I don't see any hooks related change. Why do you need to update runtime-spec?
There was a problem hiding this comment.
Because of this: https://github.com/kata-containers/agent/blob/master/Gopkg.toml#L19
$ sed -i 's/7e8e20b10b71fe3044a24175b8a686421e9d2c24/03f040f14dceb858f6411db275fcac77f9f42a8d/' Gopkg.toml
$ dep ensure -update github.com/kata-containers/agent
Solving failure: No versions of github.com/kata-containers/agent met constraints:
03f040f14dceb858f6411db275fcac77f9f42a8d: Could not introduce github.com/kata-containers/agent@03f040f14dceb858f6411db275fcac77f9f42a8d, as it has a dependency on github.com/opencontainers/runtime-spec with constraint 5806c35637336642129d03657419829569abc5aa, which has no overlap with existing constraint 4e3b9264a330d094b0386c3703c5f379119711e8 from (root)
[...]
Makefile
Outdated
| @@ -149,6 +149,7 @@ DEFENABLEDEBUG := false | |||
| DEFDISABLENESTINGCHECKS := false | |||
| DEFMSIZE9P := 8192 | |||
| DEFHOTPLUGVFIOONROOTBUS := false | |||
| DEFGUESTHOOKPATH := | |||
There was a problem hiding this comment.
If the default value is empty, please drop DEFGUESTHOOKPATH.
There was a problem hiding this comment.
I will use /usr/share/oci/hooks, if that's fine for you. The option is commented by default anyway.
There was a problem hiding this comment.
OK, I missed the part where it's used for generating default values in a Go file.
To untangle the two I can remove the template from the toml file and put /usr/share/oci/hooks, and drop this variable from the Makefile. Is that what you prefer?
There was a problem hiding this comment.
Updated my PR with what I mentioned above, in case my description was not clear. Let me know if I should change it back.
cli/config/configuration.toml.in
Outdated
| @@ -167,6 +167,13 @@ enable_iothreads = @DEFENABLEIOTHREADS@ | |||
| # all practical purposes. | |||
| #entropy_source= "@DEFENTROPYSOURCE@" | |||
|
|
|||
| # If set to an absolute path within the guest rootfs, the agent will search | |||
| # this directory for OCI hooks and add them to the guest container's lifecycle. | |||
There was a problem hiding this comment.
Please emphasize the fact that the hooks are executed in the guest rather than in the host.
There was a problem hiding this comment.
You could also add a comment along the lines of:
This is an advanced option. Since OCI hooks are always run on the host, most users can leave this variable unset.
There was a problem hiding this comment.
Since OCI hooks are always run on the host
Did you mean "guest"? Or are you referring to OCI hooks from the original OCI runtime spec?
|
For the re-vendoring, please can you list all the commits pulled in to those vendor packages using |
cli/config/configuration.toml.in
Outdated
| @@ -167,6 +167,13 @@ enable_iothreads = @DEFENABLEIOTHREADS@ | |||
| # all practical purposes. | |||
| #entropy_source= "@DEFENTROPYSOURCE@" | |||
|
|
|||
| # If set to an absolute path within the guest rootfs, the agent will search | |||
| # this directory for OCI hooks and add them to the guest container's lifecycle. | |||
There was a problem hiding this comment.
You could also add a comment along the lines of:
This is an advanced option. Since OCI hooks are always run on the host, most users can leave this variable unset.
Given that I need to update |
|
@flx42 @jodh-intel any updates? Thx. |
|
@raravena80 I'm waiting for more feedback. I don't think I need to change anything else, unless I missed something. |
cli/config/configuration.toml.in
Outdated
| # of the guest. See the official documentation: | ||
| # https://github.com/opencontainers/runtime-spec/blob/v1.0.1/config.md#posix-platform-hooks | ||
| # Hooks must be stored in a subdirectory of guest_hook_path named after the | ||
| # hook type, e.g. prestart hooks must be in "guest_hook_path/prestart/" |
There was a problem hiding this comment.
A few points that I think need to be clarified in this comment:
- When you say, "Path to OCI hook binaries in the guest rootfs" do you mean that this path must already exist in the (implicitly custom) image?
- If so, what happens if the path isn't valid in the configured image?
- How would I go about creating such a custom image (add a link to https://github.com/kata-containers/osbuilder).
- What order will hooks be called in?
- Do hooks have to be executable? If not, is this logged?
- If I create
guest_hook_path/prestart/foo/bar/baz.sh, will it be run? You use the term "search" above which might imply "recursive" unless you specify the precise behaviour :) - It would be worth noting what is / isn't logged (success, failure, hook name being executed) and where it gets logged.
- It probably wouldn't hurt to list all the sub-directories that will be checked for hooks (since there are only 3 of them).
- I still think the comment needs to be clearer in stating that this is entirely optional and that the "normal" set of OCI hooks will be run (on the host) regardless. It needs to be clearer that these are an "additional" set of hooks I think.
|
Hi @flx42 - thanks for updating the re-vendor commit. However, please can you add details to the main commit. Yes, our process does have a certain amount of redundancy (since there is a reference to the github issue), but after yesterday, I think it's clear we cannot necessarily assume full access to this site so we always add in full descriptions in the commit message as well as on the issue "just in case" :) |
|
Once these two quick-to-fix issues have been addressed, I think we'll be good to go on this PR. |
|
btw - please do ping us here as a simple re-push is silent and we often don't notice as a result ;) |
|
lgtm |
|
/test |
Codecov Report
@@ Coverage Diff @@
## master #834 +/- ##
==========================================
+ Coverage 65.71% 66.28% +0.57%
==========================================
Files 88 87 -1
Lines 10692 10591 -101
==========================================
- Hits 7026 7020 -6
+ Misses 2926 2866 -60
+ Partials 740 705 -35 |
|
Updated :) |
|
/test |
|
Thanks @flx42! lgtm A couple of CI builds failed due to timeouts so I've restarted them. |
|
Runtime-spec and agent are updated in another PR, so I think you can drop the "vendor" commit to resolv the conflicts. Another hint is that if you want to replace your previous PR, you can just amend your original commits and Thanks for doing this feature! 😄 LGTM |
The previous PR wasn't from me :) |
Add support for specifying an optional drop-in path for guest OCI hooks. This is the runtime side for leveraging the agent change introduced in kata-containers/agent@980023e Fixes: kata-containers#720 Co-authored-by: Edward Guzman <[email protected]> Co-authored-by: Felix Abecassis <[email protected]> Signed-off-by: Felix Abecassis <[email protected]>
|
Rebased on |
|
/test |
|
Merging. Thanks for your nice patch! @flx42 |
The agent needs to update device entries in the OCI spec so that it has the correct major:minor numbers for the guest, which may differ from the host. Entries in the main device list are looked up by device path, but entries in the device resources list are looked up by (host) major:minor. This is done one device at a time, updating as we go in updateSpecDeviceList(). But since the host and guest have different namespaces, one device might have the same major:minor as a different device on the host. In that case we could update one resource entry to the correct guest values, then mistakenly update it again because it now matches a different host device. To avoid this, rather than looking up and updating one by one, we make all the lookups in advance, creating a map from (host) device path to the indices in the spec where the device and resource entries can be found. Fixes: kata-containers#834 Signed-off-by: David Gibson <[email protected]>
Replaces #720
kata-containers/agent, in a separate commit.