pod probe marker controller #1075

zmberg · 2022-09-14T03:24:07Z

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

pkg/controller/podprobemarker/node_pod_probe.go

furykerry · 2022-09-15T02:20:04Z

pkg/controller/podprobemarker/pod_probe_marker_controller.go

+		return err
+	}
+	// remove podProbe from NodePodProbe.Spec
+	if !ppm.DeletionTimestamp.IsZero() {


shall we stop probing when pod with never restart policy exits, actually we can use IsPodActive directly

func getMatchingPods already use IsPodActive to filter pods.

if pod become not active, we should remove the pod from NodePodProe

pkg/controller/podprobemarker/pod_probe_marker_controller.go

furykerry · 2022-09-15T02:26:48Z

pkg/controller/podprobemarker/pod_probe_marker_controller.go

+	}
+	// add finalizer
+	if !controllerutil.ContainsFinalizer(ppm, PodProbeMarkerFinalizer) {
+		err = util.UpdateFinalizer(r.Client, ppm, util.AddFinalizerOpType, PodProbeMarkerFinalizer)


can we add the finalizer in webhook , so that we don't need to update podmarker itself

There is currently no logic for mutating webhook, is it a bit heavy for only finalizer?

pkg/controller/podprobemarker/ppm_event_handler.go

FillZpp · 2022-09-16T02:52:08Z

pkg/controller/podprobemarker/podprobemarker_event_handler.go

+	}
+	// remove pod probe from nodePodProbe.spec
+	if kubecontroller.IsPodActive(new) && new.Spec.NodeName != "" &&
+		old.DeletionTimestamp.IsZero() && !new.DeletionTimestamp.IsZero() {


If kubecontroller.IsPodActive(new) is true, then !new.DeletionTimestamp.IsZero() will always be false.

pkg/controller/podprobemarker/pod_probe_marker_controller.go

FillZpp · 2022-09-16T03:03:04Z

pkg/controller/podprobemarker/podprobemarker_event_handler.go

+	q.Add(reconcile.Request{
+		NamespacedName: types.NamespacedName{
+			Namespace: npp.Namespace,
+			Name:      fmt.Sprintf("%s#%s", ReconNodePodProbe, npp.Name),


Should it be ReconPodProbeMarker?

FillZpp · 2022-09-16T03:52:55Z

pkg/controller/podprobemarker/podprobemarker_event_handler.go

+		old.DeletionTimestamp.IsZero() && !new.DeletionTimestamp.IsZero() {
+
+		npp := &appsalphav1.NodePodProbe{}
+		if err := p.reader.Get(context.TODO(), client.ObjectKey{Name: new.Spec.NodeName}, npp); err != nil {


Should better use field index instead of enqueuing every NodePodProbe for every terminating pod

FillZpp · 2022-09-16T06:11:09Z

pkg/controller/podprobemarker/node_pod_probe.go

+)
+
+var (
+	nodeImageCreationDelayAfterNodeReady = time.Second * 30


nodePodProbeCreationDelayAfterNodeReady

pkg/controller/nodepodprobe/node_pod_probe_controller.go

furykerry · 2022-09-19T05:39:03Z

pkg/controller/nodepodprobe/node_pod_probe_controller.go

+		} else {
+			conStatus = corev1.ConditionFalse
+		}
+		ppmName, probeName := strings.Split(probeState.Name, "#")[0], strings.Split(probeState.Name, "#")[1]


since we can get pod probe marker name from NodePodProbe.probes[*].podProbeMarkerName, why we have to encode the podProbeMarkerName in the probe name ?

furykerry · 2022-09-19T05:43:07Z

pkg/controller/podprobemarker/pod_probe_marker_controller.go

+			podProbe.Probes = append(podProbe.Probes, appsv1alpha1.ContainerProbe{
+				Name:          fmt.Sprintf("%s#%s", ppm.Name, probe.Name),
+				ContainerName: probe.ContainerName,
+				Probe:         probe.Probe,


populate podProbeMarkerName using. probe.Name

pkg/controller/podprobemarker/podprobemarker_event_handler.go

Signed-off-by: liheng.zms <[email protected]>

furykerry · 2022-09-19T07:28:20Z

/lgtm

FillZpp · 2022-09-19T11:12:13Z

pkg/util/finalizer.go

+	RemoveFinalizerOpType FinalizerOpType = "Remove"
+)
+
+func UpdateFinalizer(c client.Client, object client.Object, op FinalizerOpType, finalizer string) error {


We don't have to define UpdateFinalizer function, just use Patch in the two places that want to add or remove finalizer.

FillZpp · 2022-09-20T06:29:46Z

/approve

kruise-bot · 2022-09-20T06:29:52Z

[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:

~~OWNERS~~ [FillZpp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: peng.xin <[email protected]> optimize workloadspread when suitable subset maxReplicas is niil (openkruise#1066) Signed-off-by: mingzhou.swx <[email protected]> Signed-off-by: mingzhou.swx <[email protected]> Co-authored-by: mingzhou.swx <[email protected]> Optimize performance of LabelSelector conversion (openkruise#1068) Signed-off-by: FillZpp <[email protected]> Signed-off-by: FillZpp <[email protected]> Support timezone for AdvancedCronJob (openkruise#1070) Signed-off-by: FillZpp <[email protected]> Signed-off-by: FillZpp <[email protected]> ignore if pod condition has been updated (openkruise#1074) Signed-off-by: mingzhou.swx <[email protected]> Signed-off-by: mingzhou.swx <[email protected]> Co-authored-by: mingzhou.swx <[email protected]> pod probe marker apis (openkruise#1073) * pod probe marker apis Signed-off-by: liheng.zms <[email protected]> * fix statefulset truncateHistory panic Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> pod probe marker controller (openkruise#1075) Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> consider whether patch field is matched when assign existing pods to subset (openkruise#1083) Signed-off-by: mingzhou.swx <[email protected]> Signed-off-by: mingzhou.swx <[email protected]> Co-authored-by: mingzhou.swx <[email protected]> pod probe marker webhook (openkruise#1078) Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> sidecarset support pods ns(kube-system, kube-public) (openkruise#1084) Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> pod probe marker proposal (openkruise#1053) Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> node pod probe daemon (openkruise#1077) Signed-off-by: liheng.zms <[email protected]> sidecarset support patch pod metadata proposal (openkruise#993) Signed-off-by: liheng.zms <[email protected]> Support predownload image in ads (openkruise#1057) Signed-off-by: Abner <[email protected]> * support predownload image for ads Add PreDownloadImageForDaemonSetUpdate featureGate (openkruise#1093) Signed-off-by: FillZpp <[email protected]> Signed-off-by: FillZpp <[email protected]> Add changelog for v1.3.0 (openkruise#1092) Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> fix sidecarset inject annotations abnormal (openkruise#1101) Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> fix InPlaceUpdateEnvFromMetadata bug (openkruise#1108) Co-authored-by: 郭已钦 <[email protected]> support uniteddeployment persistentVolumeClaimRetentionPolicy inherit from advancestatefulset template (openkruise#1110) Signed-off-by: cheyuexian <[email protected]> Signed-off-by: cheyuexian <[email protected]> Co-authored-by: cheyuexian <[email protected]> Add PreNormal Lifecycle Hook for CloneSet (openkruise#1071) * add pre-normal(pre-available) hook Signed-off-by: mingzhou.swx <[email protected]> * add pre-normal(pre-available) hook Signed-off-by: mingzhou.swx <[email protected]> Signed-off-by: mingzhou.swx <[email protected]> Co-authored-by: mingzhou.swx <[email protected]> partition support float percent (openkruise#1124) Signed-off-by: shiyan2016 <[email protected]> Signed-off-by: shiyan2016 <[email protected]> improve error hanlding of inplace update env from metadata (openkruise#1125) Signed-off-by: mingzhou.swx <[email protected]> Signed-off-by: mingzhou.swx <[email protected]> Co-authored-by: mingzhou.swx <[email protected]> pub support to configure Evict,Delete,Update Operation (openkruise#1126) Signed-off-by: liheng.zms <[email protected]> Signed-off-by: liheng.zms <[email protected]> feat: add watch whitelist feat: add watch whitelist add watch whitelist check add dynamic watch option feat: add persistent annotations format feat: add pps support custom workload Signed-off-by: peng.xin <[email protected]> fix: nodeSelector and nodeAffinity have not been modified Signed-off-by: peng.xin <[email protected]> fix: error string Signed-off-by: peng.xin <[email protected]> fix: error string Signed-off-by: peng.xin <[email protected]> fix: goimports Signed-off-by: peng.xin <[email protected]> refactor: refactoring ersistentPodAnnotations type Signed-off-by: peng.xin <[email protected]> - add statefulet group check - modify the pod create processing logic Signed-off-by: peng.xin <[email protected]> refactor: pps watch of whitelist workload Signed-off-by: peng.xin <[email protected]> fix: add statefulsetlike's pod controller Signed-off-by: peng.xin <[email protected]>

kruise-bot requested review from Fei-Guo and FillZpp September 14, 2022 03:24

kruise-bot added the size/XXL label Sep 14, 2022

zmberg force-pushed the pod-probe-marker-controller branch 2 times, most recently from 2a7d8a1 to 015b43b Compare September 14, 2022 03:30

furykerry reviewed Sep 14, 2022

View reviewed changes

pkg/controller/podprobemarker/node_pod_probe.go Outdated Show resolved Hide resolved

furykerry reviewed Sep 14, 2022

View reviewed changes

pkg/controller/podprobemarker/node_pod_probe.go Outdated Show resolved Hide resolved

zmberg force-pushed the pod-probe-marker-controller branch 2 times, most recently from d388010 to 94afac6 Compare September 15, 2022 02:15

furykerry reviewed Sep 15, 2022

View reviewed changes

zmberg force-pushed the pod-probe-marker-controller branch 3 times, most recently from 235fd8e to 4e121cb Compare September 16, 2022 06:00

FillZpp reviewed Sep 16, 2022

View reviewed changes

zmberg force-pushed the pod-probe-marker-controller branch 2 times, most recently from 1352504 to 656363e Compare September 19, 2022 02:30

furykerry reviewed Sep 19, 2022

View reviewed changes

pkg/controller/nodepodprobe/node_pod_probe_controller.go Outdated Show resolved Hide resolved

zmberg force-pushed the pod-probe-marker-controller branch from 656363e to 7ac82dd Compare September 19, 2022 02:38

furykerry reviewed Sep 19, 2022

View reviewed changes

zmberg force-pushed the pod-probe-marker-controller branch from 7ac82dd to f542471 Compare September 19, 2022 06:22

pod probe marker controller

6b7dbf9

Signed-off-by: liheng.zms <[email protected]>

zmberg force-pushed the pod-probe-marker-controller branch from f542471 to 6b7dbf9 Compare September 19, 2022 07:07

kruise-bot assigned furykerry Sep 19, 2022

kruise-bot added the lgtm label Sep 19, 2022

FillZpp reviewed Sep 19, 2022

View reviewed changes

kruise-bot added the approved label Sep 20, 2022

kruise-bot merged commit 984d649 into openkruise:master Sep 20, 2022

zmberg deleted the pod-probe-marker-controller branch September 26, 2022 10:02

zmberg mentioned this pull request Jan 5, 2023

[feature request] PodMarker: mark Pods by number, labels, nodes, probes #640

Open

ppbits pushed a commit to ppbits/kruise that referenced this pull request Apr 4, 2024

pod probe marker controller (openkruise#1075)

c92a947

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pod probe marker controller #1075

pod probe marker controller #1075

zmberg commented Sep 14, 2022

furykerry Sep 15, 2022

zmberg Sep 15, 2022

furykerry Sep 15, 2022

furykerry Sep 15, 2022

zmberg Sep 15, 2022

FillZpp Sep 16, 2022

FillZpp Sep 16, 2022

FillZpp Sep 16, 2022

FillZpp Sep 16, 2022

furykerry Sep 19, 2022

furykerry Sep 19, 2022

furykerry commented Sep 19, 2022

FillZpp Sep 19, 2022

FillZpp commented Sep 20, 2022

kruise-bot commented Sep 20, 2022

pod probe marker controller #1075

pod probe marker controller #1075

Conversation

zmberg commented Sep 14, 2022

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furykerry commented Sep 19, 2022

Choose a reason for hiding this comment

FillZpp commented Sep 20, 2022

kruise-bot commented Sep 20, 2022