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

kubelet: Refactor RunInContainer/ExecInContainer/PortForward. #6491

Merged

Conversation

yifan-gu
Copy link
Contributor

@yifan-gu yifan-gu commented Apr 6, 2015

Replace GetKubeletDockerContainers() with GetPods().

This removes the docker specific code in further. (Now we only have one GetKubeletDockerContainers!)

I remember @dchen1107 said now each pod has its UID, so I think we can remove the podFullName part in the kubelet's interface.(correct me if I am wrong, thank you!).

@vmarmol @yujuhong

@yujuhong
Copy link
Contributor

yujuhong commented Apr 6, 2015

Thanks for the PR! We've made quite some progress in this direction! :)

I may be wrong, but IMO, kubelet should use UID internally, but should also support querying using pod (namespace, name) tuple in public-facing interface. As far as I know, the interface is defined here https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/kubelet/server.go#L99

@dchen1107
Copy link
Member

Didn't review PR in detail yet, @yujuhong is right. UID is terrible on user friendly.

@yifan-gu yifan-gu force-pushed the depreciate_getkubeletdockercontainers branch 2 times, most recently from 11ce5d2 to 2e11690 Compare April 7, 2015 00:07
@yifan-gu
Copy link
Contributor Author

yifan-gu commented Apr 7, 2015

@yujuhong Thank you for clarification! I was wondering why e2e is failing... Maybe this is related.
Just updated the PR to keep the podFullName. Running e2e again.

@yifan-gu yifan-gu force-pushed the depreciate_getkubeletdockercontainers branch from 2e11690 to 8fe408a Compare April 7, 2015 00:12
Replace GetKubeletDockerContainers() with findContainer().
@yifan-gu yifan-gu force-pushed the depreciate_getkubeletdockercontainers branch from 8fe408a to ba1ad9f Compare April 7, 2015 00:15
@vmarmol
Copy link
Contributor

vmarmol commented Apr 7, 2015

LGTM, will wait on your results with e2e.

One day, my dream is that we will only use UID internally (and translate full name to UID once we take it in). But it's not currently a priority.

@rjnagal rjnagal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2015
@yifan-gu
Copy link
Contributor Author

yifan-gu commented Apr 7, 2015

Just cannot finish e2e because I have a terrible network at home... Will run tomorrow morning when I arrive the company.

@yifan-gu
Copy link
Contributor Author

yifan-gu commented Apr 7, 2015

@vmarmol
Got 1 failure, think it's not related: #6517

Summarizing 1 Failure:

[Fail] Cluster level logging using Elasticsearch [It] should check that logs from pods on all nodes are ingested into Elasticsearch
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/es_cluster_logging.go:326

Ran 32 of 37 Specs in 1318.850 seconds
FAIL! -- 31 Passed | 1 Failed | 0 Pending | 5 Skipped watchEvents output: |

@satnam6502
Copy link
Contributor

Pretty sure this test failure is not related since it is pointing out an unrelated issue in our system.

@vmarmol
Copy link
Contributor

vmarmol commented Apr 7, 2015

LGTM @yifan-gu! Merging.

vmarmol added a commit that referenced this pull request Apr 7, 2015
…tainers

kubelet: Refactor RunInContainer/ExecInContainer/PortForward.
@vmarmol vmarmol merged commit ef3cdb2 into kubernetes:master Apr 7, 2015
@yifan-gu yifan-gu deleted the depreciate_getkubeletdockercontainers branch May 7, 2015 17:29
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants