Skip to content
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

Support using sandbox name for sandbox operating #174

Merged

Conversation

miaoyq
Copy link
Contributor

@miaoyq miaoyq commented Nov 2, 2017

  • Show namespace in table format output of crictl sandboxes
  • Support using sandbox name for inspects, stops and rms.

Example:
Show namespace:

# crictl sandboxes
SANDBOX ID                                                         NAME                STATE               NAMESPACE
432374d9dfa61d8768668784e9d5392159c8b87810628fefe2871b9233ff0187   busybox             SANDBOX_READY       myq
41555a4bd665dd92078701eb8cbd84723c9257825b1219240de3292343316e3f   busybox             SANDBOX_READY       default
cce6f4f601d4494f9ea2aba48c36b70fea107d9e047e8916052348a128ad51e2   kube-proxy-gwb2k    SANDBOX_READY       kube-system

Get the status of a sandbox by sandbox name:

# crictl inspects busybox -n myq
ID: 432374d9dfa61d8768668784e9d5392159c8b87810628fefe2871b9233ff0187
Name: busybox
UID: 8fed396c-bf74-11e7-bd9d-5254007ee9b4
Namespace: myq
Attempt: 0
Status: SANDBOX_READY
Created: 2017-11-02 10:19:28.555867171 +0800 CST
IP Address: 10.88.0.88
Labels:
	io.kubernetes.pod.name -> busybox
	io.kubernetes.pod.namespace -> myq
	io.kubernetes.pod.uid -> 8fed396c-bf74-11e7-bd9d-5254007ee9b4
Annotations:
	kubernetes.io/config.seen -> 2017-11-02T10:19:28.137670526+08:00
	kubernetes.io/config.source -> api

Get the status of a sandbox by sandbox ID:

# crictl inspects 432374d9dfa61d8768668784e9d5392159c8b87810628fefe2871b9233ff0187
ID: 432374d9dfa61d8768668784e9d5392159c8b87810628fefe2871b9233ff0187
Name: busybox
UID: 8fed396c-bf74-11e7-bd9d-5254007ee9b4
Namespace: myq
Attempt: 0
Status: SANDBOX_READY
Created: 2017-11-02 10:19:28.555867171 +0800 CST
IP Address: 10.88.0.88
Labels:
	io.kubernetes.pod.name -> busybox
	io.kubernetes.pod.namespace -> myq
	io.kubernetes.pod.uid -> 8fed396c-bf74-11e7-bd9d-5254007ee9b4
Annotations:
	kubernetes.io/config.seen -> 2017-11-02T10:19:28.137670526+08:00
	kubernetes.io/config.source -> api

/cc @feiskyer @Random-Liu

Signed-off-by: Yanqiang Miao miao.yanqiang@zte.com.cn

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 2, 2017
@miaoyq miaoyq force-pushed the support-sandbox-name-for-sandbox-op branch from b7a07c9 to f1cf2a7 Compare November 2, 2017 05:47
@Random-Liu Random-Liu self-assigned this Nov 2, 2017
@Random-Liu
Copy link
Contributor

Random-Liu commented Nov 2, 2017

@miaoyq

  1. I do think crictl sandboxes should show namespace, but I prefer ID Name Namespace State.
  2. Can we shorten the sandbox id just like what we do for image id?
  3. Name + Namespace doesn't uniquely identify one sandbox. We may have several sandboxes with the same name and namespace, but have different attempt number. Instead of support inspect sandbox by name + namespace, I prefer provide a filter in crictl sandboxes to filter sandboxes for a specific namespace or with a specific name.

@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 2, 2017

  1. I do think crictl sandboxes should show namespace, but I prefer ID Name Namespace State.
  2. Can we shorten the sandbox id just like what we do for image id?

@Random-Liu Will do.

  1. Name + Namespace doesn't uniquely identify one sandbox. We may have several sandboxes with the same name and namespace, but have different attempt number. Instead of support inspect sandbox by name + namespace, I prefer provide a filter in crictl sandboxes to filter sandboxes for a specific namespace or with a specific name.

