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

Remove RunInContainer interface in Kubelet Runtime interface #24921

Merged
merged 1 commit into from
May 15, 2016

Conversation

feiskyer
Copy link
Member

According to #24689, we should merge RunInContainer and ExecInContainer in the container runtime interface.

@yujuhong @kubernetes/sig-node

@feiskyer feiskyer changed the title Remove RunInContainer interface in Kuberlete Runtime interface Remove RunInContainer interface in Kubelet Runtime interface Apr 28, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 28, 2016
@feiskyer
Copy link
Member Author

@k8s-bot test this issue #24689

1 similar comment
@feiskyer
Copy link
Member Author

@k8s-bot test this issue #24689

@yujuhong yujuhong assigned yujuhong and unassigned dchen1107 May 2, 2016
@yujuhong yujuhong added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 2, 2016
@yujuhong
Copy link
Contributor

yujuhong commented May 2, 2016

@feiskyer, thanks a lot for cleaning this up! LGTM.

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2016
@feiskyer
Copy link
Member Author

@yujuhong Am I missing something, k8s-bot has tested this for several times, but not merged it.

@yujuhong
Copy link
Contributor

@feiskyer nope, we just have a long queue of PRs to merge, and the tree was closed half of the time due to blocking build/test failures.
You can check the queue at http://submit-queue.k8s.io/#/queue There are tens of PRs before this one...

@yujuhong yujuhong assigned yujuhong and unassigned dchen1107 May 11, 2016
@k8s-bot
Copy link

k8s-bot commented May 15, 2016

GCE e2e build/test passed for commit 831203c.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented May 15, 2016

GCE e2e build/test passed for commit 831203c.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 59b7b1c into kubernetes:master May 15, 2016
@feiskyer feiskyer deleted the merge-exec branch May 15, 2016 10:04
k8s-github-robot pushed a commit that referenced this pull request Aug 20, 2016
Automatic merge from submit-queue

Always return command output for exec probes and kubelet RunInContainer

Always return command output for exec probes and kubelet RunInContainer, even if the command invocation returns nonzero.

When #24921 replaced RunInContainer with ExecInContainer, it introduced a change where an exec probe that failed no longer included the stdout/stderr from the probe in the event. For example, when running at log level 4, you see:

```
I0816 15:01:36.259826 29713 exec.go:38] Exec probe response: "Failed to access the status endpoint : HTTP Error 404: Not Found.\nHawkular metrics has only been running for 7\n seconds not aborting yet.\n"
```

But the event looks like this:

```
54s 22s 5 hawkular-metrics-hjme4 Pod spec.containers{hawkular-metrics} Warning Unhealthy {kubelet corbeau} Readiness probe failed:
```

Note the absence of the exec probe response after "Readiness probe failed". This PR restores the previous behavior.

cc @kubernetes/rh-cluster-infra @mwringe 

xref openshift/origin#10424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants