-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Clean up dockertools/manager.go and add more unit tests #7533
Conversation
I think we had some discussion on this early, I remember @dchen1107 said the GetContainers is for getting not only kubelet containers, but also all containers so user can introspect the state of the node. So we would probably leave it in the interface IMO. |
@yifan-gu, this PR assumes we are getting only the kubelet containers though. If what we want is to list all containers, I'd have to fix the PR. :) |
I think we'll remove it long-term :) From the two uses, the first we can replace with I'm fine going this route for now though. It'll simplify what we have to move when the time comes. |
for _, c := range actualContainers { | ||
actual.Insert(c.Name) | ||
} | ||
if !reflect.DeepEqual(expected.List(), actual.List()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't these be non-deterministically sorted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util.StringSet
actually sorts this https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/util/set.go#L122
@vmarmol, I agree that the use cases can probably be eliminated. I think What about the reason that @yifan-gu said? Should we get all containers for users to introspect? If that's the main purpose of the function, then this PR needs to change quite a bit because most of the docker code looks at only k8s containers. |
@yujuhong I think we can remove this from the runtime interface for now, leave a TODO and add them later... |
LGTM. I don't think we need it for that type of introspection, and if we do I think a better mechanism is the "unknown" pod. We can remove it from the runtime (but can we keep the rest of the refactoring you did? I think that's valuable) |
Keep the refacoring + 1 |
This change refactors the GetPods function and add some basic unit tests. We should start migrating docker specific tests from kubelet_test to manager_test.go.
959b3f5
to
3586c8c
Compare
3586c8c
to
919d782
Compare
Updated the commit to remove I will remove |
LGTM, thanks @yujuhong |
LGTM |
Clean up dockertools/manager.go and add more unit tests
This function is part of the container runtime interface, so docker should
support it.
This change also adds some basic unit tests in manager_test.go. We should
start migrating docker specific tests from kubelet_test to manager_test.go.