-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Make Daemonset use GeneralPredicates #28803
Conversation
@@ -37,16 +36,16 @@ import ( | |||
"k8s.io/kubernetes/pkg/controller" | |||
"k8s.io/kubernetes/pkg/controller/framework" | |||
"k8s.io/kubernetes/pkg/controller/framework/informers" | |||
"k8s.io/kubernetes/pkg/kubelet/util/format" |
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.
it's weird to be depending on a pkg/kubelet/util package in the controller manager. Perhaps we should move required functionality to pkg/util
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.
Ah. Yes, it's weird. Actually I don't need it. I will just log Pod name.
@mikedanese @davidopp so guys, what do you think about this change? |
LGTM |
@mikedanese / @lukaszo I'm not going to have a chance to review this for a while -- it's OK to LGTM but can you document here exactly which predicates DaemonSet will now be subjected to that it was not previously, and which (if any -- I assume none) predicates it used to be subjected to that it will now not be? |
GCE e2e build/test passed for commit 528bf7a. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit 528bf7a. |
Automatic merge from submit-queue |
@mikedanese @davidopp yes, node affinity and gpu resource. I will also update documentation for DaemonSets. |
fixes: #21454 #22205