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

Kubelet: implement GetPods for new runtime API #30121

Merged
merged 1 commit into from
Aug 22, 2016
Merged

Kubelet: implement GetPods for new runtime API #30121

merged 1 commit into from
Aug 22, 2016

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented Aug 5, 2016 •
edited
Loading

Implement GetPods for kuberuntime. Part of #28789 .

CC @yujuhong @Random-Liu

This change is Reviewable

@feiskyer feiskyer changed the title Kubelet: implement GetPods for kuberuntime Kubelet: implement GetPods for new runtime API Aug 5, 2016
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Aug 5, 2016
@yujuhong yujuhong assigned Random-Liu and yujuhong and unassigned dchen1107 Aug 5, 2016
@yujuhong yujuhong added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 5, 2016
yujuhong
yujuhong reviewed Aug 5, 2016
View reviewed changes
pkg/kubelet/kuberuntime/helpers.go
@@ -113,7 +125,43 @@ func parseContainerName(name string) (podName, podNamespace, podUID, containerNa
return parts[2], parts[3], parts[4], containerName, hash, nil
}

// toRuntimeProtocol converts api.Protocol to runtimeApi.Protocol
// isSandBoxManagedByKubelet returns true is the sandbox is managed by kubelet.
func isSandBoxManagedByKubelet(name string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/SandBox/Sandbox

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@feiskyer
Copy link
Member Author

feiskyer commented Aug 9, 2016

@yujuhong @Random-Liu Rebased since #30049 is in. PTAL.

@feiskyer feiskyer mentioned this pull request Aug 9, 2016
Merged
@yujuhong
Copy link
Contributor

yujuhong commented Aug 9, 2016

/cc @tmrts @yifan-gu @euank

tmrts
tmrts reviewed Aug 10, 2016
View reviewed changes
pkg/kubelet/kuberuntime/helpers.go
milliCPUToCPU = 1000

// 100000 is equivalent to 100ms
quotaPeriod = 100000
Copy link
Contributor

Choose a reason for hiding this comment

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

quotaPeriod = minQuotaPeriod * 100 is more readable

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

@yujuhong
Copy link
Contributor

pkg/kubelet/kuberuntime/helpers.go, line 126 [r2] (raw file):

Previously, feiskyer (Pengfei Ni) wrote…

@tmrts Not sure about what a lot of errors unrelated to the responsibility of this function means, but sandbox is not managed by kubelet only if parseSandboxName is errored. Do you mean the logs inside parseSandboxName() are too much?

We have the naming discussion in a separate issue, which IMO, should make naming parsing not necessary.

Comments from Reviewable

@yujuhong
Copy link
Contributor

Reviewed 1 of 5 files at r1, 1 of 4 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

pkg/kubelet/kuberuntime/kuberuntime_container.go, line 208 [r2] (raw file):

Previously, euank (Euan Kemp) wrote…

I missed that there was ever a question as to whether a runtime's CRI impl was exclusive to kubelet. Anyone have links to the existing discussion on that?

Kubelet needs to see non-kubelet managed containers for image management. I think retaining the ability to distinguish a kubelet-managed container is still good for now.

pkg/kubelet/kuberuntime/kuberuntime_manager.go, line 226 [r1] (raw file):

Previously, yifan-gu (Yifan Gu) wrote…

Then it's just the same as getpods today 😛 Which is OK, (so list sandbox contains containers)

I am not super concerned about the performance as long as it's not too back, since for the long term we will use event system.

Agreed that performance optimization can be done later. Switching to event is going to take some time though. We can't be refactoring everything at once.

pkg/kubelet/kuberuntime/kuberuntime_manager_test.go, line 36 [r3] (raw file):

)

func createTestFakeRuntimeManager() (*apitest.FakeRuntimeService, *apitest.FakeImageService, *kubeGenericRuntimeManager, error) {

nit: s/createTestFakeRuntimeManager/createTestRuntimeManager
The runtime manager is not exactly a fake since we are testing the logic in it.

pkg/kubelet/kuberuntime/kuberuntime_manager_test.go, line 189 [r3] (raw file):

  }

  // Set fake sandbox and faked containers to fakeRuntime.

