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.

Sign up for GitHub

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

Jump to bottom

A pod never terminated if a container image registry was unavailable #23746

Merged
merged 1 commit into from
Apr 1, 2016
Merged

A pod never terminated if a container image registry was unavailable #23746

merged 1 commit into from
Apr 1, 2016
+4 −3

Conversation

derekwaynecarr
Copy link
Member

Fixes #23742
Fixes #22045

Pods will now show proper status when this happens as well:

$ cluster/kubectl.sh get pods --all-namespaces 
NAMESPACE   NAME                   READY     STATUS                RESTARTS   AGE
test        foo-4072956304-1g8qs   0/1       RegistryUnavailable   0          7s
test        foo-4072956304-i045g   0/1       RegistryUnavailable   0          7s
test        foo-4072956304-qem2n   0/1       RegistryUnavailable   0          7s

Where as previously they would never report a reason.

I also removed the "temporary" part of the message because we have no idea if its temporary or permanent.

/cc @kubernetes/sig-node @kubernetes/rh-cluster-infra

@derekwaynecarr derekwaynecarr added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 1, 2016
@eparis eparis added this to the v1.2 milestone Apr 1, 2016
This was referenced Apr 1, 2016
Closed
Closed
@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 1, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/XS

@ncdc
Copy link
Member

ncdc commented Apr 1, 2016

LGTM

@eparis eparis added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 1, 2016
@derekwaynecarr
Copy link
Member Author

A good summary here:
#23742 (comment)

@yujuhong yujuhong assigned yujuhong and unassigned dchen1107 Apr 1, 2016
@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2016
@yujuhong
Copy link
Contributor

yujuhong commented Apr 1, 2016

We should also fix the default container state to eliminate the need to enumerate all the special errors. This will help prevent breakage in the future. #23742 (comment)
@Random-Liu, are you working on a PR? If not, I'll do it.

@smarterclayton
Copy link
Contributor

Test case?

@yujuhong
Copy link
Contributor

yujuhong commented Apr 1, 2016

@Random-Liu's pending PR (#21224) will add more unit tests for this part of the code. I am okay with merging this PR as it is to patch 1.2

@k8s-bot
Copy link

k8s-bot commented Apr 1, 2016

GCE e2e build/test passed for commit eeeccd0.

@vishh
Copy link
Contributor

vishh commented Apr 1, 2016

LGTM

@vishh
Copy link
Contributor

vishh commented Apr 1, 2016

Ideally, we should add a test in test/e2e_node/kubelet.go....

@Random-Liu Random-Liu mentioned this pull request Apr 1, 2016
Closed
3 tasks
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@derekwaynecarr
Copy link
Member Author

Will look at an e2e node test next week.

@k8s-bot
Copy link

k8s-bot commented Apr 1, 2016

GCE e2e build/test passed for commit eeeccd0.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@dchen1107
Copy link
Member

I am ok to merge this as is, but could you add a test case in a separate pr?

@k8s-bot
Copy link

k8s-bot commented Apr 1, 2016

GCE e2e build/test passed for commit eeeccd0.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 224684c into kubernetes:master Apr 1, 2016
@wojtek-t
Copy link
Member

wojtek-t commented Apr 1, 2016

Nice finding @derekwaynecarr !

@jeremyeder
Copy link

Thanks, @derekwaynecarr!

@derekwaynecarr
Copy link
Member Author

@dchen1107 @vishh - I was trying to think how to write an e2e for this, but to reproduce I need to simulate docker returning a 50x error on a particular image pull request which is proving non-trivial for me to figure out how to plumb through. any tips would be appreciated.

From a unit test perspective, I agree that #21224 provides coverage and removes the open list around kubecontainer error states.

@yujuhong
Copy link
Contributor

yujuhong commented Apr 4, 2016

I think unit tests are enough in this case. As @derekwaynecarr mentioned, we don't have a good way to inject specific errors in docker client at the node/cluster e2e level, and I'm also not sure how much it'd help for testing one specific error case.

@derekwaynecarr derekwaynecarr mentioned this pull request Apr 4, 2016
Merged
@dchen1107
Copy link
Member

I didn't expect an e2e test for this. unitttests provided by #21224 sounds good to me. Thanks!

@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 4, 2016
@vishh
Copy link
Contributor

vishh commented Apr 4, 2016 via email

@bgrant0607
Copy link
Member

release-note-none is not allowed for cherrypicks

@bgrant0607
Copy link
Member

That is, if it matters enough to warrant a cherrypick, it needs a release note

@yujuhong
Copy link
Contributor

yujuhong commented Apr 5, 2016

@bgrant0607, will the PR description be included in the release note as well?

@bgrant0607 bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 6, 2016
@bgrant0607
Copy link
Member

@yujuhong Currently I think only the title is included in the release note, though I would like to be able to extract a section from the description.
cc @david-mcmahon

@david-mcmahon
Copy link
Contributor

It is currently only the title. #23743 will address collecting more detail.

zmerlynn added a commit that referenced this pull request Apr 6, 2016
@zmerlynn
Merge pull request #23824 from derekwaynecarr/automated-cherry-pick-o…
c9e76db 
…f-#23746-upstream-release-1.2

Automated cherry pick of #23746 upstream release 1.2
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@zmerlynn
Merge pull request kubernetes#23824 from derekwaynecarr/automated-che…
29903ca 
…rry-pick-of-#23746-upstream-release-1.2

Automated cherry pick of kubernetes#23746 upstream release 1.2
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@zmerlynn
Merge pull request kubernetes#23824 from derekwaynecarr/automated-che…
27d382d 
…rry-pick-of-#23746-upstream-release-1.2

Automated cherry pick of kubernetes#23746 upstream release 1.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yujuhong yujuhong

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.