agent: add support for guest-hooks by eguzman3 · Pull Request #365 · kata-containers/agent

eguzman3 · 2018-09-12T19:03:59Z

Adds support for running OCI hooks within the guest.
A 'drop-in' path (guest_hook_path) is specified in
the cli configuration file and if set, the agent will
look for OCI hooks in this directory and inject them
into the container life cycle.

Old Discussion: #346, #347
Runtime Change: kata-containers/runtime#720

Fixes #348

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

Blocked on (kata-containers/agent#365), will need to revendor.

grpc.go

protocols/grpc/utils.go

protocols/grpc/agent.proto

Adds support for running OCI hooks within the guest. A 'drop-in' path (guest_hook_path) is specified in the cli configuration file and if set, the agent will look for OCI hooks in this directory and inject them into the container life cycle. Fixes kata-containers#348 Signed-off-by: Edward Guzman <eguzman@nvidia.com>

Blocked on (kata-containers/agent#365), will need to revendor.

bergwolf

@eguzman3 Thanks for the updates. It looks mostly good. I only have one comment inline.

Another question, since you are relying on the rootfs layout, is there a matching osbuilder PR that allows you to build a rootfs with customized hooks? Or do you simply add the hooks by hand between the rootfs creation and image creation steps? I'm asking because we might want to test this infrastructure by adding a simple hook and making sure it executes in CI, so that we can make sure the special use case is not broken by some future change.

bergwolf · 2018-09-14T01:18:30Z

grpc.go

+	a.sandbox.guestHooksPresent = false
+
+	if req.GuestHookPath != "" {
+		a.sandbox.scanGuestHooks(req.GuestHookPath)


So it is silently ignored if the path doesn't exist or if there are no hooks in the hook path, which is mostly because a user have installed a wrong rootfs image. I wonder if you want to return failure here since otherwise users have no idea if the hooks are executed or not unless they somehow check it in their container. OTOH, if you return failure here, such errors can be found early when creating the sandbox.

Another question, since you are relying on the rootfs layout, is there a matching osbuilder PR that allows you to build a rootfs with customized hooks? Or do you simply add the hooks by hand between the rootfs creation and image creation steps?

Currently I am using scripts to add the hooks and other files needed for my use case between rootfs and image creation.

I wonder if you want to return failure here since otherwise users have no idea if the hooks are executed or not unless they somehow check it in their container.

What about the case where a user wants to swap between images (some with hooks, some without) without having to change the configuration option?

I agree with @bergwolf - if a user switches images, I'd say they should expect to have to update the config if one has hooks and the other doesn't.

As an absolute minimum, can you add a log call showing if the user requested hooks or not (if GuestHookPath is set or not we should log something).

I do wonder if we want to add either a second bool field in the proto file (bool require_hooks = n), or maybe even a kernel cmdline option for the agent to consume (the former is cleaner though):

https://github.com/kata-containers/agent/blob/master/config.go#L67

That way, the agent can clearly establish whether the user wants hooks or not allowing the value (and validity) of GuestHookPath to be more clearly determined.

I suggest we log this call for now. I was thinking if we would want to support using a different rootfs for different pods in the future similar to how we support different kernels for k8s @bergwolf @jodh-intel

@amshinde In that case, I think the GuestHookPath config should also be passed by spec (e.g., through labels or annotations).

@eguzman3 I would prefer a fail-early strategy to let users know that they have pointed to a wrong rootfs image. For your mentioned use case, right now users have to remove GuestHookPath if they switch to a rootfs image w/o hooks. In future we might support to specify all kata configs through labels or annotations, and then there is no dependency on a fixed cli configuration and each pod can be customized.

ref kata-containers/runtime#753

WeiZhang555

Overall it looks good,

This sounds like a good feature, it can be useful. Though I didn't reads all your previous discussions.

WeiZhang555 · 2018-09-17T04:02:43Z

protocols/grpc/utils.go

 }
+
+// ChangeToBundlePath changes the cwd to the bundle path defined in the OCI spec
+func ChangeToBundlePath(spec *specs.Spec) (string, error) {


These utils functions might be better to move out from grpc package.

You know, this package can be vendored later to kata-runtime, because kata-runtime will be client and communicate with kata-agent via GRPC protocol, they share same GRPC protocol definitions.

So if these functions do no help to runtime, which means it can't help kata-runtime do GRPC client operations, it might be better to move out from this package.

Done in the new PR :)

WeiZhang555 · 2018-09-17T04:04:08Z

protocols/grpc/utils.go

+	}
+
+	defer f.Close()
+	err = json.NewEncoder(f).Encode(&spec)


Question: spec is already a pointer, why need &spec here?

Good catch @WeiZhang555 :)