nit: s/faked/fake

Comments from Reviewable

@yujuhong
Copy link
Contributor

Looks good overall with some nits.

Comments from Reviewable

@feiskyer
Copy link
Member Author

@yujuhong Addressed comments and squashed. PTAL.

@yujuhong
Copy link
Contributor

pkg/kubelet/kuberuntime/kuberuntime_container.go, line 195 [r4] (raw file):

// getKubeletContainers lists all (or just the running) containers managed by kubelet.
func (m *kubeGenericRuntimeManager) getKubeletContainers(allContainers bool) ([]*runtimeApi.Container, error) {

Suggestion: create two functionsgetContainers() and getKubeletContainers().
getKubeletContainers() can just call getContainers() and filter out non-kubelet-managed containers before returning.

I think that's cleaner than use a boolean to toggle the behavior :)

Comments from Reviewable

@feiskyer
Copy link
Member Author

@yujuhong Thanks for good idea. PTAL.

@yujuhong
Copy link
Contributor

pkg/kubelet/kuberuntime/kuberuntime_container.go, line 195 [r4] (raw file):

Previously, yujuhong (Yu-Ju Hong) wrote…

Suggestion: create two functionsgetContainers() and getKubeletContainers().
getKubeletContainers() can just call getContainers() and filter out non-kubelet-managed containers before returning.

I think that's cleaner than use a boolean to toggle the behavior :)

I misread this. `allContainers` include non-running containers.

I think allContainersshould not be overloaded with two meanings: 1) whether the container is running, and 2) whether the container is managed by kubelet.

Let's factor out the common code and create separate functions.
E.g., getContainersHelper(filter runtimeApi.ContainerFilter)
getKubeletRunningContainers can call getContainersHelper with the right filter, and then check the container labels before returning.

Comments from Reviewable

@yujuhong
Copy link
Contributor

pkg/kubelet/kuberuntime/kuberuntime_container.go, line 195 [r4] (raw file):

Previously, yujuhong (Yu-Ju Hong) wrote…

I misread this. allContainers include non-running containers.

I think allContainersshould not be overloaded with two meanings: 1) whether the container is running, and 2) whether the container is managed by kubelet.

Let's factor out the common code and create separate functions.
E.g., getContainersHelper(filter runtimeApi.ContainerFilter)
getKubeletRunningContainers can call getContainersHelper with the right filter, and then check the container labels before returning.

I still prefer a more explicit `getKubeletRunningContainers()`function, but we can work on that later.

Comments from Reviewable

@yujuhong
Copy link
Contributor

Looks good to me. @yifan-gu or @tmrts , do you still have more comments?

Comments from Reviewable

@yujuhong
Copy link
Contributor

+lgtm

Comments from Reviewable

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2016
@feiskyer
Copy link
Member Author

Rebased.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 22, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit e3e10dd.

@feiskyer feiskyer mentioned this pull request Aug 22, 2016
Merged
@yujuhong yujuhong added lgtm "Looks good to me", indicates that a PR is ready to be merged. retest-not-required labels Aug 22, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 969ce77 into kubernetes:master Aug 22, 2016
@euank
Copy link
Contributor

euank commented Aug 22, 2016 •
edited
Loading

belated LGTM :)

@feiskyer feiskyer deleted the kuberuntime-getpods branch August 22, 2016 23:09
@yujuhong yujuhong mentioned this pull request Aug 25, 2016
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@euank euank

@yifan-gu yifan-gu

@tmrts tmrts

@Random-Liu Random-Liu

@yujuhong yujuhong

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@feiskyer @yujuhong @yifan-gu @k8s-bot @k8s-github-robot @euank @tmrts @Random-Liu @googlebot @dchen1107