handle container deps removal with postsop hooks

[fahedouch]

#Before
the container deps ( etchosts files, names, container state) removal is deferred by RemoveContainer which means container deletion happens before deps deletion.

#After
Using runtime-spec postStop Hook, we ensure that container deps deletion is successfully completed before the container deletion returns

Signed-off-by: fahed dorgaa fahed.dorgaa@gmail.com

[fahedouch]

I still have work to do here:

Once the runtime deps ( etchosts files, names, container state...) removal is handled by postStop oci hook ( see ), with the same logic the runtime deps ( etchosts files, names, container state...) creation should be handled by the oci hook too (createRuntime hook). Actually, it is cobra cmd that handles this creation ( e.g container names management) which is not a good design in my opinion; runtime deps creation/removal should be close to the runtime level, then the runtime event hooks (createRuntime / postStop) seems to be the best place to handle these actions.

This will result :

• task.NewTask() => trigger createRuntime hooks => create runtime deps ( etchosts , names, container states etc ..)
• task.DeleteTask() => trigger poststop hooks => delete runtime deps

WDYT @containerd/nerdctl-maintainers

[djdongjin]

@fahedouch SGTM! as long as runtime/oci provides such capabilities we should utilize them other than handling in nerdctl. Just a few questions (this is new to me :)):

1. Is there any behavior changes? (e.g. new error-returnning logic/scenario).
2. How many container deps we can change to utilize the ocihook (you mentioned etchosts files, names, container state)? Mostly, any specific dep that is still need to be managed by nerdctl?
3. you mentioned container name management. Does that mean if we change to use oci hook, we can elimate the use of NameStore ?
4. Does this impact which versions of containerd/runc that nerdctl supports?

Thanks

[manugupt1]

I have a basic question. Does taskStart and task.kill call ocihooks?
Can a container have more than 1 task? What happens then for let's say /etc/hosts file?

[fahedouch]

@djdongjin

@fahedouch SGTM! as long as runtime/oci provides such capabilities we should utilize them other than handling in nerdctl. Just a few questions (this is new to me :)):

1. Is there any behavior changes? (e.g. new error-returnning logic/scenario).

I believe that returned error error is handled by runc through OCI hook specification, same thing as hoststore error management

2. How many container deps we can change to utilize the ocihook (you mentioned etchosts files, names, container state)? Mostly, any specific dep that is still need to be managed by nerdctl?

not all deps can be handled by the ocihook, such as anon volume remove as it depends on cli options. So the idea is to start moving theses deps under oci hook scope:

• etchosts files
• names
• container states
  And complete the rest laster ( different PRs)

3. you mentioned container name management. Does that mean if we change to use oci hook, we can elimate the use of NameStore ?
   NameStore interaction/management move under oci-hook => oci-hook will trigger NameStore interface

4. Does this impact which versions of containerd/runc that nerdctl supports?

No. containerd/runc out of technical scope. runtime-spec provide a decoupled architecture so we can implement the hook logic without impacting with runc itself. there is a good abstraction.

@manugupt1

I have a basic question. Does taskStart and task.kill call ocihooks?

task.New => trigger createRuntime hook (that need a double check)
task.Delete => trigger poststop

full list of hooks

Can a container have more than 1 task?

see

What happens then for let's say /etc/hosts file?

no changes for /etc/hosts because they are already handled by oci-hook

[fahedouch]

I have some blocker(s) to make oci hooks managing the container deps. I can't figure out what is creating hostsDirBasename ( /var/lib/nerdctl/<ADDRHASH>/etchosts).
Theoretically is done by the [onCreateRuntime] (https://github.com/containerd/nerdctl/blob/main/pkg/ocihook/ocihook.go#L378) hook, but by disabling the onCreateRuntime hook the hostsDirBasename is always create. It seems like an other thing is creating/controlling the hostsDirBasename 🧐

[fahedouch]

I have some blocker(s) to make oci hooks managing the container deps. I can't figure out what is creating hostsDirBasename ( /var/lib/nerdctl/<ADDRHASH>/etchosts). Theoretically is done by the [onCreateRuntime] (https://github.com/containerd/nerdctl/blob/main/pkg/ocihook/ocihook.go#L378) hook, but by disabling the onCreateRuntime hook the hostsDirBasename is always create. It seems like an other thing is creating/controlling the hostsDirBasename 🧐

found my answer, I was targeting the wrong path :p , what I am looking for is the creation of hostsDir and not hostsDirBasename which is done by the container_run_network.go. I have to move creation login under the onCreateRuntime hook

[fahedouch]

Unfortunately, I could not totally acheive my goal using runtime-spec oci hooks, for the simple reason that all hooks are triggered after the runtime environment has been created (according to the spec configuration in config.json):

So these deps cannot be handled by runtime-spec oci hooks:

• etc/hosts files is mount-bind file handled by the config.json within runtime creation => out of oci-hooks scope

• container states are created by container and not containerd tasks => out of oci-hooks scope

But container names is now handled by the runtime-spec oci hooks.

[manugupt1]

Hi @fahedouch; I am still building my understanding.
Were you able to verify this part:

1. task.New => trigger createRuntime hook (that need a double check)

I am also curious what happens when nerdctl run -d <...> sleep infinity and exec is called together in it. I am guessing it works because the tests are passing.

[fahedouch]

Hi @fahedouch; I am still building my understanding. Were you able to verify this part:

1. task.New => trigger createRuntime hook (that need a double check)

I am also curious what happens when nerdctl run -d <...> sleep infinity and exec is called together in it. I am guessing it works because the tests are passing.

Hi @manugupt1 , you can verify this by checking the /var/lib/nerdctl/1935db59/ content when task.New() is execute. You can achieve this using some breakpoints

task.New() => containerd create a task => runc create => trigger createRuntime hook

[manugupt1]

Thank you @fahedouch for this PR; I learnt quite a bit.

[manugupt1]

thanks!

[fahedouch]

cc @AkihiroSuda PTAL!

[AkihiroSuda]

This change is relatively huge, so let me release v1.2.1 before merging this
(waiting for moby/buildkit#3651 to be released)

[AkihiroSuda]

The name has to be reserved as soon as the container is created, so this code shouldn't be moved

[fahedouch]

@AkihiroSuda this code is for Releasing name and not reserving name. Why talking about reserving name here ?

[fahedouch]

ping @AkihiroSuda

[AkihiroSuda]

Sorry, my comment is for L690 if err := containerNameStore.Acquire(name, id); err != nil {

[fahedouch]

Fixed

[fahedouch]

The name has to be reserved as soon as the container is created, so this code shouldn't be moved

the above code shouldn't be removed too, as failed creation in this level need a cleanup too

[AkihiroSuda]

Please see comments

[fahedouch]

@AkihiroSuda PTAL !

[AkihiroSuda]

CI failing

[fahedouch]

this introduce double acquire , so names Acquire() from oci-hook should be removed

[fahedouch]

@AkihiroSuda PTAL again plz

[AkihiroSuda]

Thanks