Done in the new PR

WeiZhang555 · 2018-09-17T04:08:12Z

protocols/grpc/utils.go

+
+// FindHooks searches guestHookPath for any OCI hooks for a given hookType
+func FindHooks(guestHookPath, hookType string) (hooksFound []specs.Hook) {
+	hooksPath := path.Join(guestHookPath, hookType)


So prestart hook will be in guestHookPath/prestart and poststart hook will be in guestHookPath/poststart,
and guestHookPath is specified by kata-runtime cli configuration, right?

I think we need some documents to teach user, we can't expect user can find this hidden advanced feature.

I agree, but I feel this should be documented in the configuration.toml template, so in the PR against kata-containers/runtime. Do you agree?

WeiZhang555 · 2018-09-17T06:36:04Z

This implementation looks good, but there's one missing part is that we can't specify arguments for each hook, I can't be sure that if there will be any requirements for this regarding to some unknown scenarios.

jodh-intel

Thanks @eguzman3 - This is getting close now! 😄 Just a few comments remaining. As they show, we do need to make sure it is really easy to determine whether:

hooks were enabled.
the agent found hooks in the requested location.
the hooks were valid.
the hooks were usable.
the hooks ran.

jodh-intel · 2018-09-17T07:26:35Z

protocols/grpc/utils.go

+
+// FindHooks searches guestHookPath for any OCI hooks for a given hookType
+func FindHooks(guestHookPath, hookType string) (hooksFound []specs.Hook) {
+	hooksPath := path.Join(guestHookPath, hookType)


jodh-intel · 2018-09-17T07:43:52Z

protocols/grpc/utils.go

+	return err
+}
+
+func isValidHook(file os.FileInfo) bool {


This is a nice clean function. However, I think it would be worth considering "polluting it" a bit with some log calls to help debug hook setup issues (basically a log call for every failure scenario).

jodh-intel · 2018-09-17T07:46:04Z

grpc.go

+	a.sandbox.guestHooksPresent = false
+
+	if req.GuestHookPath != "" {
+		a.sandbox.scanGuestHooks(req.GuestHookPath)


I agree with @bergwolf - if a user switches images, I'd say they should expect to have to update the config if one has hooks and the other doesn't.

As an absolute minimum, can you add a log call showing if the user requested hooks or not (if GuestHookPath is set or not we should log something).

I do wonder if we want to add either a second bool field in the proto file (bool require_hooks = n), or maybe even a kernel cmdline option for the agent to consume (the former is cleaner though):

https://github.com/kata-containers/agent/blob/master/config.go#L67

That way, the agent can clearly establish whether the user wants hooks or not allowing the value (and validity) of GuestHookPath to be more clearly determined.

jodh-intel · 2018-09-17T07:55:54Z

protocols/grpc/utils.go

+	}
+
+	defer f.Close()
+	err = json.NewEncoder(f).Encode(&spec)


Good catch @WeiZhang555 :)

jodh-intel

Thanks @eguzman3 - This is getting close now! 😄 Just a few comments remaining. As they show, we do need to make sure it is really easy to determine whether:

hooks were enabled.
the agent found hooks in the requested location.
the hooks were valid.
the hooks were usable.
the hooks ran.

jodh-intel · 2018-09-24T09:57:32Z

Hi @eguzman3 - do you think you'll get a chance to look over the final review comments some time this week? As mentioned, this is very close now I think 😄

jodh-intel · 2018-09-28T10:25:00Z

Ping @eguzman3 😄

flx42 · 2018-10-01T16:53:12Z

@jodh-intel I will take over the patch since Edward finished his internship. Sorry for the delay!

jodh-intel · 2018-10-01T17:00:20Z

Hi @flx42 - no problem! Do let us know if we can help in any way.

Adds support for running OCI hooks within the guest. A 'drop-in' path (guest_hook_path) is specified in the cli configuration file and if set, the agent will look for OCI hooks in this directory and inject them into the container life cycle. Fixes: kata-containers#348 Replaces: kata-containers#365 Co-authored-by: Edward Guzman <eguzman@nvidia.com> Co-authored-by: Felix Abecassis <fabecassis@nvidia.com> Signed-off-by: Edward Guzman <eguzman@nvidia.com> Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>

grahamwhaley · 2018-10-04T12:46:47Z

This one is superseded by #393 , so I'll mark it as DNM so we don't try to review or accidentally merge it ;-)

amshinde · 2018-10-08T21:50:09Z

Closing this PR, as it has been superseded by #393.

grahamwhaley · 2018-10-10T12:35:52Z

Nudging the CIs that failed. One is due to the nested glitch. The other had what look like maybe real errors (but anything doing apt could be an upstream timeout type error). I'll respin to see if that is reproducible or transient:

Summarizing 2 Failures:

[Fail] package manager update test check apt-get update [It] should not fail 
/tmp/jenkins/workspace/kata-containers-agent-ubuntu-16-04-PR-initrd/go/src/github.com/kata-containers/tests/integration/docker/package_manager_test.go:41

[Fail] CPUs and CPU set updating cpus and cpuset of a running container [It] should have the right number of vCPUs 
/tmp/jenkins/workspace/kata-containers-agent-ubuntu-16-04-PR-initrd/go/src/github.com/kata-containers/tests/integration/docker/cpu_test.go:432

Ran 202 of 214 Specs in 952.725 seconds
FAIL! -- 200 Passed | 2 Failed | 0 Pending | 12 Skipped --- FAIL: TestIntegration (979.86s)
FAIL

Adds support for running OCI hooks within the guest. A 'drop-in' path (guest_hook_path) is specified in the cli configuration file and if set, the agent will look for OCI hooks in this directory and inject them into the container life cycle. Fixes: kata-containers#348 Replaces: kata-containers#365 Co-authored-by: Edward Guzman <eguzman@nvidia.com> Co-authored-by: Felix Abecassis <fabecassis@nvidia.com> Signed-off-by: Edward Guzman <eguzman@nvidia.com> Signed-off-by: Felix Abecassis <fabecassis@nvidia.com>

eguzman3 added a commit to eguzman3/runtime that referenced this pull request Sep 12, 2018

cli: add guest hook path option

60e9fe4

Blocked on (kata-containers/agent#365), will need to revendor.

eguzman3 mentioned this pull request Sep 12, 2018

cli: add guest hook path option kata-containers/runtime#720

Closed

bergwolf suggested changes Sep 13, 2018

View reviewed changes

grpc.go Outdated Show resolved Hide resolved

jodh-intel reviewed Sep 13, 2018

View reviewed changes

protocols/grpc/utils.go Outdated Show resolved Hide resolved

protocols/grpc/agent.proto Show resolved Hide resolved

eguzman3 force-pushed the guest-hooks branch from 621bbf8 to 289e877 Compare September 13, 2018 19:53

eguzman3 force-pushed the guest-hooks branch from 289e877 to 0a6b119 Compare September 13, 2018 20:21

eguzman3 added a commit to eguzman3/runtime that referenced this pull request Sep 13, 2018

cli: add guest hook path option

895fa27

Blocked on (kata-containers/agent#365), will need to revendor.

bergwolf reviewed Sep 14, 2018

View reviewed changes

WeiZhang555 reviewed Sep 17, 2018

View reviewed changes

jodh-intel reviewed Sep 17, 2018

View reviewed changes

jodh-intel mentioned this pull request Sep 17, 2018

virtcontainers: Pass seccomp profile inside VM kata-containers/runtime#689

Merged

bergwolf mentioned this pull request Sep 19, 2018

support config options through annotations kata-containers/runtime#753

Closed

flx42 mentioned this pull request Oct 2, 2018

agent: add support for guest-hooks (v2) #393

Merged

grahamwhaley added the do-not-merge label Oct 4, 2018

amshinde closed this Oct 8, 2018

flx42 mentioned this pull request Oct 17, 2018

cli: add guest hook path option (v2) kata-containers/runtime#834

Merged

jodh-intel mentioned this pull request Dec 3, 2018

cli: Agent as init in rootfs images kata-containers/runtime#768

Closed

jodh-intel mentioned this pull request Jun 11, 2020

Add guest hook tests kata-containers/tests#2632

Closed

Conversation

eguzman3 commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bergwolf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eguzman3 Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeiZhang555 commented Sep 17, 2018

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented Sep 24, 2018

Uh oh!

jodh-intel commented Sep 28, 2018

Uh oh!

flx42 commented Oct 1, 2018

Uh oh!

jodh-intel commented Oct 1, 2018

Uh oh!

grahamwhaley commented Oct 4, 2018

Uh oh!

amshinde commented Oct 8, 2018

Uh oh!

grahamwhaley commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

eguzman3 commented Sep 12, 2018 •

edited

Loading

eguzman3 Sep 14, 2018 •

edited

Loading

WeiZhang555 left a comment •

edited

Loading