Don't remove containerd's container after exit #47

rumpl · 2022-08-09T07:56:20Z

Keeping the containerd's container around while our container is active makes docker start possible.

This is not really ideal, the best way to go about this would be to create the containerd container on create but this would change too much of the logic. Once we switch completely to containerd we can come back to this and change the logic.

vvoland

I'm wondering, since we already used containerd for managing the containers runtime - why does it work in the old case when Delete is being performed?

thaJeztah · 2022-08-09T10:04:12Z

Will this work for stopped containers if the daemon and/or containerd and/or machine is restarted?

rumpl · 2022-08-09T10:28:22Z

I'm wondering, since we already used containerd for managing the containers runtime - why does it work in the old case when Delete is being performed?

The issue is actually that now, when we remove the containerd's container, the snapshot is also removed. I tried using leases so that the snapshot is not deleted but with no luck.

rumpl · 2022-08-09T10:38:58Z

Will this work for stopped containers if the daemon and/or containerd and/or machine is restarted?

We still lose all the containers when the daemon restarts. My guess is that it's another issue, maybe the containers aren't restored as they should, we should take a look at this in a followup

rumpl · 2022-08-09T10:41:25Z

Will this work for stopped containers if the daemon and/or containerd and/or machine is restarted?

Yeah ok so we will need to take a closer look at the restore logic I think, it reads a directory and loops over it to restore the containers.

thaJeztah · 2022-08-09T11:33:33Z

when we remove the containerd's container, the snapshot is also removed. I tried using leases so that the snapshot is not deleted but with no luck.

Ah, hm.. gotcha; yes I'd also be thinking of leases for that. Removing the snapshots definitely would be a problem for "pet containers".

Wondering if this is what's nerdctl is using for that (I see it sets a label; could that label be to prevent it being garbage collected?); https://github.com/containerd/nerdctl/blob/31065c3e52f924d34bc9df370eef6b091a3e31ba/cmd/nerdctl/stop.go#L96-L99

thaJeztah · 2022-08-09T11:33:54Z

oh, never mind; looks like that was added to store additional settings I think; containerd/nerdctl@97ece6a

vvoland · 2022-08-09T11:38:24Z

#49 I drafted a quick implementation of the leases.

Tested with:

docker container --name aaa create busybox echo test
docker start aaa
docker start aaa
docker rm aaa

works fine

thaJeztah · 2022-08-09T11:39:36Z

daemon/delete.go

