sync status of pod #1913

xyz2277 · 2022-05-30T08:23:11Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
When I try to propagate or promote a pod, I found that the status of pod is always pending and never changed.

Which issue(s) this PR fixes:
Fixes #1988

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`:  `interpreter framework` starts to support Pod status aggregation.

pkg/resourceinterpreter/defaultinterpreter/aggregatestatus.go

XiShanYongYe-Chang · 2022-05-30T09:14:43Z

pkg/resourceinterpreter/defaultinterpreter/aggregatestatus.go

+	newStatus := &corev1.PodStatus{}
+	for _, item := range aggregatedStatusItems {
+		if item.Status == nil {
+			continue
+		}
+		if err = json.Unmarshal(item.Status.Raw, newStatus); err != nil {
+			return nil, err
+		}
+		klog.V(3).Infof("Grab pod(%s/%s) status from cluster(%s), phase: %s, qosClass: %s", newStatus.Phase, newStatus.QOSClass)
+	}


This logic looks like it only unmarshals the latest object in the items.

Actually, we have not found a good method to aggregate the pod status. The PodStatus can not represent the pod status in multiple clusters figuratively.

This logic looks like it only unmarshals the latest object in the items.

Actually, we have not found a good method to aggregate the pod status. The PodStatus can not represent the pod status in multiple clusters figuratively.

This loop only exec once. And aggregating the pod status do not involve multiple clusters.This is just a independent pod resource, not pod in deployment or something else.

When we propagate to more than one cluster, there will be more than one item.

When we propagate to more than one cluster, there will be more than one item.

you are right.
So can i set Pod status Ready only when all Pods are in that state?

I think it's maybe okay.

xyz2277 · 2022-05-30T09:39:11Z

This is how I propagate a pod.

@RainbowMango @XiShanYongYe-Chang

RainbowMango · 2022-05-31T02:52:37Z

Remember to update the documents as well. Thanks.
https://github.com/karmada-io/karmada/blob/master/docs/userguide/customizing-resource-interpreter.md#aggregatestatus

xyz2277 · 2022-05-31T02:55:58Z

Remember to update the documents as well. Thanks. https://github.com/karmada-io/karmada/blob/master/docs/userguide/customizing-resource-interpreter.md#aggregatestatus

okey

xyz2277 · 2022-05-31T08:16:03Z

/cc @RainbowMango @XiShanYongYe-Chang

XiShanYongYe-Chang · 2022-05-31T09:38:27Z

@xyz2277 thanks for your contribution.

/assign

XiShanYongYe-Chang · 2022-05-31T11:37:16Z

pkg/resourceinterpreter/defaultinterpreter/aggregatestatus.go

+
+		for _, containerStatus := range temp.ContainerStatuses {
+			tempStatus := containerStatus
+			newStatus.ContainerStatuses = append(newStatus.ContainerStatuses, tempStatus)


IMOP, the status of the collection container does not appear to be required.

IMOP, the status of the collection container does not appear to be required.

The status of containers is needed to be collected. Or the column of READY will always be 0/1.

The status of containers is needed to be collected. Or the column of READY will always be 0/1.

I'm not sure about it. I thought the Ready status is based on the .status.phase. @lonelyCZ might know it.

The status of containers is needed to be collected. Or the column of READY will always be 0/1.

I'm not sure about it. I thought the Ready status is based on the .status.phase. @lonelyCZ might know it.

I've already tried. ready status is based on the status of the container.

Hi @xyz2277, how about only collecting the fields we need.

Hi @xyz2277, how about only collecting the fields we need.

I tried too. When you see pod details, a lot of empty field will be found.

It will be ok.

XiShanYongYe-Chang · 2022-05-31T11:39:09Z

pkg/resourceinterpreter/defaultinterpreter/aggregatestatus.go

+	if reflect.DeepEqual(pod.Status, *newStatus) {
+		klog.V(3).Infof("ignore update pod(%s/%s) status as up to date", pod.Namespace, pod.Name)
+		return object, nil
+	}


This logic maybe needs to move back after newStatus.Phase assignment.

XiShanYongYe-Chang

The kubectl output format for Pod resource looks like:

# kubectl get po
NAME    READY   STATUS    RESTARTS   AGE
nginx   2/1     Running   0          9m19s

XiShanYongYe-Chang · 2022-06-01T08:52:42Z

pkg/resourceinterpreter/defaultinterpreter/aggregatestatus.go

+
+		for _, containerStatus := range temp.ContainerStatuses {
+			tempStatus := containerStatus
+			newStatus.ContainerStatuses = append(newStatus.ContainerStatuses, tempStatus)


Hi @xyz2277, how about only collecting the fields we need.

XiShanYongYe-Chang · 2022-06-01T08:55:20Z

pkg/resourceinterpreter/defaultinterpreter/aggregatestatus.go

+		for _, containerStatus := range temp.ContainerStatuses {
+			tempStatus := containerStatus
+			newStatus.ContainerStatuses = append(newStatus.ContainerStatuses, tempStatus)
+			if containerStatus.Ready {


Here judgement can refer to: https://github.com/kubernetes/kubernetes/blob/8415ae647d2c433c89910a0e677094e3a20ffb2b/pkg/printers/internalversion/printers.go#L822-L824

Here judgement can refer to: https://github.com/kubernetes/kubernetes/blob/8415ae647d2c433c89910a0e677094e3a20ffb2b/pkg/printers/internalversion/printers.go#L822-L824

do you mean when container is ready, then we can collect that containerStatus?

The judgment here may not be strong enough.

container.Ready && container.State.Running != nil

The judgment here may not be strong enough.

container.Ready && container.State.Running != nil

got it~

RainbowMango · 2022-06-02T01:42:03Z

Just a reminder, please don't forget to add a unit test for it and update the documents I mentioned above.

xyz2277 · 2022-06-02T02:13:39Z

Just a reminder, please don't forget to add a unit test for it and update the documents I mentioned above.

Got it!

xyz2277 · 2022-06-06T04:21:30Z

/cc @RainbowMango

RainbowMango · 2022-06-06T12:23:04Z

Can you share the test report with the new patch? Such as propagating a Pod to at least 2 clusters.

XiShanYongYe-Chang

How about modify like this:

        newStatus := &corev1.PodStatus{}
        newStatus.ContainerStatuses = make([]corev1.ContainerStatus, 0)
-       readySum := 0
-       containerSum := 0
+       runningFlag := true
        for _, item := range aggregatedStatusItems {
                if item.Status == nil {
+                       runningFlag = false
                        continue
                }

@@ -286,19 +286,22 @@ func aggregatePodStatus(object *unstructured.Unstructured, aggregatedStatusItems
                        return nil, err
                }

+               if temp.Phase != corev1.PodRunning {
+                       runningFlag = false
+               }
+
                for _, containerStatus := range temp.ContainerStatuses {
-                       tempStatus := containerStatus
-                       newStatus.ContainerStatuses = append(newStatus.ContainerStatuses, tempStatus)
-                       if containerStatus.Ready && containerStatus.State.Running != nil {
-                               readySum++
+                       tempStatus := corev1.ContainerStatus{
+                               Ready: containerStatus.Ready,
+                               State: containerStatus.State,
                        }
-                       containerSum++
+                       newStatus.ContainerStatuses = append(newStatus.ContainerStatuses, tempStatus)
                }
                klog.V(3).Infof("Grab pod(%s/%s) status from cluster(%s), phase: %s", pod.Namespace,
                        pod.Name, item.ClusterName, temp.Phase)
        }

-       if containerSum == readySum {
+       if runningFlag {
                newStatus.Phase = corev1.PodRunning

xyz2277 · 2022-06-07T01:58:25Z

Can you share the test report with the new patch? Such as propagating a Pod to at least 2 clusters.

case1:1 cluster, 1 pod

case2: 2 clusters,1 pod

case3: 2 clusters, 1 pod(one pod failed in one cluster)

xyz2277 · 2022-06-07T02:02:42Z

How about modify like this:

        newStatus := &corev1.PodStatus{}
        newStatus.ContainerStatuses = make([]corev1.ContainerStatus, 0)
-       readySum := 0
-       containerSum := 0
+       runningFlag := true
        for _, item := range aggregatedStatusItems {
                if item.Status == nil {
+                       runningFlag = false
                        continue
                }

@@ -286,19 +286,22 @@ func aggregatePodStatus(object *unstructured.Unstructured, aggregatedStatusItems
                        return nil, err
                }

+               if temp.Phase != corev1.PodRunning {
+                       runningFlag = false
+               }
+
                for _, containerStatus := range temp.ContainerStatuses {
-                       tempStatus := containerStatus
-                       newStatus.ContainerStatuses = append(newStatus.ContainerStatuses, tempStatus)
-                       if containerStatus.Ready && containerStatus.State.Running != nil {
-                               readySum++
+                       tempStatus := corev1.ContainerStatus{
+                               Ready: containerStatus.Ready,
+                               State: containerStatus.State,
                        }
-                       containerSum++
+                       newStatus.ContainerStatuses = append(newStatus.ContainerStatuses, tempStatus)
                }
                klog.V(3).Infof("Grab pod(%s/%s) status from cluster(%s), phase: %s", pod.Namespace,
                        pod.Name, item.ClusterName, temp.Phase)
        }

-       if containerSum == readySum {
+       if runningFlag {
                newStatus.Phase = corev1.PodRunning

yeah, it looks more clean and concise

RainbowMango · 2022-06-10T09:45:46Z

kindly ping @xyz2277

chaunceyjiang · 2022-06-10T10:26:29Z

#1913 (comment)

Why not collect all the information?

karmada 的目标不是像操作单集群一下操作多集群嘛？

如果不收集这些信息，对一些可视化页面可能不是友好。

HostIP
Phase
PodIP 
QOSClass 
PodIPs 
Conditions

RainbowMango · 2022-06-11T02:38:15Z

并不是所有信息都可以汇聚到resource template的，比如HostIP，如果Pod分布于多个集群，多个HostIP无法同时体现在resource template中。状态汇聚是有局限性的。具体需要什么状态根据实际应用场景再行考虑。

chaunceyjiang · 2022-06-13T01:33:26Z

并不是所有信息都可以汇聚到resource template的，比如HostIP，如果Pod分布于多个集群，多个HostIP无法同时体现在resource template中。状态汇聚是有局限性的。具体需要什么状态根据实际应用场景再行考虑。

Got it

Signed-off-by: bruce <[email protected]>

xyz2277 · 2022-06-13T07:32:21Z

/cc @RainbowMango @XiShanYongYe-Chang

XiShanYongYe-Chang

Thanks for your contribution!

/lgtm

RainbowMango

/approve

Note: I updated the PR description.(kind and release notes.)

karmada-bot · 2022-06-13T08:18:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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~~ [RainbowMango]

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

karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label May 30, 2022

karmada-bot requested review from iawia002 and XiShanYongYe-Chang May 30, 2022 08:23

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 30, 2022

RainbowMango reviewed May 30, 2022

View reviewed changes

pkg/resourceinterpreter/defaultinterpreter/aggregatestatus.go Outdated Show resolved Hide resolved

XiShanYongYe-Chang reviewed May 30, 2022

View reviewed changes

xyz2277 force-pushed the karmada-zyx-11 branch from e4bb942 to f17bb23 Compare May 31, 2022 02:21

karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 31, 2022

xyz2277 force-pushed the karmada-zyx-11 branch 2 times, most recently from 3984c62 to ca50789 Compare May 31, 2022 07:30

karmada-bot requested review from RainbowMango and XiShanYongYe-Chang May 31, 2022 08:16

karmada-bot assigned XiShanYongYe-Chang May 31, 2022

XiShanYongYe-Chang reviewed May 31, 2022

View reviewed changes

xyz2277 force-pushed the karmada-zyx-11 branch from ca50789 to 7dba38a Compare June 1, 2022 05:42

XiShanYongYe-Chang reviewed Jun 1, 2022

View reviewed changes

xyz2277 force-pushed the karmada-zyx-11 branch from 7dba38a to f030487 Compare June 2, 2022 02:05

xyz2277 force-pushed the karmada-zyx-11 branch from f030487 to 553cd20 Compare June 6, 2022 02:52

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2022

XiShanYongYe-Chang reviewed Jun 7, 2022

View reviewed changes

RainbowMango mentioned this pull request Jun 10, 2022

Why does Built-in Interpreter AggregateStatus not contain Pod #1988

Closed

sync status of pod

7383526

Signed-off-by: bruce <[email protected]>

xyz2277 force-pushed the karmada-zyx-11 branch from 553cd20 to 7383526 Compare June 13, 2022 07:16

karmada-bot requested a review from XiShanYongYe-Chang June 13, 2022 07:32

XiShanYongYe-Chang reviewed Jun 13, 2022

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2022

RainbowMango added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jun 13, 2022

RainbowMango approved these changes Jun 13, 2022

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2022

karmada-bot merged commit 9a0ccad into karmada-io:master Jun 13, 2022

RainbowMango added this to the v1.3 milestone Aug 30, 2022

neteric mentioned this pull request Mar 21, 2023

collect full status info when propagate pods to only one cluster #3304

Closed

sync status of pod #1913

sync status of pod #1913

Conversation

xyz2277 commented May 30, 2022 • edited by RainbowMango Loading

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

xyz2277 commented May 30, 2022

RainbowMango commented May 31, 2022

xyz2277 commented May 31, 2022

xyz2277 commented May 31, 2022

XiShanYongYe-Chang commented May 31, 2022

Choose a reason for hiding this comment

xyz2277 Jun 1, 2022 • edited Loading

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

XiShanYongYe-Chang left a 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

RainbowMango commented Jun 2, 2022

xyz2277 commented Jun 2, 2022

xyz2277 commented Jun 6, 2022

RainbowMango commented Jun 6, 2022

XiShanYongYe-Chang left a comment • edited Loading

Choose a reason for hiding this comment

xyz2277 commented Jun 7, 2022

xyz2277 commented Jun 7, 2022

RainbowMango commented Jun 10, 2022

chaunceyjiang commented Jun 10, 2022

RainbowMango commented Jun 11, 2022

chaunceyjiang commented Jun 13, 2022

xyz2277 commented Jun 13, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jun 13, 2022

xyz2277 commented May 30, 2022 •

edited by RainbowMango

Loading

xyz2277 Jun 1, 2022 •

edited

Loading

XiShanYongYe-Chang left a comment •

edited

Loading