add enhanced livenessprobe controller #1544
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
BH4AWS
force-pushed
the
add_enhanced_livenessprobe_controller_v2
branch
4 times, most recently
from
e06803a
to
aa96b38
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1544 +/- ##
==========================================
- Coverage 47.91% 47.89% -0.02%
==========================================
Files 162 165 +3
Lines 23483 23771 +288
==========================================
+ Hits 11252 11386 +134
- Misses 11011 11139 +128
- Partials 1220 1246 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
BH4AWS
force-pushed
the
add_enhanced_livenessprobe_controller_v2
branch
from
aa96b38
to
828c3fa
Compare
Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com>
BH4AWS
force-pushed
the
add_enhanced_livenessprobe_controller_v2
branch
from
828c3fa
to
689efee
Compare
|
|
||
| const ( | ||
| concurrentReconciles = 10 | ||
| controllerName = "livenessprobemapnodeprobe-controller" |
There was a problem hiding this comment.
rename controller to more simplified form LivenessNodeProbeController , also plz rename the package and file name
| podNewNppCloneSpec.PodProbes = newPodProbeTmp | ||
|
|
||
| // delete node pod probe | ||
| if len(podNewNppCloneSpec.PodProbes) == 0 { |
There was a problem hiding this comment.
if node still exists, it seems unnecessary to delete the nodepodprobe, just leave it empty seems enough
There was a problem hiding this comment.
done, remove
// delete node pod probe if len(podNewNppCloneSpec.PodProbes) == 0 { if err := r.Delete(context.TODO(), nppClone); err != nil { klog.Errorf("Failed to delete node pod probe for pod: %s/%s, err: %v", pod.Namespace, pod.Name, err) return err } return nil }
| podContainerProbe.UID == fmt.Sprintf("%v", pod.UID) { | ||
| isHit = true | ||
| // diff the current pod container probes vs the npp container probes | ||
| if reflect.DeepEqual(podContainerProbe.Probes, generatePodContainersProbe(pod, containersLivenessProbeConfig)) { |
There was a problem hiding this comment.
duplicated calls to generatePodContainersProbe(3 times), just save the result of generatePodContainersProbe in a local variable e.g. newPodContainerProbes, and the isChanged is redundant if newPodContainerProbes is available
|
|
||
| func (r *ReconcileEnhancedLivenessProbeMapNodeProbe) retryOnConflictDelNodeProbeConfig(pod *v1.Pod) error { | ||
| nppClone := &appsv1alpha1.NodePodProbe{} | ||
| err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { |
There was a problem hiding this comment.
it is not necessary in a controller to retryonconflict, just return the error and reenqueue the pod
There was a problem hiding this comment.
done.
retryOnConflictDelNodeProbeConfig -> delNodeProbeConfig
| return nil | ||
| } | ||
|
|
||
| func (r *ReconcileEnhancedLivenessProbeMapNodeProbe) retryOnConflictAddNodeProbeConfig(pod *v1.Pod, |
There was a problem hiding this comment.
it is not necessary in a controller to retryonconflict, just return the error and reenqueue the pod
There was a problem hiding this comment.
done.
retryOnConflictAddNodeProbeConfig -> addNodeProbeConfig
| klog.Errorf("Failed to get node pod probe for pod: %s/%s, err: %v", pod.Namespace, pod.Name, err) | ||
| return err | ||
| } | ||
| newNppGenerated, err := r.generateNewNodePodProbe(pod) |
There was a problem hiding this comment.
pass plivenessProbeConfig to generateNewNodePodProbe, it is not necessary to parse the liveness probe twice
There was a problem hiding this comment.
func generateNewNodePodProbe(...) has been updated for using the podLivenessProbe from annotation as the paramater.
| if !ok { | ||
| return | ||
| } | ||
| p.queue(q, new) |
There was a problem hiding this comment.
if pod liveness probe does not change, it seems not necessary to enqueue again
There was a problem hiding this comment.
done, details:
// check the livenessProbe config from new and old pod annotation whether is changed or not getNewPodConfig := getRawEnhancedLivenessProbeConfig(new) getOldPodConfig := getRawEnhancedLivenessProbeConfig(old) if !reflect.DeepEqual(getNewPodConfig, getOldPodConfig) { p.queue(q, new) }
| }, 70*time.Second, time.Second).Should(gomega.Equal(util.DumpJSON(expectNodePodProbeSpec))) | ||
| }) | ||
|
|
||
| framework.ConformanceIt("case:2 Create one pod with liveness probe config and update the", func() { |
There was a problem hiding this comment.
case:2 Create one pod with liveness probe config and update the -> case:2 Create one pod with liveness probe config and update the nodePodProbe
| }) | ||
|
|
||
| framework.ConformanceIt("case:2 Create one pod with liveness probe config and update the", func() { | ||
| ginkgo.By("New a new container with liveness probe config") |
There was a problem hiding this comment.
add case3, Create two pod with liveness probe config and delete one pod to update the nodePodProbe
There was a problem hiding this comment.
1\add two pods and check the nodePodProbe config.
2\delete one pod and check the nodePodProbe config again.
Ⅰ. Describe what this PR does
A controller converts the pod livenessprobe config to nodePodProbe config.
This controller is the part of the enhanced livenessProbe solutions.
fix: #1535
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
when a pod with livenessProbe config is created, the native livenessProbe will be replaced to pod annotation and this controller converts to nodePodProbe config immediately.
Ⅳ. Special notes for reviews