Implementing the AggregateStatus hook

[iawia002]

Signed-off-by: Xinzhao Xu [email protected]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Part of #1237

This patch implements the AggregateStatus hook, check resource interpreter webhook proposal for more info.

The original code to modify the status of core resources such as Deployment and Job has been changed to use default interpreters. After the update, the status collection of the sample deployment is the same as before:

$ kubectl get deploy

NAME    READY   UP-TO-DATE   AVAILABLE   AGE
nginx   2/2     2            2           56s

status:
  availableReplicas: 2
  observedGeneration: 1
  readyReplicas: 2
  replicas: 2
  updatedReplicas: 2

for custom resources, users only need to implement the corresponding webhook and return the patch data:

func (e *workloadInterpreter) responseWithExploreAggregateStatus(workload *workloadv1alpha1.Workload, req interpreter.Request) interpreter.Response {
	wantedWorkload := workload.DeepCopy()
	var readyReplicas int32
	for _, item := range req.AggregatedStatus {
		// NOTE: this is just an example, you need to change the method of updating status according to the actual situation
		if !item.Applied {
			continue
		}
		readyReplicas += 1
	}
	wantedWorkload.Status.ReadyReplicas = readyReplicas
	marshaledBytes, err := json.Marshal(wantedWorkload)
	if err != nil {
		return interpreter.Errored(http.StatusInternalServerError, err)
	}
	return interpreter.PatchResponseFromRaw(req.Object.Raw, marshaledBytes)
}

status:
  readyReplicas: 2

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Implementing the AggregateStatus hook.

[RainbowMango]

cc @XiShanYongYe-Chang
/assign

[XiShanYongYe-Chang]

Nice job! Thanks 👍

[iawia002]

All comments addressed, PTAL @RainbowMango @XiShanYongYe-Chang 56cd871

/hold
NOTE: I will squash the commits into one after you finished the review.

[XiShanYongYe-Chang]

Thanks, LGTM

/cc @RainbowMango

[iawia002]

/hold cancel

[RainbowMango]

We don't need to parse the .status, we can just compare the obj and newObj, since the Interpreter won't change anything but .status, so if obj != newObj means the status has been aggregated, then we do update operation.

In addition, we seem can't assume there is a .status field. See InterpretStatus.

[RainbowMango]

Something like this:

	if reflect.DeepEqual(obj, newObj) {
		klog.V(3).Infof("ignore update resource(%s/%s/%s) status as up to date", resource.Kind, resource.Namespace, resource.Name)
		return nil
	}

Maybe we can't use retry.RetryOnConflict for this case,

[iawia002]

Maybe we can't use retry.RetryOnConflict for this case

What is this for? I can compare two objects directly, but it doesn't seem to affect the use of RetryOnConflict, is it because the newObj may also not have status?

[RainbowMango]

but it doesn't seem to affect the use of RetryOnConflict, is it because the newObj may also not have status?

As after we get a new copy of the object, we have to set the status, do you mean to call the webhook again in retry?

[iawia002]

Nope, Is there a problem with the current retry method?

[RainbowMango]

The thing is

if err = unstructured.SetNestedField(obj.Object, newStatus, "status"); err != nil {
			return err
		}

Personally, I'm hesitant to assume there is a .status filed(Maybe no problem for 99.9% cases).

[iawia002]

I am not so familiar with SetNestedField, but by looking at the source code, if the original object does not have a status field, it should not report an error when setting for it, it will only report an error if the value is not compliant.

The workload object initially had no status field, but SetNestedField added it, as expected.

[RainbowMango]

I mean we can't assume all resource's status represent by .status, that might be .status-xxx or something else. That's why we design InterpretStatus operation for.

[iawia002]

I get your point now, it seems we can't use retry in this case, let's return the error directly to trigger its resync.

[RainbowMango]

Good Job! @iawia002
Generally looks good to me. Just two nits.

[RainbowMango]

/lgtm

Just the nolint need to be clarified.

[RainbowMango]

/lgtm
/approve

[karmada-bot]

[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