+		if err := daemon.containerd.Delete(context.Background(), container.ID); err != nil {
+			logrus.Errorf("%s cleanup: failed to delete container from containerd: %v", container.ID, err)


Does this return an error if the container was already gone? If so, can we detect a errdefs.IsNotExist() ?

If we can't exclude "NotExist" errors, probably should be logged as a Warnf (because we handle the situation); perhaps using WithField() and WithError()

logrus.WithError(err).WithField("container", container.ID).Error("cleanup: failed to delete container from containerd")

Does this require a daemon.containerd.DeleteTask as well? (not sure; but noticed that's done in some of the other code)

I used the same code we use in Cleanup https://github.com/moby/moby/blob/master/daemon/start.go#L264-L266

Added your suggestion for the log

If I add a check for IsNotExist, what should we do in the two cases?

thaJeztah · 2022-08-10T12:08:56Z

daemon/start.go

+				// best effort to clean up old container object
+				daemon.containerd.DeleteTask(context.Background(), container.ID)
+				if err := daemon.containerd.Delete(context.Background(), container.ID); err != nil && !errdefs.IsNotFound(err) {


Looking if this is correct; previously (before we used snapshotters), deleting the container from containerd didn't have side-effects (we had control over the container's filesystem, so deleting the container was just deleting the instance/config of the container)

However, now that we do use the snapshotters, would Delete() here also remove the container's filesystem?

Perhaps in case of IsConflict() and we're using snapshotters we should consider it "done" (container already exists; nothing to be done)

When using containerd snapshotters we won't try to create a container if it already exists, so we can't have any conflicts

Of course, unless someone tries to create a container with the same ID at the same time and we manage to somehow get in here...

Should we just return the conflict in that case? I realise it would be a rare condition (something else creating a container with the exact same ID). In general, getting a conflict in this situation would be unexpected, so not 100% sure we should "proceed as usual".

Okay, added a check and a comment to explain why we do it, thanks

thaJeztah · 2022-08-10T12:12:35Z

daemon/start.go

+		err = daemon.containerd.Create(ctx, container.ID, spec, shim, createOptions, newContainerOpts...)
 		if err != nil {
-			return translateContainerdStartErr(container.Path, container.SetExitCode, err)
+			if errdefs.IsConflict(err) {


I had to double-check if we could use our own errdefs here, but looks like containerd.Create() does conversion for this specific error;

moby/libcontainerd/remote/client.go

Lines 142 to 144 in 44626ea

if containerderrors.IsAlreadyExists(err) {

return errors.WithStack(errdefs.Conflict(errors.New("id already in use")))

}

We should indeed create a general utility for that though (making sure we convert all errors to our own?)

Keeping the containerd's container around while our container is active makes `docker start` possible. Signed-off-by: Djordje Lukic <[email protected]>

thaJeztah

LGTM

thaJeztah · 2022-09-01T10:36:26Z

Not upstreamed yet, but

may be redundant with Create a lease for the snapshots we prepare #62

Should be combined with:

thaJeztah · 2023-03-08T19:10:34Z

@rumpl this one still needed in upstream? Or superseded by the other PRs we did? (sorry, I'm lazy 😂)

rumpl · 2023-03-08T19:42:38Z

This one still needs to be (partially) upstreamed

rumpl · 2023-04-04T13:50:10Z

Okay I just checked and this one is good now

rumpl requested review from ndeloof, thaJeztah and vvoland August 9, 2022 08:40

vvoland reviewed Aug 9, 2022

View reviewed changes

rumpl requested a review from vvoland August 9, 2022 11:01

thaJeztah reviewed Aug 9, 2022

View reviewed changes

rumpl force-pushed the feat-start branch from 7523b18 to 92b8652 Compare August 10, 2022 10:44

thaJeztah reviewed Aug 10, 2022

View reviewed changes

rumpl requested a review from thaJeztah August 10, 2022 12:09

thaJeztah reviewed Aug 10, 2022

View reviewed changes

Don't remove containerd's container after exit

e89f6e6

Keeping the containerd's container around while our container is active makes `docker start` possible. Signed-off-by: Djordje Lukic <[email protected]>

rumpl force-pushed the feat-start branch from 92b8652 to e89f6e6 Compare August 10, 2022 12:42

rumpl requested a review from thaJeztah August 10, 2022 12:45

ndeloof approved these changes Aug 10, 2022

View reviewed changes

thaJeztah approved these changes Aug 10, 2022

View reviewed changes

rumpl merged commit 4551c32 into c8d Aug 10, 2022

rumpl added the to-be-upstreamed label Aug 30, 2022

This was referenced Sep 1, 2022

Create a lease for the snapshots we prepare #62

Merged

[do not merge, might explode] moby/moby#43902

Closed

This was referenced Sep 1, 2022

daemon: containerStart(): optimize logic #72

Merged

daemon: containerStart() keep container's snapshotter #71

Merged

rumpl deleted the feat-start branch November 22, 2022 00:15

rumpl added upstreamed and removed to-be-upstreamed labels Apr 4, 2023

		if err := daemon.containerd.Delete(context.Background(), container.ID); err != nil {
		logrus.Errorf("%s cleanup: failed to delete container from containerd: %v", container.ID, err)

	if containerderrors.IsAlreadyExists(err) {
	return errors.WithStack(errdefs.Conflict(errors.New("id already in use")))
	}

Don't remove containerd's container after exit #47

Don't remove containerd's container after exit #47

Uh oh!

Conversation

rumpl commented Aug 9, 2022

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 9, 2022

Uh oh!

rumpl commented Aug 9, 2022

Uh oh!

rumpl commented Aug 9, 2022

Uh oh!

rumpl commented Aug 9, 2022

Uh oh!

thaJeztah commented Aug 9, 2022

Uh oh!

thaJeztah commented Aug 9, 2022

Uh oh!

vvoland commented Aug 9, 2022

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!

rumpl Aug 10, 2022 • 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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Mar 8, 2023

Uh oh!

rumpl commented Mar 8, 2023

Uh oh!

rumpl commented Apr 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rumpl Aug 10, 2022 •

edited

Loading

thaJeztah commented Sep 1, 2022 •

edited

Loading