Implement CreateRuntime, Poststop hooks #46

akhramov · 2023-08-10T18:53:34Z

Issue number:

Description of changes:

For CNI Networking, nerdctl sets up CreateRuntime and Poststop hooks. However runj does not support hooks at all.

This change

adds preliminary support for hooks
temporary hardcodes jails to be vnet. CNI plugins work with vnet jails only

Testing done:

containerd/nerdctl/CNI plugins manual integration testing

Terms of contribution:

By submitting this pull request, I agree that this contribution is licensed under the terms found in the LICENSE file.

akhramov · 2023-08-10T18:56:24Z

jail/conf.go

@@ -31,6 +31,7 @@ const (
  vnet.interface = "{{ join .VNetInterface ", " }}";
 {{- end }}
  persist;
+  vnet; // FIXME: this must not be hardcoded


sic! need to figure out how to avoid hardcoding.

In ocijail, I'm using annotations to let the container engine communicate what it needs to the OCI runtime. This should really be part of a FreeBSD extension to the OCI runtime spec but we don't have that yet.

Lines 27-29 already have this handled as part of the existing extensions. Can you use that?

The original idea was to have something that works with nerdctl from the start, so I hardcoded it just for the sake of illustration. I guess we're going to standardize the extensions, so I fixed that, thanks!

nerdctl can also use the extensions until they've been standardized. That's what I've done with my ctr demos so far.

dfr · 2023-08-12T12:51:38Z

My CNI plugins are written to assume that they have access to the host's network utilities such as ifconfig and route. This is partly why I use a separately managed network jail as parent for the OCI jail. We cannot assume that these utilities exist inside the container image and we should not trust them even if they are present.

In theory, it should be possible to use AF_NETLINK instead of ifconfig which would involve getting a netlink socket for the jail's vnet, e.g. by forking, attaching to the jail and sending a netlink socket fd back via a control message. Probably also would need to add freebsd support to github.com/vishvananda/netlink which is a fair bit of work. This approach would avoid the second network jail for simple containers.

igalic · 2023-08-12T12:58:55Z

sadly, the go runtime doesn't support netlink on FreeBSD yet

ref, golang/go#59865

dfr · 2023-08-12T14:03:11Z

sadly, the go runtime doesn't support netlink on FreeBSD yet

ref, golang/go#59865

I forgot about that part - the go runtime probably won't move forward to 13.2+ era until stable/12 reaches end of life.

igalic · 2023-08-12T14:35:45Z

it especially won't do so, unless someone from FreeBSD submits patches.
I still need to get around to doing that for VirtIO Sockets… as soon as I'm done implementing the driver for FreeBSD.

akhramov · 2023-08-12T15:58:28Z

in theory, it should be possible to use AF_NETLINK instead of ifconfig

Inside the jail can't we just use ioctls that back ifconfig's functionality (SIOCIFCREATE, SIOCIFDESTROY and friends)?

For route I believe we could have used a PF_ROUTE socket.

I did that in my budget version of "CNI". (interface manipulations: https://github.com/akhramov/knast/blob/8c9f5f481a467a22c7cc47f11a28fe52536f9950/netzwerk/src/interface/operations.rs), (route: https://github.com/akhramov/knast/blob/8c9f5f481a467a22c7cc47f11a28fe52536f9950/netzwerk/src/route/bindings.rs)

I can imagine that in go it may be way harder, though.

dfr · 2023-08-12T16:04:05Z

That would also work but as you say, it might be messy in go. Also, since attaching to the jail is one way, it will need some rexec magic or similar to create a child process to enter the jail and perform the actions.

dfr · 2023-08-13T08:35:37Z

One further option which is available in 14-current is that both ifconfig and route support a new '-j' option to perform the operations withing the specified jail. Looking at the commit logs, both changes wre intended to be merged to stable/13 but I don't think this has happened yet. This would be the lowest cost option for managing network state without needing two jails.

igalic · 2023-08-13T09:08:00Z

if those options haven't been merged to stable/13 yet, we should prod somebody.
But i reckon everyone is a bit too excited for / busy with 14, to remember MFCs

dfr · 2023-08-13T10:17:16Z

I commented on https://reviews.freebsd.org/D40213 to suggest the MFC. For network stats in podman and cri-o, I would also need a corresponding change to netstat but that should be straightforward.

dfr · 2023-08-14T13:54:07Z

I spent some time working on the plugins today and the 'ifconfig -j ' approach is viable - we can use a single jail with vnet=new for the container and the host's ifconfig and route utilities can safely initialise it after the call to OCI create and before OCI start. I will merge the ifconfig and route changes to stable/13 later this week so this should work in 13.3 as well as 14.0.

gmshake · 2023-08-15T07:41:22Z

jail/conf.go

A bit off topic,
I have not read the source code fully, but I think maybe it is better to corp with cli interface rather than the conf, then it will be flexible to add / regenerate options when creating jails.

samuelkarp · 2023-08-19T23:33:18Z

I will merge the ifconfig and route changes to stable/13 later this week so this should work in 13.3 as well as 14.0.

We can move runj up to either of those releases. Since runj is still experimental, I don't have a problem targeting only recent versions of FreeBSD.

samuelkarp

Thanks for writing this! It looks pretty good, just a few comments. It would also be really helpful to write some integration tests for this, demonstrating that hooks are invoked at the proper time and in the proper context.

runtimespec/version.go

hook/hook.go

samuelkarp · 2023-08-19T23:37:39Z

jail/conf.go

@@ -31,6 +31,7 @@ const (
  vnet.interface = "{{ join .VNetInterface ", " }}";
 {{- end }}
  persist;
+  vnet; // FIXME: this must not be hardcoded


Lines 27-29 already have this handled as part of the existing extensions. Can you use that?

samuelkarp · 2023-08-21T00:00:57Z

samuelkarp force-pushed the feature/CNI-jails branch from 6fd0dcd to ebe2b6e

Rebased to try and make CI pass.

For CNI Networking, nerdctl sets up CreateRuntime and Poststop hooks. However runj does not support hooks at all. This change - adds preliminary support for hooks Signed-off-by: Artem Khramov <[email protected]>

Signed-off-by: Samuel Karp <[email protected]>

samuelkarp · 2023-08-21T06:57:21Z

I've fixed a few issues with the tests (the whole setup of the integration tests is fairly hacky and not super resilient, nothing wrong with your code) and added a missing nil check for hooks. That makes the tests pass, but there's a bit of cruft left over since runExitingJail isn't really expecting runj create or runj delete to fail (and TestHookTimeout intentionally fails):

=== RUN   TestHookTimeout
    integ_hooks_test.go:61: bundle /tmp/runj-integ-test-TestHookTimeout-integ-test-hooks1095528594
    integ_main_test.go:190: preserving tempdir due to error /tmp/runj-integ-test-TestHookTimeout-integ-test-hooks1095528594 exit status 255

I'm going to merge this anyway, since the functionality is good and there are actual tests. If you want to take a crack at fixing runExitingJail that'd be welcome, otherwise I'll just open an issue to track it.

akhramov mentioned this pull request Aug 10, 2023

FreeBSD: CNI plugins containerd/nerdctl#2429

Closed

akhramov commented Aug 10, 2023

View reviewed changes

gmshake reviewed Aug 15, 2023

View reviewed changes

samuelkarp reviewed Aug 19, 2023

View reviewed changes

samuelkarp mentioned this pull request Aug 19, 2023

Add support for hooks #47

Open

akhramov force-pushed the feature/CNI-jails branch from f9584d9 to d38188f Compare August 20, 2023 16:40

akhramov marked this pull request as ready for review August 20, 2023 16:45

akhramov force-pushed the feature/CNI-jails branch 3 times, most recently from a00577b to 6fd0dcd Compare August 20, 2023 19:01

samuelkarp force-pushed the feature/CNI-jails branch from 6fd0dcd to ebe2b6e Compare August 20, 2023 23:59

akhramov and others added 2 commits August 20, 2023 18:26

Implement CreateRuntime, Poststop hooks

ff4ba3d

For CNI Networking, nerdctl sets up CreateRuntime and Poststop hooks. However runj does not support hooks at all. This change - adds preliminary support for hooks Signed-off-by: Artem Khramov <[email protected]>

hooks: guard for nil hooks

d4ac22c

Signed-off-by: Samuel Karp <[email protected]>

samuelkarp force-pushed the feature/CNI-jails branch from ebe2b6e to d4ac22c Compare August 21, 2023 06:49

samuelkarp approved these changes Aug 21, 2023

View reviewed changes

samuelkarp merged commit d4ac22c into samuelkarp:main Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CreateRuntime, Poststop hooks #46

Implement CreateRuntime, Poststop hooks #46

akhramov commented Aug 10, 2023

akhramov Aug 10, 2023

dfr Aug 15, 2023

samuelkarp Aug 19, 2023

akhramov Aug 20, 2023

samuelkarp Aug 20, 2023

dfr commented Aug 12, 2023

igalic commented Aug 12, 2023 •

edited

Loading

dfr commented Aug 12, 2023

igalic commented Aug 12, 2023

akhramov commented Aug 12, 2023

dfr commented Aug 12, 2023

dfr commented Aug 13, 2023

igalic commented Aug 13, 2023

dfr commented Aug 13, 2023

dfr commented Aug 14, 2023

gmshake Aug 15, 2023

samuelkarp commented Aug 19, 2023

samuelkarp left a comment

samuelkarp Aug 19, 2023

samuelkarp commented Aug 21, 2023

samuelkarp commented Aug 21, 2023

Implement CreateRuntime, Poststop hooks #46

Implement CreateRuntime, Poststop hooks #46

Conversation

akhramov commented Aug 10, 2023

akhramov Aug 10, 2023

Choose a reason for hiding this comment

dfr Aug 15, 2023

Choose a reason for hiding this comment

samuelkarp Aug 19, 2023

Choose a reason for hiding this comment

akhramov Aug 20, 2023

Choose a reason for hiding this comment

samuelkarp Aug 20, 2023

Choose a reason for hiding this comment

dfr commented Aug 12, 2023

igalic commented Aug 12, 2023 • edited Loading

dfr commented Aug 12, 2023

igalic commented Aug 12, 2023

akhramov commented Aug 12, 2023

dfr commented Aug 12, 2023

dfr commented Aug 13, 2023

igalic commented Aug 13, 2023

dfr commented Aug 13, 2023

dfr commented Aug 14, 2023

gmshake Aug 15, 2023

Choose a reason for hiding this comment

samuelkarp commented Aug 19, 2023

samuelkarp left a comment

Choose a reason for hiding this comment

samuelkarp Aug 19, 2023

Choose a reason for hiding this comment

samuelkarp commented Aug 21, 2023

samuelkarp commented Aug 21, 2023

igalic commented Aug 12, 2023 •

edited

Loading