Since commit 51d7f95 ("libnet: remove struct endpointCnt") an
endpoint count for networks has not been persisted.
But, on downgrade to a version older than that commit, the
missing field caused daemon startup to fail.
So, create the count in the store - it only needs to exist, it's
no longer maintained as a count of endpoints. On downgrade, the
count is probably zero anyway (the daemon is stopped), but the
older daemon fixes it up on startup if necessary.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Plumb context from the API down to libnet driver method `CreateNetwork`,
and add an OTel span to the bridge driver's `createNetwork` method.
Include a few attributes describing the network configuration (e.g.
IPv4/IPv6, ICC, internal and MTU).
A new util function, `RecordStatus`, is added to the `otelutil` package
to easily record any error, and update the span status accordingly.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
endpointCnt is a refcounter used to track how many endpoints use a
network, and how many networks references a config-only network. It's
stored separately from the network.
This is only used to determine if a network can be removed.
This commit removes the `endpointCnt` struct and all its references. The
refcounter is replaced by two lookups in the newly introduced `networks`
and `endpoints` caches added to the `Controller`.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The `Controller`'s store is used by:
- `deleteFromStore`
- `getNetworks`
- `getNetworksFromStore`
- `updateToStore`
- … and other methods that can't store / delete / retrieve a Network
Calls to `updateToStore` and `deleteFromStore` have been replaced with
`upsertNetwork` and `deleteNetwork`.
Both `getNetworks` and `getNetworksFromStore` call `cacheNetwork` to
ensure networks loaded from the datastore are kept in-memory.
Finally, `sandboxRestore` was instantiating `Network` itself. These are
cached too.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The `Controller`'s store is used by:
- `deleteFromStore`
- `getEndpointFromStore`
- `getEndpointsFromStore`
- `updateToStore`
- … and other methods that can't store / delete / retrieve an Endpoint
Calls to `updateToStore` and `deleteFromStore` have been replaced with
`upsertEndpoint` and `deleteEndpoint`.
Both `getEndpointFromStore` and `getEndpointsFromStore` call
`cacheEndpoint` to ensure endpoints loaded from the datastore are kept
in-memory.
Finally, `sandboxRestore` was instantiating `Endpoint` itself. These are
cached too.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Creating a swarm network from a config-only network failed
because the new EnableIPv4 wasn't validated/propagated
correctly.
So:
- Always initialise EnableIPv4 to true, including for a config
only network, and on load from the store.
- Treat enableIPv4=true as the no-overridden state when checking
params for a config-from network.
- Propagate settings from the config 'Network' objects attributes
to its generic options, because the network driver only sees
generic options.
- This was happening already for Network.internal, after the
config-only network options were processed. Move that to
'applyConfigurationTo'.
- Add enableIPv4/enableIpv6 - enableIPv6 will normaly be present
anyway. But, for a network created pre-28.x and restored from
the store, there was no entry for 'netlabel.EnableIpv4'.
- Extend TestSwarmScopedNetFromConfig to start a service and
check it's ok.
Signed-off-by: Rob Murray <rob.murray@docker.com>
For swarm networks, Controller.NewNetwork is called to validate
network config ... nothing gets created, but ManagerRedirectError
is returned if the config is ok - then swarm does its own thing.
So, for a --config-from network, merge config before checking
whether it'll have IPv4 enabled.
Signed-off-by: Rob Murray <rob.murray@docker.com>
It was only returned in a few places, and not used any different than
a "invalid parameter" error, so let's use a standard errdefs.ErrInvalidParameter
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was only returned in a few places, and not used any different than
a "invalid parameter" error, so let's use a standard errdefs.ErrInvalidParameter
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
diagnosticServer is only written to during controller.New, and the
diagnostic server itself already has a mutex on EnableDiagnostic,
DisableDiagnostic, and IsDiagnosticEnabled, which should prevent
issues trying to concurrently change its state.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Handle situations where the server is already started or stopped internally,
instead of requiring the caller to do this.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
A gateway address is always reserved before the network driver is
asked to create the network. But, the driver doesn't always need a
gateway address, so the address reservation can be unnecessary.
This means, for example, an "--internal" IPv4 "/31" network cannot
be used as a point-to-point link, because one of its two addresses
is always reserved for a gateway.
So, before allocating a gateway address, ask the network driver if
it will need one (based on options that only the network driver can
interpret). Implement that as an optional interface for network
drivers.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Before that change, we were passing the datastore to network drivers
through a `map[string]interface{}`. Then, each driver that needed the
store would cast the datastore to the correct type.
This was not a good design, as it was not clear which drivers were using
the store and which were not. Not all unit tests were passing the store,
leading to logs about uninitialized store being written.
This change makes the store a parameter of the `RegisterX` functions.
All unit tests are now passing a valid datastore to the drivers. A new
testutil func is added for that purpose.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
It no longer needs to be moved to the top of the filter-FORWARD
chain after creating a new bridge network. But, it does need to
be rearranged after setting up Swarm ingress.
Similarly, the jump to DOCKER-INGRESS no longer needs to be
shuffled back to the top of the FORWARD chain after adding a
new network.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Commit 3ec19ff62b introduced a GC goroutine to delete files where netns
were mounted. It was primarly added to work around a race in kernel
3.18-4.0.1. Since no distros we support are using such old kernels,
there's no need to keep this code around.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
In 27.x and earlier releases libnetwork's database file was in a
sub-directory "network/files" under the daemon's root data dir.
That part of the path got lost in commit ed08486ec7
So, libnet data ended up in the daemon's main Bolt db. Then, on
upgrade, config in the original file was no longer accessible.
libnet doesn't need access to any data outside its sub-dir, so
change the meaning of its OptionDataDir - it now points at libnet's
sub-dir, so the db will be created in the right place. Also, update
other uses of that data dir to match.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Currently, the DOCKER-USER chains are set up on firewall reload or network
creation. If there are running containers at startup, configureNetworking won't
be called (daemon/daemon_unix.go), so the user chains won't be setup.
This commit puts the setup logic on a separate function, and calls it on the
original place and on initNetworkController.
Signed-off-by: Andrés Maldonado <maldonado@codelutin.com>
That function was needlessly complex. Instead of relying on a struct and
a sub-struct, it now just takes two string params: a path and a bucket
name.
Libnetwork config is now initialized with default values. A new struct
is introduced in libnetwork/config to let tests customize the path and
bucket name.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
libnetwork/controller.go:1054:32: printf: non-constant format string in call to github.com/docker/docker/libnetwork/types.NotFoundErrorf (govet)
return types.NotFoundErrorf(err.Error())
^
libnetwork/controller.go:1073:32: printf: non-constant format string in call to github.com/docker/docker/libnetwork/types.NotFoundErrorf (govet)
return types.NotFoundErrorf(err.Error())
^
libnetwork/sandbox_externalkey_unix.go:113:21: printf: non-constant format string in call to fmt.Errorf (govet)
return fmt.Errorf(string(buf[0:n]))
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Previous commit made it clear that c.store can't be nil. Hence,
`c.store.Close()` can be called without checking if c.store is nil.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This was done in a separate method, called by the ctrler constructor.
This method was returning a nil datastore when c.cfg was nil -- but that
can't happen in practice!
This was giving the impression that the controller could be run without
a datastore properly configured. It's not the case, so make it explicit
by instantiating the datastore before `Controller`.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Prior to this change, cnmallocator would call
`ConfigGlobalScopeDefaultNetworks` right before initializing its
IPAM drivers. This function was mutating some global state used
during drivers init.
This change just remove the global state, and adds an arg to
ipams.Register and defaultipam.Register to pass the global pools
by arguments instead.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The bbolt library wants exclusive access to the boltdb file and uses
file locking to assure that is the case. The controller and each network
driver that needs persistent storage instantiates its own unique
datastore instance, backed by the same boltdb file. The boltdb kvstore
implementation works around multiple access to the same boltdb file by
aggressively closing the boltdb file between each transaction. This is
very inefficient. Have the controller pass its datastore instance into
the drivers and enable the PersistConnection option to disable closing
the boltdb between transactions.
Set data-dir in unit tests which instantiate libnetwork controllers so
they don't hang trying to lock the default boltdb database file.
Signed-off-by: Cory Snider <csnider@mirantis.com>
I was trying to find out why `docker info` was sometimes slow so
plumbing a context through to propagate trace data through.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Its only remaining purpose is to elide removing the endpoint from the
service records if it was not previously added. Deleting the service
records is an idempotent operation so it is harmless to delete service
records which do not exist.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The meaning of the (*Controller).isDistributedControl() method is not
immediately clear from the name, and it does not have any doc comment.
It returns true if and only if the controller is neither a manager node
nor an agent node -- that is, if the daemon is _not_ participating in a
Swarm cluster. The method name likely comes from the old abandoned
datastore-as-IPC control plane architecture for libnetwork. Refactor
c.isDistributedControl() -> !c.isSwarmNode()
to make it easier to understand code which consumes the method.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The github.com/containerd/containerd/log package was moved to a separate
module, which will also be used by upcoming (patch) releases of containerd.
This patch moves our own uses of the package to use the new module.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It was only used in a single place, and it was defined far away from
where it was used.
Move the code inline, so that it's clear at a glance what it's doing.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Controller.getNetworkFromStore() already returns a ErrNoSuchNetwork if
no network was found, so we don't need to convert the existing error.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Fixes#18864, #20648, #33561, #40901.
[This GH comment][1] makes clear network name uniqueness has never been
enforced due to the eventually consistent nature of Classic Swarm
datastores:
> there is no guaranteed way to check for duplicates across a cluster of
> docker hosts.
And this is further confirmed by other comments made by @mrjana in that
same issue, eg. [this one][2]:
> we want to adopt a schema which can pave the way in the future for a
> completely decentralized cluster of docker hosts (if scalability is
> needed).
This decentralized model is what Classic Swarm was trying to be. It's
been superseded since then by Docker Swarm, which has a centralized
control plane.
To circumvent this drawback, the `NetworkCreate` endpoint accepts a
`CheckDuplicate` flag. However it's not perfectly reliable as it won't
catch concurrent requests.
Due to this design decision, API clients like Compose have to implement
workarounds to make sure names are really unique (eg.
docker/compose#9585). And the daemon itself has seen a string of issues
due to that decision, including some that aren't fixed to this day (for
instance moby/moby#40901):
> The problem is, that if you specify a network for a container using
> the ID, it will add that network to the container but it will then
> change it to reference the network by using the name.
To summarize, this "feature" is broken, has no practical use and is a
source of pain for Docker users and API consumers. So let's just remove
it for _all_ API versions.
[1]: https://github.com/moby/moby/issues/18864#issuecomment-167201414
[2]: https://github.com/moby/moby/issues/18864#issuecomment-167202589
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
