Add CLI support for running dockerd in a container on containerd #1260
Conversation
|
Please sign your commits following these rules: $ git clone -b "ce_q3" [email protected]:dhiltgen/cli-1.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354176960
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
Codecov Report
@@ Coverage Diff @@
## master #1260 +/- ##
==========================================
+ Coverage 54.05% 54.77% +0.71%
==========================================
Files 272 292 +20
Lines 18113 19257 +1144
==========================================
+ Hits 9791 10548 +757
- Misses 7706 8055 +349
- Partials 616 654 +38 |
|
The recent lint failure looks like a CI hiccup, unrelated to the changes I just pushed Could someone whack CI to re-run that again? |
There was a problem hiding this comment.
On design:
- shouldn't we re-use the same "output formatting" when pulling images, etc… right now, it's telling me it's pulling something, I have no idea of the progress… We could use
dockerorcontainerdprogress formatting. - should
/etc/dockerpath be configurable ? For example, my system's/etc/is read-only (managed by nixos), and I want to test/run this PR, couldn't I be able to specify where it's gonna write the configuration ?
I though it was--config-filebut
λ sudo ./build/docker-linux-amd64 engine init --config-file=/tmp/etc/docker
Failed to create docker daemon: failed to verify host dir /tmp/etc: Failed to create path /tmp/etc
- it seems there is a need to
loginbefore having a docker daemon, and this is a problem asdocker loginneeds a deamon — on this, we may want to ask for login(s) whenever the command the require them (listing license, …), and later on issuing adocker loginon the newly bootstrapped daemon (programatically). Theloginclient-side only seems hackish to me. - On
updateit's possible to change theversionand theregistry-prefixbut not theengine-image, what is the rationale for that ? 🤔 - I wished (might not be possible) that the
licensechecking (mainly used indocker engine activate) would be plugable (as our credential helpers are) — this would remove the need to vendordocker/licensingcode, but it would make it harder to migrate from CE to EE…
I still need to bootstrap a sandbox machine to test it fully though.
On code:
- It would be really good to have unit tests on the
enginepackage 👼 - I feel like
containerdutilspackage should be some sort of "wrapper" client that get's created with our "view" ofdockerandcontainerdclient. That way we wouldn't need to pass them for each and every exported functions. - In general,
command.Clishould be only used incmd/*packages.. having a dependency on it in thecontainerdutilspackage seems like a smell… - This is a public facing repository, that is easily use as library… and in this context, any exported element (function, struct) should be considered stable.. So the less we exported at first, the better… This is not the case right now.
github.com/docker/licensingneeds some tests, and it would be very handy if it were using eithervndrorgolang/deptool
cli/command/engine/activate.go
Outdated
| "strconv" | ||
| "strings" | ||
|
|
||
| "golang.org/x/net/context" |
There was a problem hiding this comment.
use context instead of this deprecated package.
cli/command/engine/activate.go
Outdated
|
|
||
| "golang.org/x/net/context" | ||
|
|
||
| "github.com/containerd/containerd/namespaces" |
There was a problem hiding this comment.
Also, we group all non-stdlib imports, so those should be grouped
cli/command/engine/activate.go
Outdated
| flags := cmd.Flags() | ||
|
|
||
| flags.StringVarP(&options.licenseFile, "license", "l", "", "License File") | ||
| flags.StringVarP(&options.engineVersion, "engine-version", "", "", "Specify engine version (default is to use existing version)") |
There was a problem hiding this comment.
Those should be flags.StringVar when they don't have a shorthand counter-part
cli/command/engine/activate.go
Outdated
|
|
||
| flags := cmd.Flags() | ||
|
|
||
| flags.StringVarP(&options.licenseFile, "license", "l", "", "License File") |
There was a problem hiding this comment.
I think we can keep those long flag only for now 👼
cli/command/engine/activate.go
Outdated
| flags.StringVarP(&options.engineVersion, "engine-version", "", "", "Specify engine version (default is to use existing version)") | ||
| flags.StringVarP(&options.registryPrefix, "registry-prefix", "", "docker.io/docker", "Override the default location where engine images are pulled") | ||
| flags.StringVarP(&options.format, "format", "", "", "Pretty-print licenses using a Go template") | ||
| flags.BoolVarP(&options.quiet, "queit", "q", false, "Only display available licenses by ID") |
cli/containerdutils/hostpaths.go
Outdated
| } | ||
| _, err = task.Delete(ctx) | ||
| if err != nil { | ||
| logrus.Debugf("Failed to delete task: %s", err) |
There was a problem hiding this comment.
almost silently ignoring these errors 🤔 shouldn't we at least print some warnings ?
cli/containerdutils/hostpaths.go
Outdated
| } | ||
| err = container.Delete(ctx, containerd.WithSnapshotCleanup) | ||
| if err != nil { | ||
| logrus.Debugf("Failed to delete container: %s", err) |
cli/containerdutils/hostpaths.go
Outdated
| if err != nil && strings.Contains(err.Error(), "no such file or directory") { | ||
| err = container.Delete(ctx, containerd.WithSnapshotCleanup) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to delete container to evaluate host dir %s: %s", target, err) |
There was a problem hiding this comment.
use errors.Wrapf here too
cli/containerdutils/hostpaths.go
Outdated
| var config map[string]interface{} | ||
| err = json.Unmarshal(configFileData, &config) | ||
| if err != nil { | ||
| return "", fmt.Errorf("Malformed config file - %s: %s", configFile, err) |
There was a problem hiding this comment.
use errors.Wrapf here too
cli/command/engine/activate.go
Outdated
| flags.StringVarP(&options.engineVersion, "version", "", "", "Specify engine version (default is to use existing version)") | ||
| flags.StringVarP(&options.registryPrefix, "registry-prefix", "", "docker.io/docker", "Override the default location where engine images are pulled") | ||
| flags.StringVarP(&options.format, "format", "", "", "Pretty-print licenses using a Go template") | ||
| flags.BoolVarP(&options.quiet, "queit", "q", false, "Only display available licenses by ID") |
There was a problem hiding this comment.
This needs a deeper review, but so far my comments are generally:
- Functionality needs ownership. Right now all functionality is scoped to a package when it should be scoped to a type.
- Errors, especially in new functionality that's added here, should be well typed so they can be properly inspected (e.g. not by checking the message itself).
- Interfaces are quite broad, both in the new interfaces being used and the interfaces being passed in to the new functionality. This should be scoped down so, for instance, a function that just needs an
io.Writerdoesn't need the whole docker CLI object. - The
fakeClientapproach where there is a struct that does nothing but store configured functions is not very clean and largely redundant. Might as well just make new objects with the needed functionality. Note you can have a base struct that provides "not implemented" sort of behavior and wrap that with specific functionality for what's being tested... but also scoping interfaces down should help a lot here.
cli/command/engine/init.go
Outdated
| return fmt.Errorf("Unable to access local containerd: %s", err) | ||
| } | ||
| defer cclient.Close() | ||
| return containerdutils.InitEngine(ctx, dockerCli, dclient, cclient, options.image, options.version, options.registryPrefix, options.configFile) |
There was a problem hiding this comment.
Also wy are we passing both client and dockerCli? There's a few places this is done.
cli/command/registry/login.go
Outdated
| @@ -126,7 +130,13 @@ func runLogin(dockerCli command.Cli, opts loginOptions) error { //nolint: gocycl | |||
|
|
|||
| response, err = clnt.RegistryLogin(ctx, *authConfig) | |||
| if err != nil { | |||
| return err | |||
| if strings.Contains(err.Error(), "annot connect to ") { | |||
There was a problem hiding this comment.
Can we get a real error type?
cli/containerdutils/client_test.go
Outdated
| ) | ||
|
|
||
| type ( | ||
| fakeDockerClient struct { |
There was a problem hiding this comment.
I'm not sure I see the purpose in these structs that do nothing except take functionality from other things.
Why not provide specific structs? If the interface you are trying to satisfy is too large, can't we narrow down the interface?
There was a problem hiding this comment.
It's purely a testing abstraction. I looked around in this repo to find a pattern to emulate with the hope that I'd be following the preferred style and saw https://github.com/docker/cli/blob/master/cli/command/manifest/client_test.go which uses this model.
@cpuguy83 can you point me to a different/better reference you would prefer?
If this model is explicitly not what we want people to follow, could a maintainer put a comment in those files noting that those tests should be refactored so others don't fall into the same trap I did thinking this pattern is OK?
cli/containerdutils/containerd.go
Outdated
| } | ||
|
|
||
| // GetContainerdClient verifies the containerd socket is present and writable by the current user | ||
| func GetContainerdClient() (*containerd.Client, error) { |
There was a problem hiding this comment.
Seems like we shouldn't bother with this.
cli/containerdutils/containerd.go
Outdated
| } | ||
|
|
||
| // PullWithAuth will use the credential cache auth to pull via containerd | ||
| func PullWithAuth(cli command.Cli, client WrappedContainerdClient, imageName string) error { |
There was a problem hiding this comment.
This is all too much.
Let's consider what type should own this behavior.... e.g. a wrapped client.
cli/containerdutils/types.go
Outdated
| EnterpriseEngineImage = "engine-enterprise" | ||
|
|
||
| // EngineSpec is the default container spec for the docker engine | ||
| EngineSpec = specs.Spec{ |
There was a problem hiding this comment.
Seems like this should be a function to get the default spec (since that's the only way it could be immutable)...
This will probably work, though.
cli/containerdutils/types.go
Outdated
|
|
||
| type ( | ||
| // WrappedContainerdClient abstracts the containerd client to aid in testability | ||
| WrappedContainerdClient interface { |
There was a problem hiding this comment.
Should also be scoped to the call site, not the package.
cli/licenseutils/client_test.go
Outdated
| ) | ||
|
|
||
| type ( | ||
| fakeLicensingClient struct { |
There was a problem hiding this comment.
Same comment as above for other of these fake clients.
cli/licenseutils/types.go
Outdated
| LicensingPublicKey = "LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0Ka2lkOiBKN0xEOjY3VlI6TDVIWjpVN0JBOjJPNEc6NEFMMzpPRjJOOkpIR0I6RUZUSDo1Q1ZROk1GRU86QUVJVAoKTUlJQ0lqQU5CZ2txaGtpRzl3MEJBUUVGQUFPQ0FnOEFNSUlDQ2dLQ0FnRUF5ZEl5K2xVN283UGNlWSs0K3MrQwpRNU9FZ0N5RjhDeEljUUlXdUs4NHBJaVpjaVk2NzMweUNZbndMU0tUbHcrVTZVQy9RUmVXUmlvTU5ORTVEczVUCllFWGJHRzZvbG0ycWRXYkJ3Y0NnKzJVVUgvT2NCOVd1UDZnUlBIcE1GTXN4RHpXd3ZheThKVXVIZ1lVTFVwbTEKSXYrbXE3bHA1blEvUnhyVDBLWlJBUVRZTEVNRWZHd20zaE1PL2dlTFBTK2hnS1B0SUhsa2c2L1djb3hUR29LUAo3OWQvd2FIWXhHTmw3V2hTbmVpQlN4YnBiUUFLazIxbGc3OThYYjd2WnlFQVRETXJSUjlNZUU2QWRqNUhKcFkzCkNveVJBUENtYUtHUkNLNHVvWlNvSXUwaEZWbEtVUHliYncwMDBHTyt3YTJLTjhVd2dJSW0waTVJMXVXOUdrcTQKempCeTV6aGdxdVVYYkc5YldQQU9ZcnE1UWE4MUR4R2NCbEp5SFlBcCtERFBFOVRHZzR6WW1YakpueFpxSEVkdQpHcWRldlo4WE1JMHVrZmtHSUkxNHdVT2lNSUlJclhsRWNCZi80Nkk4Z1FXRHp4eWNaZS9KR1grTEF1YXlYcnlyClVGZWhWTlVkWlVsOXdYTmFKQitrYUNxejVRd2FSOTNzR3crUVNmdEQwTnZMZTdDeU9IK0U2dmc2U3QvTmVUdmcKdjhZbmhDaVhJbFo4SE9mSXdOZTd0RUYvVWN6NU9iUHlrbTN0eWxyTlVqdDBWeUFtdHRhY1ZJMmlHaWhjVVBybQprNGxWSVo3VkQvTFNXK2k3eW9TdXJ0cHNQWGNlMnBLRElvMzBsSkdoTy8zS1VtbDJTVVpDcXpKMXlFbUtweXNICjVIRFc5Y3NJRkNBM2RlQWpmWlV2TjdVQ0F3RUFBUT09Ci0tLS0tRU5EIFBVQkxJQyBLRVktLS0tLQo=" | ||
| ) | ||
|
|
||
| type ( |
There was a problem hiding this comment.
I'm pretty sure I've never seen this done.... not that it shouldn't be done, just that it's not idiomatic in the sense that none of the other code does this.
Can we stick to type Foo T?
vendor.conf
Outdated
| @@ -85,4 +85,15 @@ k8s.io/client-go kubernetes-1.11.0 | |||
| k8s.io/kube-openapi d8ea2fe547a448256204cfc68dfee7b26c720acb | |||
| k8s.io/kubernetes v1.11.0 | |||
| vbom.ml/util 256737ac55c46798123f754ab7d2c784e2c71783 | |||
|
|
|||
| github.com/containerd/fifo 3d5202a | |||
There was a problem hiding this comment.
Also, is this used? Don't see why we'd need fifo here.
|
It would be nice if there was an issue, or I suppose a comment in this PR, describing this change (what new commands are, what are they for, demo output, etc). |
cli/command/engine/check.go
Outdated
| import ( | ||
| "fmt" | ||
|
|
||
| "golang.org/x/net/context" |
There was a problem hiding this comment.
it's just "context" now
There was a problem hiding this comment.
Thanks - I'll clean this up - I started this work quite a while back and I think the CLI was still using that pattern when I started... just overlooked switching these as I rebased.
cli/command/engine/check.go
Outdated
| } | ||
|
|
||
| imageName := options.registryPrefix + "/" + currentEngine | ||
| versions, err := containerdutils.GetEngineVersions(context.Background(), dockerCli, currentVersion, imageName, options.all) |
There was a problem hiding this comment.
pass ctx instead of context.Background() here
cli/containerdutils/containerd.go
Outdated
| // GetContainerdClient verifies the containerd socket is present and writable by the current user | ||
| func GetContainerdClient() (*containerd.Client, error) { | ||
| // Check for presence first | ||
| _, err := os.Stat(ContainerdSockPath) |
There was a problem hiding this comment.
tisk tisk ;), connect and handle errors on connect os.IsNotExist, not check first. These types of checks are racy
cli/containerdutils/containerd.go
Outdated
| fmt.Fprintf(cli.Out(), "Pulling %s... ", imageName) | ||
| ctx := namespaces.WithNamespace(context.Background(), EngineNamespace) | ||
|
|
||
| _, err = client.Pull(ctx, imageName, containerd.WithResolver(resolver), containerd.WithPullUnpack) |
There was a problem hiding this comment.
If you want progress, you can use content.Fetch from containerd
Good suggestion. Let me take a look at what it would take to wire that up.
Yes, that was the intention but it seems you found a bug - I'll investigate this further and figure out what's going wrong and add a test case for this.
If you're just deploying a community engine from hub it works without a login. If you're using an alternative registry (e.g., a DTR behind a firewall) then you might need to login to pull images. If you're initializing an enterprise engine from hub you'll need to be logged in as well. I tried to wire this up so it's not mandatory, but so we can support initialization from a private repo if necessary. Does that help address your concern, or are you still concerned with this model?
Good point. That wasn't intentional. I forgot to add the image flag to the update flows when I added it recently to the init flow.
I'll see what I can do. :-)
Let me coordinate with @crosbymichael and @dmcgowan and see what we can come up with.
👍 I'll clean this up.
Will do - I'll reduce the set of exported components in my next refactoring pass.
\cc @mason-fish ^^ |
👍
👍
👍
I thought I was following existing patterns, but if there's a different preferred model I can adjust the implementation. @cpuguy83 can you point to example's as reference in the code of how you'd like me to structure the test adapters? |
If you need to implement many methods, then I'd use something like this: type FooBar interface {
Foo() error
Bar() error
}
type baseFoo struct {}
func (baseFoo) Foo() error { return errors.New("not implemented") }
func(baseFoo) Bar() error { return errors.New("not implemented") }
type withFoo struct {
baseFoo
}
func(withFoo) Foo() error {
return nil
}But I would also try and reduce the things I need to mock out as much as possible, which means defining interfaces locally for the methods you are actually calling rather than using a concrete type or some (large) external interface. |
The following is a little dated and some changes have been made to the implementation based on UX feedback, but it shows the basic flow. Problem AbstractThe Docker EE platform is distributed and the software lifecycle This PR enables the docker engine to run as a privileged container running UX ExampleThe following examples outline how users would interact with the local Before InitializationThis new version of the docker CLI would be capable of initializing Initializing a Local CE EngineProvided containerd is present on the local system and the user has the Notes:
Activating EEIf the user wants to start running an EE engine they would use the The following example shows the UX for a brand-new customer who has never logged into hub Notes:
The following variation shows the UX for an existing customer with some existing licenses. Seeing current engine statusThe info and version commands should be updated to expose if the engine Notes:
Detecting Updates are AvailableChecking for updates requires scanning hub repos for new tags, and requires Notes:
Updating a Local Engine |
Hmm... maybe I'm just not seeing it, but isn't this pattern going to result in a massive explosion in the amount of code necessary to mock out various different scenarios? Wouldn't I then have to create unique types for each scenario? That seems pretty heavy weight. The nice thing about the existing pattern I replicated is it only gets involved in the test code, not the mainline production code, and each test case can easily alter the behavior of any routine. Do you have a good example in the tree that's following your pattern for test cases I can look more closely at to see how it should map? Since the comment's now hidden - some more explanation on why I used this pattern is here #1260 (comment) |
|
@vdemeester @cpuguy83 I've pushed two new commits that should hopefully address almost all your comments. I still need to add some unit test coverage for the engine package, and I'm still not sure what the preferred mock model is in this tree. Update: I forgot to mention I haven't made substantive changes to the licensing portion yet, as the upstream licensing lib work is still WIP - hoping we'll have a bump there in the next few days and then I can update the related glue code in this PR. |
cli/command/engine/activate.go
Outdated
|
|
||
| flags.StringVar(&options.licenseFile, "license", "", "License File") | ||
| flags.StringVar(&options.version, "version", "", "Specify engine version (default is to use existing version)") | ||
| flags.StringVar(&options.registryPrefix, "registry-prefix", "docker.io/docker", "Override the default location where engine images are pulled") |
There was a problem hiding this comment.
Wouldn't this make more sense to have it as just docker.io/ and let the engine-image contain the prefix for the registry organization?
There was a problem hiding this comment.
This applies to all other uses of the registry-prefix
There was a problem hiding this comment.
I think this boils down to a question of use-case and when these flags will be used. I was trying to optimize the UX around end-user use-cases to support local mirrors, while enabling developer use-cases.
Specifically, if you're a typical user, I don't think you'll ever use --engine-image - the default behavior for the community engine and enterprise engine scenarios will do the right thing. This flag is really only meant for developers who want to deviate from the standard naming scheme.
Many customers run an internal registry in their firewalled environments, and the intention with the --registry-prefix flag was to give them a flag to establish the prefix where they store all their docker product mirrored images, both community and enterprise engines, and in the future, additional products like UCP and DTR itself.
My concern with splitting it the way you suggest is I think it makes it harder for end-users using local registry mirrors to benefit from the automatic community/enterprise logic. These mirroring customer would always have to pin the exact image name to use a mirror. In this version of the engine, we're using a single image so it seems ~simple, but it's entirely possible as we work on the modularization work in moby (buildkit, etc.) we will start to split the engine into multiple images in the future. When/if we do so, then we'd have to add new flags for developers to select the other component images, but one could see this getting out of control for a typical user. By using a common prefix and letting the CLI select the right image names by default using the common prefix we hide that complexity of multiple component images from the typical end user. It also will make it harder for us to support this mirroring pattern for other products down the road. For example, UCP today doesn't support installing from a mirror, and UCP itself is made up of a dozen plus images already. I'd like to see us follow this pattern so someday you can docker ucp install --registry-prefix mylocalregistry.acme.com/docker ... (or something along those lines) where the registry-prefix is common for all the mirrored content.
There was a problem hiding this comment.
λ ./docker engine init --config-file=/tmp/etc/docker
docker engine init is only supported on a Docker cli with experimental cli features enabled
λ cat ~/.docker/config.json
{
// […]
"experimental": "enabled"
}
Getting a weird error on my experimental enabled cli 😝
cli/command/engine/activate.go
Outdated
| } | ||
|
|
||
| func getLicenses(ctx context.Context, cli command.Cli, options activateOptions) (*model.IssuedLicense, error) { | ||
| user, err := licenseutils.Login(ctx, cli, options.registryPrefix) |
There was a problem hiding this comment.
Pretty sure Login doesn't need to get the whole command.Cli interface as argument.
cli/command/engine/check.go
Outdated
| } | ||
|
|
||
| imageName := options.registryPrefix + "/" + currentEngine | ||
| versions, err := client.GetEngineVersions(ctx, dockerCli.RegistryClient(false), currentVersion, imageName, options.all) |
There was a problem hiding this comment.
Not really fan of boolean args as it doesn't really provide any insight of what we read (i.e. reading only that line, I have no idea what false mean for this func.)
cli/command/engine/check.go
Outdated
| // filter out our current version | ||
| for _, ver := range versions.Patches { | ||
| if ver.Tag != currentVersion { | ||
| availUpdates = append(availUpdates, containerizedengine.Update{ |
There was a problem hiding this comment.
these append can be extracted in a function I think 😉
cli/command/engine/client_test.go
Outdated
| activateEngineFunc func(ctx context.Context, | ||
| opts containerizedengine.EngineInitOptions, | ||
| out containerizedengine.OutStream, | ||
| authResolver func( |
There was a problem hiding this comment.
This could definitely be a local type (at least)
type resolver func(ctx Context.Context, index *registrytypes.IndexInfo) type.AuthConfig
// …
activateEngineFunc func(ctx context.Context, opts …EngineInitOptions, out …OutStream, authResolver resolver) errorThere was a problem hiding this comment.
Not that really, we should just pass it the types.AuthConfig instead and not require the func/wrapper 😉
cli/command/registry/login.go
Outdated
| @@ -126,7 +126,13 @@ func runLogin(dockerCli command.Cli, opts loginOptions) error { //nolint: gocycl | |||
|
|
|||
| response, err = clnt.RegistryLogin(ctx, *authConfig) | |||
| if err != nil { | |||
| return err | |||
| if client.IsErrConnectionFailed(err) { | |||
There was a problem hiding this comment.
This flow is getting weirder and weirder to read. (from command.GetDefaultAuthConfig to the last if err != nil line 133). We may want to refactor that a bit 👼
cli/containerizedengine/update.go
Outdated
|
|
||
| // DoUpdate performs the underlying engine update | ||
| func (c baseClient) DoUpdate(ctx context.Context, opts EngineInitOptions, out OutStream, | ||
| authResolver func(ctx context.Context, index *registrytypes.IndexInfo) types.AuthConfig) error { |
There was a problem hiding this comment.
Same as above types.AuthConfig instead of the resolver func.
cli/containerizedengine/update.go
Outdated
| return fmt.Errorf("please pick the version you want to update to") | ||
| } | ||
|
|
||
| imageName := fmt.Sprintf("%s/%s:%s", opts.RegistryPrefix, opts.EngineImage, opts.EngineVersion) |
There was a problem hiding this comment.
Same as above, if we use opts only for that, we're better of passing the image reference string directly
cli/licenseutils/utils.go
Outdated
| return &license, nil | ||
| } | ||
|
|
||
| func getRegistryAuth(cli command.Cli, registryPrefix string) (*types.AuthConfig, error) { |
There was a problem hiding this comment.
We should pass a types.AuthConfig to this package instead of having to define this one. This would remove a lot of package deps (containerizedengine, trust, …)
pkg/containerized/signal_unix.go
Outdated
|
|
||
| var ( | ||
| // SIGKILL maps to unix.SIGKILL | ||
| SIGKILL = unix.SIGKILL |
There was a problem hiding this comment.
Those are already defined above (and exported too it seems), We should define them only once and use on both (or… not defined them as exported)
pkg/containerized/snapshot.go
Outdated
| } | ||
| */ | ||
|
|
||
| // nolint: interfacer |
There was a problem hiding this comment.
Why ignoring the linter here ? I guess it's on client *containerd.Client that could only be client oci.Client (or even a local interface)
There was a problem hiding this comment.
Yeah, it wanted me to change the types to be incompatible with the upstream containerd pattern which I think is wrong.
There was a problem hiding this comment.
how is it incompatible with the upstream containerd pattern ? it just says there is a small interface you can "declare your variable with". The other "way around" would be to define a local interface that satisfy the contract you need 😝
cli/command/engine/activate.go
Outdated
| quiet bool | ||
| } | ||
|
|
||
| // NewActivateCommand creates a new `docker engine activate` command |
There was a problem hiding this comment.
nit: it should be newActivateCommand
cli/command/engine/activate.go
Outdated
| Long: `Activate Enterprise Edition. | ||
|
|
||
| With this command you may apply an existing Docker enterprise license, or | ||
| interactively download one from Docker. In the interactive exchange, you can |
There was a problem hiding this comment.
nit: . In <- Extra space
cli/command/engine/activate.go
Outdated
|
|
||
| With this command you may apply an existing Docker enterprise license, or | ||
| interactively download one from Docker. In the interactive exchange, you can | ||
| sign up for a new trial, or download an existing license. If you are currently |
cli/command/engine/activate.go
Outdated
| support. | ||
|
|
||
| For more information about different Docker Enterprise license types visit | ||
| http://www.docker.com/licenses |
There was a problem hiding this comment.
Use https instead?
By the way the link seems to be broken...
Maybe move this part into another PR, ready to be merged as soon as the page is online?
cli/command/engine/activate.go
Outdated
| flags := cmd.Flags() | ||
|
|
||
| flags.StringVar(&options.licenseFile, "license", "", "License File") | ||
| flags.StringVar(&options.version, "version", "", "Specify engine version (default is to use existing version)") |
There was a problem hiding this comment.
I think existing version can be misleading. Do you mean current version? Maybe installed version or running version instead?
cli/command/formatter/updates.go
Outdated
| } | ||
|
|
||
| // UpdatesWrite writes the context | ||
| func UpdatesWrite(ctx Context, availUpdates []containerizedengine.Update) error { |
There was a problem hiding this comment.
nit: availUpdates -> availableUpdates
| context Context | ||
| expected string | ||
| }{ | ||
|
|
| {Type: "updateType1", Version: "version1", Notes: "description 1"}, | ||
| {Type: "updateType2", Version: "version2", Notes: "description 2"}, | ||
| } | ||
| out := bytes.NewBufferString("") |
There was a problem hiding this comment.
nit: out := &bytes.Buffer{}
| {"Version": "version2", "Notes": "note2", "Type": "updateType2"}, | ||
| } | ||
|
|
||
| out := bytes.NewBufferString("") |
cli/command/registry/login.go
Outdated
| Status: status, | ||
| IdentityToken: token, | ||
| }, err | ||
|
|
|
|
||
| func (c baseClient) pullWithAuth(ctx context.Context, imageName string, out OutStream, | ||
| authResolver func(ctx context.Context, index *registrytypes.IndexInfo) types.AuthConfig) (containerd.Image, error) { | ||
|
|
|
|
||
| distributionRef, err := reference.ParseNormalizedNamed(imageName) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to parse image name: %s: %s", imageName) |
There was a problem hiding this comment.
Why are there 2 '%s'? Is it missing an argument?
| } | ||
| imgRefAndAuth, err := trust.GetImageReferencesAndAuth(ctx, nil, authResolver, distributionRef.String()) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to get imgRefAndAuth") |
There was a problem hiding this comment.
I don't think imgRefAndAuth in the message will help a user...
cli/containerizedengine/progress.go
Outdated
| continue | ||
| } | ||
| // update status of active entries! | ||
| for _, active := range active { |
There was a problem hiding this comment.
shadowing active with another active variable makes the code not easy to review...
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| var ( |
cli/containerizedengine/hostpaths.go
Outdated
| if err != nil { | ||
| logrus.Debugf("Failed to kill task: %s", err) | ||
| } | ||
| _, err = task.Delete(ctx) |
There was a problem hiding this comment.
if _, err := ... ; err != nil{
cli/containerizedengine/hostpaths.go
Outdated
| logrus.Debugf("Failed to delete task: %s", err) | ||
| } | ||
| err = container.Delete(ctx, containerd.WithSnapshotCleanup) | ||
| if err != nil { |
There was a problem hiding this comment.
if err := ... ; err != nil{
cli/containerizedengine/hostpaths.go
Outdated
| configFileData := buf.Bytes() | ||
|
|
||
| var config map[string]interface{} | ||
| err = json.Unmarshal(configFileData, &config) |
There was a problem hiding this comment.
if err := ... ; err != nil{
cli/containerizedengine/hostpaths.go
Outdated
| return "", errors.Wrapf(err, "malformed config file - %s", configFile) | ||
| } | ||
| rootDir := defaultDockerRootDir | ||
| tmp, defined := config["data-root"] |
There was a problem hiding this comment.
if data, defined := config["data-root"]; defined{
rootDir = data.(string)
}
cli/containerizedengine/update.go
Outdated
| imageName := fmt.Sprintf("%s/%s:%s", opts.RegistryPrefix, opts.EngineImage, opts.EngineVersion) | ||
|
|
||
| // Look for desired image | ||
| image, err := c.cclient.GetImage(ctx, imageName) |
There was a problem hiding this comment.
This pattern seems to be the same as in baseClient.InitEngine, it could be extracted and factorized.
Hmm... I'm a bit confused by what's going on here. I tried switching the |
|
@vdemeester @cpuguy83 I've added one commit that refactors one of the test mocks using the model that I think you are proposing. - 9d73826 Please take a look at that and let me know what you think. My preference is the original model as I think its easier to reason about, but if this is how you'd like the tests structured, I'll apply it to the other mocks in a follow up commit. |
cli/command/engine/auth.go
Outdated
| ) | ||
|
|
||
| func getRegistryAuth(cli command.Cli, registryPrefix string) (*types.AuthConfig, error) { | ||
| fullName := registryPrefix + "/" + containerizedengine.EnterpriseEngineImage |
There was a problem hiding this comment.
This seems wrong — the EntrepriseEngineImage there 🤔 It's gonna get use in docker engine init too which by default use the Community image — we should pass the image too (and thus probably, at that point, just a reference string 👼)
There was a problem hiding this comment.
Also, if no registryPrefix, the reference will be /something right (looking at the default value in options), and thus an invalid reference, isn't it ?
| // This client can be used to manage the lifecycle of | ||
| // dockerd running as a container on containerd. | ||
| func NewClient() (Client, error) { | ||
| cclient, err := containerd.New(containerdSockPath) |
There was a problem hiding this comment.
One question on that, we don't want to allow customization of the containerdSockPath when engine init ?
|
|
||
| err := updateNonActive(ctx, ongoing, cs, statuses, keys, activeSeen, &done, start) | ||
| if err != nil { | ||
| continue outer |
There was a problem hiding this comment.
Shouldn't this return an error instead ? 🤔 (this would make outer: not required anymore)
There was a problem hiding this comment.
This algorithm has been structured this way for the past year (or more) in the containerd tree, so i'm a little hesitant to start changing it significantly without understanding all the partial failure modes.
cli/updates/types.go
Outdated
| package updates | ||
|
|
||
| // Update stores available updates for rendering in a table | ||
| type Update struct { |
There was a problem hiding this comment.
|
In addition, not a huge fan of the proposed test refactoring, and as most of the code is under |
Sounds good. Thanks! I'll drop the test refactoring commit, make the other requested changes, get a vendor bump of the licensing lib and add some unit test coverage on the licensing side, and get that pushed later today. Hopefully that will get us in a state where this is mergeable with only minor comments, then I'll submit a follow up clean-up PR after FC to address those. |
c00bfd9
to
5a95df7
Compare
There was a problem hiding this comment.
I'm getting some weird errors :thinking:
λ sudo ./build/docker-linux-amd64 --config=/home/vincent/.docker engine rm
runtime for task io.containerd.runtime.process.v1: rpc error: code = NotFound desc = unknown runtime "io.containerd.runtime.process.v1": unknown
λ sudo ctr -n docker c ls
CONTAINER IMAGE RUNTIME
dockerd docker.io/docker/engine-community:18.09.0-dev io.containerd.runtime.process.v1
Also, on update, I was "waiting" for a list to choose from 😝
λ sudo ./build/docker-linux-amd64 --config=/home/vincent/.docker engine update
please pick the version you want to update to
λ sudo ./build/docker-linux-amd64 --config=/home/vincent/.docker engine update --version 18.09.0-dev
failed to lookup task: runtime for task io.containerd.runtime.process.v1: rpc error: code = NotFound desc = unknown runtime "io.containerd.runtime.process.v1": unknown
Also, the first time I did the init, it told me the daemon is responsive really quick because I already had a daemon running… didn't really check the daemon answering was the same it spawned (using version or sthg) — there is not too much log in debug mode so it's a bit tricky to know what going wrong 😓
| } | ||
| return client.InitEngine(ctx, options.EngineInitOptions, dockerCli.Out(), authConfig, | ||
| func(ctx context.Context) error { | ||
| client := dockerCli.Client() |
There was a problem hiding this comment.
This doesn't take the engineInitOptions into account at all — i.e. if the the engine init configuration file changes the default host the daemon listen on, … — it will try to ping the default docker daemon. This kinda force to pass -H to the docker engine init command in those cases.
There was a problem hiding this comment.
For the scenario where the daemon config is set up to listen on a non-standard sock, or only tcp, the user should also set their DOCKER_HOST to match, however I agree we should attempt to detect a misconfiguration here so we can give a good error message if they're wrong.
There was a problem hiding this comment.
Alright, managed to make it work 👼 🎉
So there is still a few comments to take into account (some errors.Wrap and some more complex ones), but as said before, as this is experimental and most of the code happens in internal package, I'm fine doing those as follow-ups 😉
LGTM 🐯
Signed-off-by: Daniel Hiltgen <[email protected]>
This new collection of commands supports initializing a local engine using containerd, updating that engine, and activating the EE product Signed-off-by: Daniel Hiltgen <[email protected]>
|
Squashed to 2 commits (vendor bump, and the impl) and rebased on master so it should be ready to merge once CI goes green. |
|
LGTM |
- What I did
This PR adds support to the Docker CLI to manage the lifecycle of a Docker Engine running as a privileged container on top of containerd. New CLI UX is introduced to initialize a local engine, check for available updates, and update the local engine. Additional UX is introduced to allow users to transition from a community based engine to an enterprise engine if they so desire, by downloading an existing enterprise license from the Docker Store or initiating a new free trial license.
Not included in this PR are packaging level changes to provide a streamlined user experience based on OS level packages, which allow users to continue to use OS level packages and related automation if they desire for installation and update operations.
- How I did it
The implementation leverages the containerd client API and interacts with a local containerd to download images from a registry (defaulting to Docker Hub) as well as a docker licensing library to interact with Docker Store for generating trial licenses and downloading existing licenses.
- How to verify it
Unit tests and a new e2e test are included in the PR. The e2e test runs a nested containerd in a container upon which a nested dockerd is launched.
Nightly builds for the engine will be published at https://hub.docker.com/r/docker/engine-community/tags/
Manual validation may be performed by installing containerd locally, then running this CLI, specifying a nightly version tag. For example:
- Description for the changelog
Add support for running dockerd on top of containerd
- A picture of a cute animal (not mandatory but encouraged)