This method did not provide any special handling for accessing the
field, and did not handle locking. Let's remove it for now to
not pretend we're doing anything more safe than directly accessing
the field.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This constructor did not do anything other than creating an empty struct
for an exported type. While we should look at initializing with a proper
state, we currently do not, so let's not pretend we do some magic here,
and leave it for a future exercise to create a proper constructor if we
need one.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The State.Health struct has a mutex, but in various places
we access the embedded Health struct directly.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The Container.State struct holds the container's state, and most of
its fields are expected to change dynamically. Some o these state-changes
are explicit, for example, setting the container to be "stopped". Other
state changes can be more explicit, for example due to the containers'
process exiting or being "OOM" killed by the kernel.
The distinction between explicit ("desired") state changes and "state"
("actual state") is sometimes vague; for some properties, we clearly
separated them, for example if a user requested the container to be
stopped or restarted, we store state in the Container object itself;
HasBeenManuallyStopped bool // used for unless-stopped restart policy
HasBeenManuallyRestarted bool `json:"-"` // used to distinguish restart caused by restart policy from the manual one
Other properties are more ambiguous. such as "HasBeenStartedBefore" and
"RestartCount", which are stored on the Container (and persisted to
disk), but may be more related to "actual" state, and likely should
not be persisted;
RestartCount int
HasBeenStartedBefore bool
Given that (per the above) concurrency must be taken into account, most
changes to the `container.State` struct should be protected; here's where
things get blurry. While the `State` type provides various accessor methods,
only some of them take concurrency into account; for example, [State.IsRunning]
and [State.GetPID] acquire a lock, whereas [State.ExitCodeValue] does not.
Even the (commonly used) [State.StateString] has no locking at all.
The way to handle this is error-prone; [container.State] contains a mutex,
and it's exported. Given that its embedded in the [container.Container]
struct, it's also exposed as an exported mutex for the container. The
assumption here is that by "merging" the two, the caller to acquire a lock
when either the container _or_ its state must be mutated. However, because
some methods on `container.State` handle their own locking, consumers must
be deeply familiar with the internals; if both changes to the `Container`
AND `Container.State` must be made. This gets amplified more as some
(exported!) methods, such as [container.SetRunning] mutate multiple fields,
but don't acquire a lock (so expect the caller to hold one), but their
(also exported) counterpart (e.g. [State.IsRunning]) do.
It should be clear from the above, that this needs some architectural
changes; a clearer separation between "desired" and "actual" state (opening
the potential to update the container's config without manually touching
its `State`), possibly a method to obtain a read-only copy of the current
state (for those querying state), and reviewing which fields belong where
(and should be persisted to disk, or only remain in memory).
This PR preserves the status quo; it makes no structural changes, other
than exposing where we access the container's state. Where previously the
State fields and methods were referred to as "part of the container"
(e.g. `ctr.IsRunning()` or `ctr.Running`), we now explicitly reference
the embedded `State` (`ctr.State.IsRunning`, `ctr.State.Running`).
The exception (for now) is the mutex, which is still referenced through
the embedded struct (`ctr.Lock()` instead of `ctr.State.Lock()`), as this
is (mostly) by design to protect the container, and what's in it (including
its `State`).
[State.IsRunning]: c4afa77157/daemon/container/state.go (L205-L209)
[State.GetPID]: c4afa77157/daemon/container/state.go (L211-L216)
[State.ExitCodeValue]: c4afa77157/daemon/container/state.go (L218-L228)
[State.StateString]: c4afa77157/daemon/container/state.go (L102-L131)
[container.State]: c4afa77157/daemon/container/state.go (L15-L23)
[container.Container]: c4afa77157/daemon/container/container.go (L67-L75)
[container.SetRunning]: c4afa77157/daemon/container/state.go (L230-L277)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
kernel-memory limits are not supported in cgroups v2, and were obsoleted in
[kernel v5.4], producing a `ENOTSUP` in kernel v5.16. Support for this option
was removed in runc and other runtimes, as various LTS kernels contained a
broken implementation, resulting in unpredictable behavior.
We deprecated this option in [moby@b8ca7de], producing a warning when used,
and actively ignore the option since [moby@0798f5f].
Given that setting this option had no effect in most situations, we should
just remove this option instead of continuing to handle it with the expectation
that a runtime may still support it.
Note that we still support RHEL 8 (kernel 4.18) and RHEL 9 (kernel 5.14). We
no longer build packages for Ubuntu 20.04 (kernel 5.4) and Debian Bullseye 11
(kernel 5.10), which still have an LTS / ESM programme, but for those it would
only impact situations where a runtime is used that still supports it, and
an old API version was used.
[kernel v5.4]: https://github.com/torvalds/linux/commit/0158115f702b0ba208ab0
[moby@b8ca7de]: b8ca7de823
[moby@0798f5f]: 0798f5f5cf
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
So far, on ContainerStart, the daemon was silently backfilling empty
PortBindings slices with a PortBinding with unspecified HostIP and
HostPort. This was done by github.com/docker/go-connections/nat.SortPortMap.
This backfilling doesn't make much sense, and we're trying to remove
that package. So, move the backfilling to the API server, keep it for
older API versions, deprecate it for API 1.52, and drop it for API 1.53
and above.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
It better describes its purpose, and allows "Port" to be used for
other purposes (e.g. to replace "nat.Port").
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Co-authored-by: Austin Vazquez <austin.vazquez@docker.com>
Follow-up to 494677f93f, which added
the aliases, but did not yet replace our own use of the nat types.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These types used to be internal to the container package, but were
moved to the API in 100102108b.
However, the `StateStatus` type is only used internally; it's used
as an intermediate type because [`container.State`] contains a sync.Mutex
field which would make copying it unsafe (see [moby@2998945]).
This moves the type and re-introduces an internal type
in the original location, effectively reverting
100102108b
[`container.State`]: 19e79906cb/container/state.go (L15-L23)
[moby@2998945]: 2998945a54
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The stringid package is used in many places; while it's trivial
to implement a similar utility, let's just provide it as a utility
package in the client, removing the daemon-specific logic.
For integration tests, I opted to use the implementation in the
client, as those should not ideally not make assumptions about
the daemon implementation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was deprecated in 44b653ef99, which
was part of v28.2, but the container package was moved inside the daemon
in 5419eb1efc, so these aliases were no
longer useful.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was deprecated in df662ebc59, which
was part of v28.2, but the container package was moved inside the daemon
in 5419eb1efc, so these aliases were no
longer useful.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These were deprecated in 100102108b, which
was part of v28.2, but the container package was moved inside the daemon
in 5419eb1efc, so these aliases were no
longer useful.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The restartmanager is only used internally by the daemon, and has no external
users. Move it to the daemon/internal package.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