Sandbox name is the value of pod.Metadate.Name, but not the name of pause container, so I think it is impossible to have several sandboxes with the same name and namespace.
I think it should be like that, but not ensure. :)

@Random-Liu
Copy link
Contributor

Random-Liu commented Nov 2, 2017

Sandbox name is the value of pod.Metadate.Name, but not the name of pause container, so I think it is impossible to have several sandboxes with the same name and namespace.

We also have podSandbox.Metadata.Attempt.

There may be multiple sandboxes with the same name, that's how CRI works. :)

@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 2, 2017

@Random-Liu Thanks for explaining, I think I should take more time to study this part in k8s.

@miaoyq miaoyq force-pushed the support-sandbox-name-for-sandbox-op branch 3 times, most recently from ee19f89 to c4eb72f Compare November 3, 2017 14:04
@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 3, 2017

  • crictl sandboxes show namespace in table format
  • Add filter for crictl sandboxes to filter sandboxes with name and namespace
  • Shorten sandbox id and container id in table format

@Random-Liu

# crictl sandboxes
SANDBOX ID          NAME                NAMESPACE           STATE
41555a4bd665d       busybox             default             SANDBOX_READY
cce6f4f601d44       kube-proxy-gwb2k    kube-system         SANDBOX_READY
432374d9dfa61       busybox             myq                 SANDBOX_READY
# crictl sandboxes --name busybox
SANDBOX ID          NAME                NAMESPACE           STATE
41555a4bd665d       busybox             default             SANDBOX_READY
432374d9dfa61       busybox             myq                 SANDBOX_READY
# crictl sandboxes --namespace myq
SANDBOX ID          NAME                NAMESPACE           STATE
432374d9dfa61       busybox             myq                 SANDBOX_READY
# crictl sandboxes --name busybox --namespace myq
SANDBOX ID          NAME                NAMESPACE           STATE
432374d9dfa61       busybox             myq                 SANDBOX_READY
# crictl ps
CONTAINER ID        CREATED             STATE               NAME
d67cc2866acff       2 weeks ago         CONTAINER_RUNNING   kube-proxy
ddad3288bced9       2 hours ago         CONTAINER_EXITED    busybox
44adf44fe7013       About an hour ago   CONTAINER_EXITED    busybox
b9a62c3560e06       About an hour ago   CONTAINER_RUNNING   busybox
6a89881280e61       7 seconds ago       CONTAINER_RUNNING   busybox

@Random-Liu
Copy link
Contributor

We may want to add a flag to show Attempt number, but I'm fine with doing that in another PR.

@@ -357,15 +373,16 @@ func ListPodSandboxes(client pb.RuntimeServiceClient, opts listOptions) error {

w := tabwriter.NewWriter(os.Stdout, 20, 1, 3, ' ', 0)
if !opts.verbose && !opts.quiet {
fmt.Fprintln(w, "SANDBOX ID\tNAME\tSTATE")
fmt.Fprintln(w, "SANDBOX ID\tNAME\tNAMESPACE\tSTATE")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I found that for containers we put name at the end. Let's keep this consitent.

Let's do the same sandbox id\tstate\tname\tnamespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Random-Liu
Copy link
Contributor

LGTM with one last nit.

@miaoyq miaoyq force-pushed the support-sandbox-name-for-sandbox-op branch from c4eb72f to 59108b5 Compare November 4, 2017 01:45
Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>
@Random-Liu
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2017
@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 4, 2017

/retest

@miaoyq
Copy link
Contributor Author

miaoyq commented Nov 4, 2017

@Random-Liu The travis-ci failed, but unrelated to the change of this pr.
runtime should support log verification failed because the container didn't start normally. Maybe it related to test environment.

Could you help to trigger the test again?

Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI green now, LGTM

@feiskyer feiskyer merged commit 6254a2a into kubernetes-sigs:master Nov 5, 2017
@miaoyq miaoyq deleted the support-sandbox-name-for-sandbox-op branch November 6, 2017 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants