-
Notifications
You must be signed in to change notification settings - Fork 771
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
Optimize Advanced DaemonSet internal new pod for imitating scheduling #1011
Optimize Advanced DaemonSet internal new pod for imitating scheduling #1011
Conversation
Signed-off-by: FillZpp <FillZpp.pub@gmail.com>
newPod := &corev1.Pod{Spec: ds.Spec.Template.Spec, ObjectMeta: ds.Spec.Template.ObjectMeta} | ||
newPod.Namespace = ds.Namespace | ||
newPod.Spec.NodeName = nodeName | ||
// no need to set nodeName |
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.
since we're confident that pod for ds not exist here, plz consider using newPodForDSCache. LoadOrStore so that we don't have to maintain newPodForDSLock
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.
No, only LoadOrStore might have multiple NewPod to be create and store, for this is not only called in reconcile, but also event handler.
@@ -223,6 +223,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { | |||
ds := e.Object.(*appsv1alpha1.DaemonSet) | |||
klog.V(4).Infof("Deleting DaemonSet %s/%s", ds.Namespace, ds.Name) | |||
dsc.expectations.DeleteExpectations(keyFunc(ds)) | |||
newPodForDSCache.Delete(ds.UID) |
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.
plz delete entry of ds.UID if daemonset is not found in the beginning of Reconcile()
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.
Emm.. I can't do that, for there is no UID since daemonset is not found. Delete handler should be enough.
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.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FillZpp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…openkruise#1011) Signed-off-by: FillZpp <FillZpp.pub@gmail.com> Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>
Signed-off-by: FillZpp FillZpp.pub@gmail.com
Ⅰ. Describe what this PR does
Optimize Advanced DaemonSet internal new pod for imitating scheduling