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>
This change changes the default for noOverwriteDirNonDir to be true
internally, with the intent to change the default at the API to follow
accordingly.
The `AllowOverwriteDirWithFile` option in the Client was added when
reimplementing the CLI using the API Client lib in [moby@1b2b91b].
Before that refactor, the `noOverwriteDirNonDir` query argument
[would be set unconditionally][1] by the CLI, with no options to
control the behavior.
The `noOverwriteDirNonDir` query parameter was added in [moby@db9cc91] to
set the `NoOverwriteDirNonDir` option that was implemented in pkg/archive
in [moby@a74799b].
It was added in [PR13171-comment2], following a discussion on the risk
of replacing a directory with a file and vice-versa in [PR13171-comment].
> In my latest changes from yesterday:
>
> - Removed the `GET stat-path` endpoint and added a `HEAD` handler to
> the `archive-path` endpoint. Updated the api docs to reflect this.
> Also moved api docs changes from `v1.19` to `v1.20`.
> - Added a `NoOverwriteDirNonDir` flag to `archive.TarOptions` to indicate
> that we do not want to overwrite a directory with a non-directory (and
> vice versa) when unpacking an archive.
> - Added a corresponding but optional `noOverwriteDirNonDir` parameter
> to the `PUT extract-to-dir` endpoint to specify desired behavior.
>
> These changes combine to keep the behavior we want
It's unclear why these were added as an *option* and why it was implemented
as opt-in (not opt-out), as overwriting a file with a directory (or vice-versa)
would generally be unexpected behavior.
[1]: 8c9ad7b818/api/client/cp.go (L345-L346)
[moby@1b2b91b]: 1b2b91ba43
[moby@a74799b]: a74799b701
[moby@db9cc91]: db9cc91a9e
[PR13171-comment]: https://github.com/moby/moby/pull/13171#issuecomment-106559765
[PR13171-comment2]: https://github.com/moby/moby/pull/13171#issuecomment-108538643
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These comments were added to enforce using the correct import path for
our packages ("github.com/docker/docker", not "github.com/moby/moby").
However, when working in go module mode (not GOPATH / vendor), they have
no effect, so their impact is limited.
Remove these imports in preparation of migrating our code to become an
actual go module.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- remove named error-returns
- make error-handling slightly more idiomatic (check for non-nil errors)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Windows containers do not currently support a read-only rootFS, and a
[validateReadonlyRootfs] (added in 6f7dc65847)
will reject containers created with this option. Similarly, Windows does
not support read-only volume;
Windows originally did not support volumes (47c56e4353).
Support for volumes was added later, but without support for read-only volumes
(see e5261d6e4a). Support for read-only values
was originally planned to be supported after TP4, but later moved to "post TP5"
when support for TP4 was removed in 331c8a86d4.
This patch removes code related to read-only paths on Windows, but keeps
a TODO / comment as a reminder.
[validateReadonlyRootfs]: a052449271/runconfig/hostconfig_windows.go (L56-L62)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Combine the check for read-only volume paths and the container's rootFS
to be read-only into a single check.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was a no-op on Windows, which currently doesn't support it, so we
can remove this code in the meantime until there's support for this.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was using a confusing syntax because `Isolation.IsDefault()`
and `Isolation.IsHyperV()` don't accept an argument. It's valid (see below),
but just confusing, so let's use a more common approach.
```go
package main
import "fmt"
type NameSayer string
func (f NameSayer) SayMyName() {
fmt.Println(f)
}
func main() {
var foo NameSayer = "my name is"
foo.SayMyName()
NameSayer("my name is..").SayMyName()
// Thought SayMyName() would take no arguments? Think again!
NameSayer.SayMyName("slim shady!")
}
```
While at it, also renamed the `container` argument as it was shadowing
the `container` import.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Define consts for the Actions we use for events, instead of "ad-hoc" strings.
Having these consts makes it easier to find where specific events are triggered,
makes the events less error-prone, and allows documenting each Action (if needed).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This one is a "bit" fuzzy, as it may not be _directly_ related to `archive`,
but it's always used _in combination_ with the archive package, so moving it
there.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It is only applicable to Windows so it does not need to be called from
platform-generic code. Fix locking in the Windows implementation.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The Linux implementation needs to diverge significantly from the Windows
one in order to fix platform-specific bugs. Cut the generic
implementation out of daemon/archive.go and paste identical, verbatim
copies of that implementation into daemon/archive_{windows,linux}.go to
make it easier to compare the progression of changes to the respective
implementations through Git history.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Add CreateImage() to the daemon
Refactor daemon.Comit() and expose a Image.NewChild()
Update copy to use IDMappings.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Currently, daemonbuilder package (part of daemon) implemented the
builder backend. However, it was a very thin wrapper around daemon
methods and caused an implementation dependency for api/server build
endpoint. api/server buildrouter should only know about the backend
implementing the /build API endpoint.
Removing daemonbuilder involved moving build specific methods to
respective files in the daemon, where they fit naturally.
Signed-off-by: Anusha Ragunathan <anusha@docker.com>
So other packages don't need to import the daemon package when they
want to use this struct.
Signed-off-by: David Calavera <david.calavera@gmail.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
