Global compose lock #3543
Global compose lock #3543
Conversation
apostasie
force-pushed
the
lock-compose
branch
2 times, most recently
from
2c3deb8
to
554c2f8
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
When nerdctl compose exits, it will release the lock - we do not release explicitly.
The reason I did no do it explicitly is because (I think) we would have to have that code inside all compose commands, which would make for a large/more intrusive patch.
This makes here for a much simpler patch.
There was a problem hiding this comment.
Could you add code comments to explain that
There was a problem hiding this comment.
Also wondering if we can just add *Composer.Close().
The caller of compose.New should have defer c.Close()
There was a problem hiding this comment.
Also wondering if we can just add
*Composer.Close().
The caller ofcompose.Newshould havedefer c.Close()
Sure.
Is it worth the effort and extra code though?
| "github.com/containerd/nerdctl/v2/pkg/netutil" | ||
| "github.com/containerd/nerdctl/v2/pkg/referenceutil" | ||
| "github.com/containerd/nerdctl/v2/pkg/signutil" | ||
| "github.com/containerd/nerdctl/v2/pkg/strutil" | ||
| ) | ||
|
|
||
| //nolint:unused | ||
| var locked *os.File |
There was a problem hiding this comment.
Otherwise the lock will get released once we exit the scope of New().
We could add the lock to the Composer struct, but I am not sure that would bring much value?
There was a problem hiding this comment.
Could you add code comments?
Also, how is the file lock associated with the lifetime of the Go variable? I don’t think *os.File has a destructor method to unlock the file?
There was a problem hiding this comment.
Could you add code comments?
Sure.
Also, how is the file lock associated with the lifetime of the Go variable? I don’t think *os.File has a destructor method to unlock the file?
I was surprised too.
My (vague) explanation is that garbage collection does close the file descriptor - and yes, flock() will release the lock when the fd gets closed.
apostasie
force-pushed
the
lock-compose
branch
from
554c2f8
to
683c6b3
Compare
Signed-off-by: apostasie <[email protected]>
apostasie
force-pushed
the
lock-compose
branch
from
683c6b3
to
fa39963
Compare
|
Do you plan to update this for Unlock()? |
I would rather not - unless you think it would really be better? Just lmk. As far as I can think about this, the meaning of this lock is to stay for the entire life of the command execution. Unlocking it manually does not seem to serve much purpose. This here overall is a ugly hack to alleviate a hard to fix problem - but should not stay in the long term. |
Our compose implementation is inherently not safe to use concurrently.
The root issue is that we do shell out multiple times to nerdctl to manipulate a bunch of resources, while assuming a state of the world that has possibly changed meanwhile.
Because of that, we cannot leverage API locking mechanisms (eg: as implemented in VolStore, etc) across multiple distincts invocations of nerdctl.
This PR suggests that we put in place a global filesystem lock for compose.
This is not pretty - and it will definitely prevent running multiple
nerdctl composecommands in parallel (they will be queued).Also, this is not going to solve all concurrency / racy issues with compose - as normal nerdctl operations may still run in parallel with a compose invocation.
But at least, it will prevent some of the most egregious problems - until we fix the core issues.
Let me know your thoughts.