Split container runtime state from config#28483
Conversation
|
@cpuguy83 I'm not really a fan of singletons. What's problem with creating object on-demand? |
|
@LK4D4 I'm generally not a fan either, but I think it works out pretty well here, particularly because container ids must be unique. I'm not sure where else to put this at the moment other than on the daemon, which seems worse to me. |
There was a problem hiding this comment.
let's follow "convention" and put mu on top of protected fields.
There was a problem hiding this comment.
where cleanup of this state happens?
There was a problem hiding this comment.
On daemon.cleanupContainer we call container.RemoveRunState(), which calls this.
There was a problem hiding this comment.
Yeah, I mean what stops logger and copier and other stuff. Where we make sure that we have no references to fields of state in goroutines.
There was a problem hiding this comment.
You want to verify that everything is stopped first, works for me.
There was a problem hiding this comment.
ok, now we container.Reset() before calling removeRunState
There was a problem hiding this comment.
looks like add is not really used as add, more like create. Maybe remove s argument and allocate state here?
|
Test failure looks legit, taking a look. |
cpuguy83
force-pushed
the
split_cntr_runtime_data
branch
from
cf349fe to
5db3fee
Compare
|
And all green. |
|
@LK4D4 Maybe I can put his into container.Store somewhere. |
|
I think for now I'll just store it on container. This PR will introduce a nice split bewteen config and runtime, can take it further in another PR. |
cpuguy83
force-pushed
the
split_cntr_runtime_data
branch
3 times, most recently
from
c229328 to
bd66004
Compare
|
Would be great to get more eyes on this. |
cpuguy83
force-pushed
the
split_cntr_runtime_data
branch
from
bd66004 to
46875e4
Compare
This is step-1 of implementing a lock-less container object. This moves the live/runtime state off the container object onto a separately managed object. Effectively, these are items on the container struct that cannot (or should not) be copied, e.g. i/o streams, signal channels, etc. Signed-off-by: Brian Goff <[email protected]>
cpuguy83
force-pushed
the
split_cntr_runtime_data
branch
from
46875e4 to
b1dacbe
Compare
thaJeztah
added
the
status/needs-attention
|
ping @tonistiigi @crosbymichael could you have a look at this one? |
|
@cpuguy83 needs a rebase |
|
@cpuguy83 Could we have a design outline for "lockless container object" with the description of the steps/problems involved. There maybe general comments first and it would make reviewing the individual steps easier. |
|
@cpuguy83 and other maintainers: given the proposal is being discussed now in the GH issue, should we still be reviewing this code? I had offered to start reviewing, but if you think there might be a shift in direction based on the proposal discussion, I'll hold off. |
|
This still depends on #30225 |
|
closing for now while we discuss on #30225 |
7 participants
This is step-1 of implementing a lock-less container object.
This moves the live/runtime state off the container object onto a
separately managed object.
Effectively, these are items on the container struct that cannot (or
should not) be copied, e.g. i/o streams, signal channels, secrets, etc.
The new object is stored in a package level singleton that the container
object is able to get at. The objects are keyed by the container ID,
this means using the singleton should not cause any issues with other
uses (e.g. tests, dep injection, etc) as container ID's are unique.
Replaces some of the work from #26759
Signed-off-by: Brian Goff [email protected]