Container remove (or create?) is not atomic and does break name unicity

[apostasie]

Description

It is possible to produce conditions where the name store ends-up with multiple references for the same name.

While the namestore implementation has been fixed to be atomic, the rest of the flow might not be

It is presumably hard to have a simple reproducer, but here is how I got there:

Loop through rm and run:

while true; do nerdctl rm -f foo; nerdctl run -ti --name foo debian echo lol;  done

Then try to kill it "at the right time". Best way is probably to concurrently make binaries to fuck with the execution flow (this is assuming your nerdctl is ran from the build location).

Suggesting to review rm and run flow and ensure we implement this atomically, or at least have some recovery mechanism.

Steps to reproduce the issue

Describe the results you received and expected

Not break name unicity.

What version of nerdctl are you using?

main

Are you using a variant of nerdctl? (e.g., Rancher Desktop)

None

Host information

No response

[apostasie]

Presumably some toctou issue in there.

[apostasie]

https://github.com/containerd/nerdctl/blob/main/pkg/cmd/container/remove.go#L139

🤦‍♂️

So, I want a brownie point for having figured this out already a year ago.
Also deserve to be slapped for not fixing it at that time...

[apostasie]

Doing some reading, the problem is probably more involved that just a quick patch.

None of this code (any container state-changing operations) is atomic / meant to be interrupted.

We should have an isolated pkg/safe-container that is able to perform container manipulations as safely as possible, but also able to recover from previously interrupted operations.

We could implement some sort of "pending" mechanism:

• lock
• create .change-in-progress marker file - sync
• do whatever is needed
• remove .change-in-progress - sync
• unlock

If an op has been interrupted, the marker file will be left, and the next time we run the op we could do some preliminary cleanup before trying again.

Thoughts?

[AkihiroSuda]

nerdctl/pkg/labels/labels.go

Line 29 in f860057

// WARNING: multiple containers may have same the name label

This is something between bug and enhancement, as the name unicity is not designed to be guaranteed

[apostasie]

Yeah, agreed that we cannot guarantee we will never have two containers with the same name (as this can be achieved out of band just manipulating labels).

I think we should still squash the "bug" part - eg: just using nerdctl without outside data manipulation should never produce a broken situation.

Need to think about all this a bit more though. I have been chasing concurrency issues a lot over the past year and I think we have made great progress overall, but getting truly ACID is really hard, and we cannot expect casual feature development to be at that level of careful. We need better primitives - just not sure "what" exactly.