Commit graph

8178 commits

Author SHA1 Message Date
Sebastiaan van Stijn
3d94eb9bcd
daemon: Daemon.getPidContainer: change to accept "id" argument
This function didn't need the whole container, only its ID, so let's
use that as argument. This also makes it consistent with getIpcContainer.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-24 16:20:42 +02:00
Sebastiaan van Stijn
bc7f341f29
daemon: WithNamespaces(): fix incorrect error for PID, IPC namespace
`Daemon.getPidContainer()` was wrapping the error-message with a message
("cannot join PID of a non running container") that did not reflect the
actual reason for the error; `Daemon.GetContainer()` could either return
an invalid parameter (invalid / empty identifier), or a "not found" error
if the specified container-ID could not be found.

In the latter case, we don't want to return a "not found" error through
the API, as this would indicate that the container we're _starting_ was
not found (which is not the case), so we need to convert the error into
an `errdefs.ErrInvalidParameter` (the container-ID specified for the PID
namespace is invalid if the container doesn't exist).

This logic is similar to what we do for IPC namespaces. which received
a similar fix in c3d7a0c603.

This patch updates the error-types, and moves them into the getIpcContainer
and getPidContainer container functions, both of which should return
an "invalid parameter" if the container was not found.

It's worth noting that, while `WithNamespaces()` may return an "invalid
parameter" error, the `start` endpoint itself may _not_ be. as outlined
in commit bf1fb97575, starting a container
that has an invalid configuration should be considered an internal server
error, and is not an invalid _request_. However, for uses other than
container "start", `WithNamespaces()` should return the correct error
to allow code to handle it accordingly.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-24 16:19:07 +02:00
Sebastiaan van Stijn
bd5d6480e7
daemon: WithNamespaces(): remove redundant "if"
This check was originally used to only validate the mode if it was set to
a non-empty value (see commit 072400fc4b), but
validation was made unconditional in c3d7a0c603.

Given that a `CgroupnsMode` can't be both [`CgroupnsMode.IsEmpty()`][1]
and [`CgroupnsMode.IsPrivate`][2], we can remove the extra check.

[1]: e0da5cb929/api/types/container/hostconfig.go (L33-L36)
[2]: e0da5cb929/api/types/container/hostconfig.go (L23-L26)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-24 16:12:22 +02:00
Sebastiaan van Stijn
6eaefe5bf5
daemon: WithNamespaces(): use switch instead of if/else if/else
We were using a mixture of approaches for these; aligning them a bit
to all use switch statements.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-24 16:12:22 +02:00
Sebastiaan van Stijn
43aa67a982
daemon: WithNamespaces(): use OCI-spec consts for namespaces
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-24 16:12:21 +02:00
Sebastiaan van Stijn
759698581e
daemon: WithNamespaces(): inline variables
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-24 16:12:21 +02:00
Sebastiaan van Stijn
13648a0e21
daemon: remove Daemon.checkContainer and related utils
This was added in 12485d62ee to save some
duplication, but was really over-engineered to save a few lines of code,
at the cost of hiding away what it does and also potentially returning
inconsistent errors (not addressed in this patch). Let's start with
inlining these.

This removes;

- Daemon.checkContainer
- daemon.containerIsRunning
- daemon.containerIsNotRestarting

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-24 16:12:18 +02:00
Sebastiaan van Stijn
64f5d9b119
Merge pull request #46213 from thaJeztah/daemon_remove_errors
daemon: cleanupContainer: don't fail if container is already stopped
2023-08-24 13:34:43 +02:00
Paweł Gronowski
01214bafd2
c8d/builder: Don't drop fields from created image
Previous image created a new partially filled image.
This caused child images to lose their parent's layers.

Instead of creating a new object and trying to replace its fields, just
clone the original passed image and change its ID to the manifest
digest.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2023-08-24 09:44:12 +02:00
Sebastiaan van Stijn
53afd2ae9f
Merge pull request #46201 from thaJeztah/daemon_updateSandboxNetworkSettings_err
libnetwork: remove unused err-return, and minor refactor around Sandbox creating
2023-08-23 19:56:58 +02:00
Sebastiaan van Stijn
d74f0a47de
Merge pull request #46207 from thaJeztah/daemon_cleanup_withLibnetwork
daemon: withLibnetwork(): return early if networking is disabled
2023-08-23 19:56:01 +02:00
Sebastiaan van Stijn
9b9348ce86
Merge pull request #46284 from vvoland/c8d-legacybuilder-fix-from-scratch
c8d: Fix building Dockerfiles that have `FROM scratch`
2023-08-23 18:57:53 +02:00
Sebastiaan van Stijn
e2fc596de4
Merge pull request #46293 from rumpl/fix-platform-check
Don't return lease not found errors on platform mismatch
2023-08-23 15:55:04 +02:00
Sebastiaan van Stijn
2b583c0923
daemon: cleanupContainer: slightly cleanup error messages
Also remove integration-cli: `DockerAPISuite.TestContainerAPIDeleteConflict`,
which was testing the same conditions as `TestRemoveContainerRunning` in
integration/container.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-23 15:50:43 +02:00
Sebastiaan van Stijn
69cf2ad6a5
daemon: cleanupContainer: don't fail if container is already stopped
Saw this failure in a flaky test, and I wondered why we consider this an
error condition;

    === RUN   TestKillWithStopSignalAndRestartPolicies
        main_test.go:32: assertion failed: error is not nil: Error response from daemon: Could not kill running container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7, cannot remove - Container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7 is not running: failed to remove 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7
    --- FAIL: TestKillWithStopSignalAndRestartPolicies (0.84s)
    === RUN   TestKillWithStopSignalAndRestartPolicies/same-signal-disables-restart-policy
        --- PASS: TestKillWithStopSignalAndRestartPolicies/same-signal-disables-restart-policy (0.42s)
    === RUN   TestKillWithStopSignalAndRestartPolicies/different-signal-keep-restart-policy
        --- PASS: TestKillWithStopSignalAndRestartPolicies/different-signal-keep-restart-policy (0.23s)

In the above;

1. `Error response from daemon: Could not kill running container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7`
2. `cannot remove - Container 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7 is not running`
3. `failed to remove 668f62511f4aa62357269cd405cff1fbe295b7f6d5011e7cfed434e3072330b7`

So it looks like the removal fails because we couldn't kill the container
because it was already stopped, which may be a race condition where the first
check shows the container to be running (but may already be in process to be
removed or killed. In either case, we probably shouldn't fail the removal if
the container is already stopped.

This patch adds a `isNotRunning()` utility, so that we can ignore this case,
and proceed with the removal.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-23 15:47:43 +02:00
Sebastiaan van Stijn
20b770237c
daemon/network: Settings: move sandbox fields together
Just a minor nit; let's move these fields together.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-23 14:26:58 +02:00
Sebastiaan van Stijn
405f571ae8
daemon: updateSandboxNetworkSettings: remove unused error-return
This function never returns an error, so let's remove the error-return,
and give it a slightly more to-the-point name.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-23 14:26:58 +02:00
Sebastiaan van Stijn
70ae5c13ea
Merge pull request #44704 from thaJeztah/api_hostconfig_RestartPolicyMode_enum
api/types/container: add RestartPolicyMode type and enum
2023-08-22 22:31:51 +02:00
Bjorn Neergaard
8383430946
Merge pull request #45759 from akerouanton/validate-ipam-config
Validate IPAM config before handing it over to libnetwork
2023-08-22 13:58:28 -06:00
Paweł Gronowski
bedcc94de4
daemon: Handle NotFound when deleting container lease
If the lease doesn't exit (for example when creating the container
failed), just ignore the not found error.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2023-08-22 17:32:22 +02:00
Paweł Gronowski
dfaff9598c
c8d/run: Allow running container without image
This allows the legacy builder to apply changes to the `FROM scratch`
layer.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2023-08-22 17:32:20 +02:00
Paweł Gronowski
eb56493f4e
c8d/commit: Don't produce an empty layer
If the diff is empty and don't produce an empty layer.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2023-08-22 17:32:18 +02:00
Albin Kerouanton
4f47013feb
api: Validate IPAM config before creating a network
Currently, IPAM config is never validated by the API. Some checks
are done by the CLI, but they're not exhaustive. And some of these
misconfigurations might be caught early by libnetwork (ie. when the
network is created), and others only surface when connecting a container
to a misconfigured network. In both cases, the API would return a 500.

Although the `NetworkCreate` endpoint might already return warnings,
these are never displayed by the CLI. As such, it was decided during a
maintainer's call to return validation errors _for all API versions_.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-22 17:11:54 +02:00
Sebastiaan van Stijn
2be118379e
api/types/container: add RestartPolicyMode type and enum
Also move the validation function to live with the type definition,
which allows it to be used outside of the daemon as well.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-22 16:40:57 +02:00
Djordje Lukic
b8ff8ea58e
Don't return an error if the lease is not found
If the image for the wanted platform doesn't exist then the lease
doesn't exist either. Returning this error hides the real error, so
let's not return it.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
2023-08-22 14:52:41 +02:00
Sebastiaan van Stijn
6dba98cf38
libnetwork: implement Controller.GetSandbox(containerID)
Various parts of the code were using "walkers" to iterate over the
controller's sandboxes, and the only condition for all of them was
to find the sandbox for a given container-ID. Iterating over all
sandboxes was also sub-optimal, because on Windows, the ContainerID
is used as Sandbox-ID, which can be used to lookup the sandbox from
the "sandboxes" map on the controller.

This patch implements a GetSandbox method on the controller that
looks up the sandbox for a given container-ID, using the most optimal
approach (depending on the platform).

The new method can return errors for invalid (empty) container-IDs, and
a "not found" error to allow consumers to detect non-existing sandboxes,
or potentially invalid IDs.

This new method replaces the (non-exported) Daemon.getNetworkSandbox(),
which was only used internally, in favor of directly accessing the
controller's method.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-21 15:06:26 +02:00
Sebastiaan van Stijn
a866e50e6e
Merge pull request #46238 from vvoland/c8d-inspect-handle-missing-config
c8d/inspect: Ignore manifest with missing config
2023-08-18 15:35:44 +02:00
Sebastiaan van Stijn
1c937c5816
Merge pull request #46189 from vvoland/c8d-more-mount-refcount
c8d integration: Use refcount mounter for diff and export
2023-08-18 15:29:13 +02:00
Paweł Gronowski
a64adda4e7
c8d/inspect: Ignore manifest with missing config
Fix a failure to inspect image if any of its present manifest references
an image config which isn't present locally.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2023-08-17 09:48:29 +02:00
Sebastiaan van Stijn
b325dcbff6
Merge pull request #46195 from thaJeztah/daemon_cleanup_start
daemon: Daemon.ContainerStart(): make validateState a regular function, and remove containerNotModifiedError
2023-08-16 12:18:40 +02:00
Sebastiaan van Stijn
18a0ff2b2b
daemon: Daemon.handleContainerExit(): rename err-var for clarity
The "cpErr" naming was a bit confusing; give it a more descriptive name.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-14 15:17:43 +02:00
Sebastiaan van Stijn
f8363690ca
daemon: Daemon.handleContainerExit(): reduce ambiguity in error handling
This goroutine was added in c458bca6dc, and
looks for errors from the wait channel. If no error is returned, it attempts
to start the container, and *updates* the error if a failure happened while
doing so, so that the code below it can update the container's status, and
perform auto-remove (if set for the container).

However, due to the formatting of the code, it was easy to overlook that
the "err" variable was not local to the "if" statement.

This patch breaks up the if-statement in an attempt to make it clearer that
this is not a local "err" variable, and adds a code-comment explaining the
logic.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-14 13:05:16 +02:00
Sebastiaan van Stijn
e0f5bb4820
daemon: withLibnetwork(): return early if networking is disabled
The function was checking in a loop if networking for the container was
disabled. Change the function to return early, and to only set hooks
if one needs to be set.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-12 19:03:32 +02:00
Sebastiaan van Stijn
225691e258
daemon: withLibnetwork(): use OCI-spec consts for namespaces
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-12 19:03:32 +02:00
Sebastiaan van Stijn
80d158e0de
daemon: remove containerNotModifiedError
Removing this type, because:

- containerNotModifiedError is not an actual error, and abstracting it away
  was hiding some of these details. It also wasn't used as a sentinel error
  anywhere, so doesn't have to be its own type.
- Defining a type just to toggle the error-message between "not running"
  and "not stopped" felt a bit over-the-top, as each variant was only used once.
- So  "it only had one job", and it didn't even do that right; it produced
  capitalized error messages, which makes linters unhappy.

So, let's just inline what it does in the two places it was used.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-11 21:28:06 +02:00
Sebastiaan van Stijn
dffe634c19
daemon: Daemon.ContainerStart(): make validateState a regular function
There's no need for this to be a closure; let's just make it a regular
function. While moving it out, also make some minor code-changes and
add some code-comments to describe the flow / intent, which may not
be trivial for people that are not familiar with these details.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-11 21:28:06 +02:00
Paweł Gronowski
6da42ca830
c8d/diff: Reuse mount, mount parent as read-only
The container rw layer may already be mounted, so it's not safe to use
it in another overlay mount. Use the ref counted mounter (which will
reuse the existing mount if it exists) to avoid that.

Also, mount the parent mounts (layers of the base image) in a read-only
mode.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2023-08-11 15:30:38 +02:00
Paweł Gronowski
051d51b222
c8d/export: Use ref counted mounter
To prevent mounting the container rootfs in a rw mode if it's already
mounted.  This can't use `mount.WithReadonlyTempMount` because the
archive code does a chroot with a pivot_root, which creates a new
directory in the rootfs.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2023-08-11 15:30:36 +02:00
Sebastiaan van Stijn
bf1fb97575
daemon: Daemon.containerStart(): add comment to clarify error-type
Any error that occurs while creating the spec, even if it's the
result of an invalid container config, must be considered a System
error (internal server error), as it's not an error with the request
to start the container.

Invalid configuration in the config itself must be validated when
creating the container (creating its config), but some errors are
dependent on the current state, for example when starting a container
that shares a namespace with another container, and that container
is not running (or missing).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-11 14:47:22 +02:00
Albin Kerouanton
38e26c4717
daemon/create.go: Fix error capitalization
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
2023-08-10 12:15:34 +02:00
Albin Kerouanton
742475bc8d
daemon/create.go: Supersede github.com/pkg/errors
Will make it possible to use `errors.Join()` in that file.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
2023-08-10 01:30:10 +02:00
Sebastiaan van Stijn
74354043ff
remove uses of libnetwork/Network.Info()
Now that we removed the interface, there's no need to cast the Network
to a NetworkInfo interface, so we can remove uses of the `Info()` method.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-08 22:05:30 +02:00
Sebastiaan van Stijn
0485b8f0fb
Merge pull request #46170 from akerouanton/daemon-network-replace-pkg-errors
daemon/network.go: Remove github.com/pkg/errors pkg
2023-08-08 18:37:31 +02:00
Albin Kerouanton
fd4ec26313
daemon/network.go: Fix error capitalization
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
2023-08-08 17:40:19 +02:00
Albin Kerouanton
64e01c627f
daemon/network.go: Remove github.com/pkg/errors pkg
PR moby/moby#45759 is going to use the new `errors.Join` function  to
return a list of validation errors.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
2023-08-08 17:39:53 +02:00
Sebastiaan van Stijn
f2be77a5ab
Merge pull request #46058 from thaJeztah/cleanup_buildSandboxOptions
daemon: cleanup Daemon.buildSandboxOptions
2023-08-08 11:38:24 +02:00
Sebastiaan van Stijn
4b19b2f4ba
Merge pull request #46004 from elezar/add-cdi-spec-dirs-to-info
Add CDISpecDirs to Info output
2023-08-07 20:14:06 +02:00
Sebastiaan van Stijn
4f9540f19f
daemon: remove some intermediate vars and some cleanup
Remove some intermediate vars, move vars closer to where they're used,
and introduce local var for `nw.Name()` to reduce some locking/unlocking in:

- `Daemon.allocateNetwork()`
- `Daemon.releaseNetwork()`
- `Daemon.connectToNetwork()`
- `Daemon.disconnectFromNetwork()`
- `Daemon.findAndAttachNetwork()`

Also un-wrapping some lines to make it slightly easier to read the conditions.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-05 10:44:21 +02:00
Sebastiaan van Stijn
8153a7760e
daemon: Daemon.tryDetachContainerFromClusterNetwork: cleanup
- Remove intermediate variable
- Optimize the order of checks in the condition; check for unmanaged containers
  first, before getting information about cluster state and network information.
- Simplify the log messages, as the error would already contain the same
  information about the network (name or ID) and container (ID), so would
  print the network ID twice:

    error detaching from network <ID>: could not find network attachment for container <ID> to network <name or ID>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-05 10:44:21 +02:00
Sebastiaan van Stijn
a018bb1da7
daemon: Daemon.initializeNetworking: remove var declaration
The function was declaring an err variable which was shadowed. It was
intended for directly assigning to a struct field, but as this function
is directly mutating an existing object, and the err variable was declared
far away from its use, let's use an intermediate var for that to make it
slightly more atomic.

While at it, also combined two "if" branches.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-05 10:44:21 +02:00