-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fast context switch: commands #1501
Conversation
8bf1f8b
to
a08fd72
Compare
cli/command/system/version.go
Outdated
@@ -22,6 +22,7 @@ import ( | |||
|
|||
var versionTemplate = `{{with .Client -}} | |||
Client:{{if ne .Platform.Name ""}} {{.Platform.Name}}{{end}} | |||
Context: {{.Context}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we want the context
here in the version
command or in the info
? 🤔
@thaJeztah any thought?
569039e
to
47eb162
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Really not fan of the
dockerEndpointOptions
andkubernetesEndpointOptions
struct… - Not sure for the command either :
docker context set-docker-endpoint …
vsdocker context set docker-endpoint …
, same with otherset-*
options… we may want to have more subcommands… This would allowdocker context set defaultStackOrchestrator
commands (targeting current context if no context is provided via a flag 👼 )
Linter complains
|
One test is failing:
|
f05b40b
to
d81004d
Compare
aa13ee6
to
bba053c
Compare
Codecov Report
@@ Coverage Diff @@
## master #1501 +/- ##
==========================================
- Coverage 55.32% 55.16% -0.16%
==========================================
Files 283 301 +18
Lines 19330 20359 +1029
==========================================
+ Hits 10694 11232 +538
- Misses 7939 8323 +384
- Partials 697 804 +107 |
458c27f
to
1644ef1
Compare
@simonferquel when you get the chance, can you update the description with any user interaction changes? just a simple example of what is the new command or option or output from docker version or docker info or whatnot would help describe to the end user what this PR will bring it because there's a lot of code changes in this one. |
i talked with @simonferquel @silvin-lubecki @thaJeztah about this and opened up an issue to describe the bigger idea and reasoning and track outstanding issues #1586 |
cli/command/cli.go
Outdated
if config != nil && config.CurrentContext != "" { | ||
_, err := contextstore.GetContextMetadata(config.CurrentContext) | ||
if store.IsErrContextDoesNotExist(err) { | ||
warnCurrentContextNotFound(config.CurrentContext, stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this one: I don't think we should do the automatic fallback, but instead make it a hard fail. Falling back can be dangerous, for example:
docker --context=develop container rm -f somecontainer
WARNING: current context "develop" is not found on filesystem. Falling back to default Docker endpoint
And now you removed the container from "whatever happened to be in DOCKER_HOST
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Erf, you are right, but this one will be tricky: docker context use implicitly requires an initialized dockerCli.
As this error can occur only if the user messes directly with the store dir, is it ok if I do a hard fail, indicating that he should fix his configfile manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to grasp your comment;
All the docker context
commands are local operations, so (technically) should not need to care about the current context. i.e, if the "current" context uses unix:///no-such-thing
), it should not be an error until I try to connect to dockerd or the k8s API, so docker context use <a valid context>
should work (as it doesn't require a context to perform that action).
I will go for
The idea is to be able to use it with other tools like kubectl etc.
ok for _path suffixed fields |
cli/command/context/options.go
Outdated
@@ -2,6 +2,7 @@ package context | |||
|
|||
import ( | |||
"fmt" | |||
"github.com/docker/docker/client" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: imports reordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Getting close; left some comments inline 🤗
cli/command/cli.go
Outdated
if config != nil && config.CurrentContext != "" { | ||
_, err := contextstore.GetContextMetadata(config.CurrentContext) | ||
if store.IsErrContextDoesNotExist(err) { | ||
warnCurrentContextNotFound(config.CurrentContext, stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to grasp your comment;
All the docker context
commands are local operations, so (technically) should not need to care about the current context. i.e, if the "current" context uses unix:///no-such-thing
), it should not be an error until I try to connect to dockerd or the k8s API, so docker context use <a valid context>
should work (as it doesn't require a context to perform that action).
defer cleanup() | ||
createTestContextWithKube(t, cli) | ||
withPipeInOut(closeChan)(cli) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this needed?
edit: ah, looks like you updated in a follow-up commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep it is not needed anymore (and removed)
cli/command/context/list_test.go
Outdated
golden.Assert(t, cli.OutBuffer().String(), "quiet-list.golden") | ||
} | ||
|
||
func TestInspect(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this one is in the wrong file (should probably be in inspect_test.go
)
cli/command/context/list.go
Outdated
} | ||
|
||
flags := cmd.Flags() | ||
flags.StringVar(&opts.format, "format", formatter.TableFormatKey, "Pretty-print contexts using a Go template") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (for a follow-up); for other commands we use ""
as default, and in the code use something like;
format := opts.format
if len(format) == 0 {
format = formatter.TableFormatKey
}
e.g.
Lines 45 to 48 in a900ba8
format := opts.format | |
if len(format) == 0 { | |
format = formatter.TableFormatKey | |
} |
We can do a follow-up to decide which approach to take for all (either way makes some sense)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for a followup
golden.Assert(t, cli.OutBuffer().String(), "list.unset.golden") | ||
} | ||
|
||
func TestListNoContext(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the default
context still disappears after selecting a context. Can we make the default
context always visible in docker context ls
?
docker context ls
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock swarm
foobar unix:///var/run/docker.sock
docker context use foobar
foobar
Current context is now "foobar"
docker context ls
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
foobar * unix:///var/run/docker.sock
docker context use default
default
Current context is now "default"
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock swarm
foobar unix:///var/run/docker.sock
return errors.New("context name cannot be empty") | ||
} | ||
if name == "default" { | ||
return errors.New(`"default" is a reserved context name`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (ok for a follow-up): we should probably handle this with for other situations as well. Perhaps when using inspect; return the information that we obtained from the default (DOCKER_HOST). Currently;
docker context inspect default
[]
context "default" does not exist
docker context rm default
default: context "default" does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about that: for now, when a context is set and I list the contexts, the default
context appears with no docker/kubernetes endpoint. Not sure if Inspect makes sense.
Maybe indicate that the default
context cannot be inspected ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it's ok to produce a better /helpful error for now (then we can revisit in future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in fact part of the latest commit :)
} | ||
|
||
// IsErrContextDoesNotExist checks if the given error is a "context does not exist" condition | ||
func IsErrContextDoesNotExist(err error) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for a follow-up; use an interface for these (like the errdefs
package (or use that package); https://github.com/moby/moby/blob/d48392a35b157612d28c48eb8e4fad1272fa1442/errdefs/defs.go#L3-L6
// ErrNotFound signals that the requested object doesn't exist
type ErrNotFound interface {
NotFound()
}
// IsNotFound returns if the passed in error is an ErrNotFound
func IsNotFound(err error) bool {
_, ok := getImplementer(err).(ErrNotFound)
return ok
}
// NotFound is a helper to create an error of the class with the same name from any error type
func NotFound(err error) error {
if err == nil {
return nil
}
return errNotFound{err}
}
It would make these errors consistent with other handling, and also preserve the original error in some cases where it may be relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the docker context commands are local operations, so (technically) should not need to care about the current context. i.e, if the "current" context uses unix:///no-such-thing), it should not be an error until I try to connect to dockerd or the k8s API, so docker context use should work (as it doesn't require a context to perform that action).
We don't need to do any request, but the initialization of the CLI still occur. And thus it parses the Host to create the WithHost option (without issuing request). so unix:///some-not-existing-file
work (it is valid syntactically), but unix///malformed
(note the missing :
) fails with a parsing error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arf, yes.
We'll have to think of a solution for that (e.g., if we can delay evaluating that option until it's actually needed 🤔 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I now validate the Host on create/update so it should not be a problem anymore. Only case this error can happen, is if the user delete its store content manually but keep the currentContext set in its config file
a3d2a5a
to
394482e
Compare
Signed-off-by: Simon Ferquel <[email protected]>
ping @thaJeztah, can you PTAL the last commit ? |
If I do a full Giving this another spin docker context ls
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock swarm docker context ls --format '{{json .}}' | jq .
{
"Current": true,
"Description": "Current DOCKER_HOST based configuration",
"DockerEndpoint": "unix:///var/run/docker.sock",
"KubernetesEndpoint": "",
"Name": "default",
"StackOrchestrator": "swarm"
} Formatting placeholders work as expected 👍 : docker context ls --format '{{json .Name}}'
"default" Validation works docker context create --docker host=unix:/bla local
unable to create docker endpoint config: unable to apply docker endpoint options: unable to parse docker host `unix:/bla`
docker context create --docker host=unix:///var/run/docker.sock default
"default" is a reserved context name
docker context create --docker host=unix:///var/run/docker.sock local
local
Successfully created context "local"
docker context create --docker host=unix:///var/run/docker.sock local
context "local" already exists We could even consider showing the same error message when trying to create a context named
docker context ls
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
default * Current DOCKER_HOST based configuration unix:///var/run/docker.sock swarm
local unix:///var/run/docker.sock
docker context use local
local
Current context is now "local"
docker context ls
NAME DESCRIPTION DOCKER ENDPOINT KUBERNETES ENDPOINT ORCHESTRATOR
default Current DOCKER_HOST based configuration
local * unix:///var/run/docker.sock
docker context use default
default
Current context is now "default" 🎉 the Inspecting; docker context inspect local
[
{
"Name": "local",
"Metadata": {},
"Endpoints": {
"docker": {
"Host": "unix:///var/run/docker.sock",
"SkipTLSVerify": false
}
},
"TLSMaterial": {},
"Storage": {
"MetadataPath": "/root/.docker/contexts/meta/25bf8e1a2393f1108d37029b3df5593236c755742ec93465bbafa9b290bddcf6",
"TLSPath": "/root/.docker/contexts/tls/25bf8e1a2393f1108d37029b3df5593236c755742ec93465bbafa9b290bddcf6"
}
}
] And custom format; docker context inspect local --format '{{ .Storage.MetadataPath }}'
/root/.docker/contexts/meta/25bf8e1a2393f1108d37029b3df5593236c755742ec93465bbafa9b290bddcf6 "Pretty print" is still missing (but not a blocker for me; it's a "nice to have") docker context inspect --format=pretty local
pretty Exporting: docker context export default
"default" is a reserved context name
docker context export local -
cowardly refusing to export to a terminal, please specify a file path
docker context export local - > ./bla.tar
docker context export local
Written file "local.dockercontext"
docker context export local
open local.dockercontext: file exists Importing: docker context import
"docker context import" requires exactly 2 arguments
docker context import bla ./bla.tar
bla
Successfully imported context "bla"
cat bla.tar | docker context import piped -
piped
Successfully imported context "piped"
docker context import imported-context ./local.dockercontext
imported-context
Successfully imported context "imported-context" (we could mention docker context import imported-context ./local.dockercontext
context "imported-context" already exists
docker context import default ./local.dockercontext
"default" is a reserved context name Handling non existing / non-reachable contexts docker context create nosuchhost --docker host=http://nosuchhost.nosuchdomain
nosuchhost
Successfully created context "nosuchhost"
docker context use nosuchhost
docker ps
error during connect: Get http://nosuchhost.nosuchdomain/v1.40/containers/json: dial tcp: lookup nosuchhost.nosuchdomain on 192.168.65.1:53: no such host
docker ps --help
Usage: docker ps [OPTIONS]
... Looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Got some code nits / suggestions, but nothing blocking for now
can you cleanup the commits a bit?
The full export does not contain a kubeconfig file, no. |
394482e
to
591385a
Compare
That's something we should think about doing, so that we can export / backup |
@silvin-lubecki @vdemeester still LGTY? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still LGTM 😉
Actually the full export contains everything we need to construct a kubeconfig file, but the layout is different (partly because we thought it would be nice to make it mandatory to put TLS material in a separate dir) |
Depends on #1500
This brings a context subcommands to manage contexts stored in the context store, as well as modifying docker info / docker version to show current context