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

kic: explicitly provide the type in inspect commands #8229

Merged
merged 1 commit into from
May 21, 2020

Conversation

afbjorklund
Copy link
Collaborator

Should avoid errors like: map has no entry for key "State"

When inspecting a volume, rather than a container or image

Should help with #8192

Reviewer note: also fixed a typo in an internal function name

@afbjorklund afbjorklund requested a review from medyagh May 21, 2020 08:27
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @afbjorklund,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: ab1e4720-9b3d-11ea-acf2-2540bfc6cd37

@afbjorklund
Copy link
Collaborator Author

Here is the issue we are trying to address:

 docker run -d --name foo -d busybox true
c4e9d99e7c959014b5e0b6c543bc4ca5ff54475b6e79dbc2c717a2fff304971d
 docker inspect foo --format={{.State.Status}}
exited
 docker container inspect foo --format={{.State.Status}}
exited
 docker volume create foo
 docker rm foo
 docker inspect foo --format={{.State.Status}}

Template parsing error: template: :1:8: executing "" at <.State.Status>: map has no entry for key "State"
 docker container inspect foo --format={{.State.Status}}

Error: No such container: foo

That is, when using "inspect" it will match the volume name too. So we add "container" explicitly.

This seems to be a common problem at minikube start, when it tries to re-create the container...

Should avoid errors like: map has no entry for key "State"

When inspecting a volume, rather than a container or image
@codecov-commenter
Copy link

Codecov Report

Merging #8229 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8229   +/-   ##
=======================================
  Coverage   34.50%   34.50%           
=======================================
  Files         147      147           
  Lines        9412     9412           
=======================================
  Hits         3248     3248           
  Misses       5765     5765           
  Partials      399      399           
Impacted Files Coverage Δ
pkg/minikube/cruntime/crio.go 52.12% <100.00%> (ø)
pkg/minikube/cruntime/docker.go 27.27% <100.00%> (ø)

@medyagh
Copy link
Member

medyagh commented May 21, 2020

this is a great improvment ! thank you @afbjorklund do you know if this "type" inspect is supported in older docker versions? or is there a min version that we would require for this change?

@afbjorklund
Copy link
Collaborator Author

this is a great improvment ! thank you @afbjorklund do you know if this "type" inspect is supported in older docker versions? or is there a min version that we would require for this change?

It's available in 1.13 / 17.03, I don't think we need to support any older than that...

@medyagh
Copy link
Member

medyagh commented May 21, 2020

this is a great improvment ! thank you @afbjorklund do you know if this "type" inspect is supported in older docker versions? or is there a min version that we would require for this change?

It's available in 1.13 / 17.03, I don't think we need to support any older than that...

sounds good

@medyagh
Copy link
Member

medyagh commented May 21, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 21, 2020
@kubernetes kubernetes deleted a comment from minikube-pr-bot May 21, 2020
@minikube-pr-bot
Copy link

kvm2 Driver
Times for minikube: [66.296490546 66.35982231800001 65.575983862]
Average time for minikube: 66.077432242

Times for Minikube (PR 8229): [63.798267839000005 64.36760161299999 64.059085035]
Average time for Minikube (PR 8229): 64.074984829

Averages Time Per Log

+--------------------------------+-----------+--------------------+
|              LOG               | MINIKUBE  | MINIKUBE (PR 8229) |
+--------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian   |  0.066705 |           0.058000 |
|                           9.11 |           |                    |
| * Using the kvm2 driver based  |  0.018586 |           0.018886 |
| on existing profile            |           |                    |
| * Starting control plane node  |  0.003418 |           0.003488 |
| minikube in cluster minikube   |           |                    |
| * Creating kvm2 VM (CPUs=2,    | 41.435987 |          40.283736 |
| Memory=3700MB, Disk=20000MB)   |           |                    |
| ...                            |           |                    |
| * Preparing Kubernetes v1.18.2 | 22.484256 |          21.277047 |
| on Docker 19.03.8 ...          |           |                    |
| * Verifying Kubernetes         |  1.366114 |           1.633436 |
| components...                  |           |                    |
| * Enabled addons:              |  0.623074 |           0.723115 |
| default-storageclass,          |           |                    |
| storage-provisioner            |           |                    |
| * Done! kubectl is now         |  0.075849 |           0.072736 |
| configured to use "minikube"   |           |                    |
|                                |  0.003444 |           0.004540 |
+--------------------------------+-----------+--------------------+

docker Driver
Times for minikube: [25.569687386000002 26.964672858999997 25.848291492]
Average time for minikube: 26.127550579

Times for Minikube (PR 8229): [27.840063079 28.124562301000005 28.632874853000004]
Average time for Minikube (PR 8229): 28.199166744333336

Averages Time Per Log

+----------------------------------------+-----------+--------------------+
|                  LOG                   | MINIKUBE  | MINIKUBE (PR 8229) |
+----------------------------------------+-----------+--------------------+
| * minikube v1.10.1 on Debian           |  0.075825 |           0.074437 |
|                                   9.11 |           |                    |
| * Using the docker driver              |  0.002379 |           0.002403 |
| based on existing profile              |           |                    |
| * Starting control plane node          |  0.057649 |           0.058441 |
| minikube in cluster minikube           |           |                    |
| * Creating docker container            |  7.648571 |           7.963128 |
| (CPUs=2, Memory=3700MB) ...            |           |                    |
| * Preparing Kubernetes v1.18.2         |  0.117558 |           0.118085 |
| on Docker 19.03.2 ...                  |           |                    |
|   -                                    | 16.936561 |          18.720010 |
| kubeadm.pod-network-cidr=10.244.0.0/16 |           |                    |
| * Verifying Kubernetes                 |  1.120217 |           1.092449 |
| components...                          |           |                    |
| * Enabled addons:                      |  0.103223 |           0.100504 |
| default-storageclass,                  |           |                    |
| storage-provisioner                    |           |                    |
| * Done! kubectl is now                 |  0.061248 |           0.065911 |
| configured to use "minikube"           |           |                    |
|                                        |  0.004319 |           0.003800 |
+----------------------------------------+-----------+--------------------+

@medyagh medyagh changed the title Never use docker inspect, always provide the type kic: explicitly provide the type in inspect commands May 21, 2020
@medyagh medyagh merged commit ad437c2 into kubernetes:master May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

6 participants