Skip to content

add events for ResourceInterpreter #5541

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.

Sign up for GitHub

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

Open
whitewindmills wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add events for ResourceInterpreter #5541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions pkg/controllers/status/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,10 @@ func updateResourceStatus(
newObj, err := interpreter.AggregateStatus(resource, bindingStatus.AggregatedStatus)
if err != nil {
klog.Errorf("Failed to aggregate status for resource template(%s/%s/%s), Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err)
eventRecorder.Event(resource, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error())
return err
}
eventRecorder.Event(resource, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, "Aggregated status to object successfully.")
Comment on lines +147 to +150
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to adjust the position of these two events?

Copy link
Member Author

@whitewindmills whitewindmills Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to add events after each resource interpretation function to reflect whether the resource interpretation is successful.

Copy link
Member

@chaosi-zju chaosi-zju Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of justification, but I have some concerns:

the event events.EventReasonAggregateStatusSucceed is used more than this place, and thus it will have so different meaning, whether differentiation or unification is needed?

Copy link
Member Author

@whitewindmills whitewindmills Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
but I don't have a suitable name to distinguish, do you have one?

Copy link
Member

@chaosi-zju chaosi-zju Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m looking for references in other interpreters and found that functions like GetDependencies have an event GetDependenciesFailed, ReflectStatus has ReflectStatusFailed, and InterpretHealth has InterpretHealthFailed. These align with the meaning introduced in your PR, so naming this event AggregateStatusFailed in the position of your updated lines seems reasonable.

However, in workstatus.go, there are two place also using this event name which seem to represent successfully collected and updated status of RB. To differentiate, perhaps SyncBindingStatusFailed would be more appropriate? (ps: I'm also poor at naming.)

Copy link
Member

@RainbowMango RainbowMango Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you want the AggregateStatusSucceed and AggregateStatusFailed dedicated to indicating resource interpreter status? That means that updating failure after interpreter.AggregateStatus, should not report event.

Copy link
Member Author

@whitewindmills whitewindmills Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, such update error can be fixed by reconciliation.

Copy link
Member Author

@whitewindmills whitewindmills Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @RainbowMango

Copy link
Member

@RainbowMango RainbowMango Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But at the time emitting the success event, the aggregate operation might still not happen, in addition, there might be more than one events due to the retry loop.

I'm afraid it is not acceptable.

Copy link
Member Author

@whitewindmills whitewindmills Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other functions of the current resource interpretation framework are also designed in this way, so another purpose of this PR is to be consistent with them.

  • ReflectStatus:

    karmada/pkg/controllers/status/work_status_controller.go

    Lines 359 to 366 in 836b878

    statusRaw, err := c.ResourceInterpreter.ReflectStatus(clusterObj)
    if err != nil {
    klog.Errorf("Failed to reflect status for object(%s/%s/%s) with resourceInterpreter, err: %v",
    clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err)
    c.EventRecorder.Eventf(work, corev1.EventTypeWarning, events.EventReasonReflectStatusFailed, "Reflect status for object(%s/%s/%s) failed, err: %s.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err.Error())
    return err
    }
    c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonReflectStatusSucceed, "Reflect status for object(%s/%s/%s) succeed.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())

  • InterpretHealth:

    karmada/pkg/controllers/status/work_status_controller.go

    Lines 371 to 380 in 836b878

    healthy, err := c.ResourceInterpreter.InterpretHealth(clusterObj)
    if err != nil {
    resourceHealth = workv1alpha1.ResourceUnknown
    c.EventRecorder.Eventf(work, corev1.EventTypeWarning, events.EventReasonInterpretHealthFailed, "Interpret health of object(%s/%s/%s) failed, err: %s.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err.Error())
    } else if healthy {
    resourceHealth = workv1alpha1.ResourceHealthy
    c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonInterpretHealthSucceed, "Interpret health of object(%s/%s/%s) as healthy.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())
    } else {
    resourceHealth = workv1alpha1.ResourceUnhealthy
    c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonInterpretHealthSucceed, "Interpret health of object(%s/%s/%s) as unhealthy.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())

  • GetDependencies:

    karmada/pkg/dependenciesdistributor/dependencies_distributor.go

    Lines 271 to 277 in 836b878

    dependencies, err := d.ResourceInterpreter.GetDependencies(workload)
    if err != nil {
    klog.Errorf("Failed to customize dependencies for %s(%s), %v", workload.GroupVersionKind(), workload.GetName(), err)
    d.EventRecorder.Eventf(workload, corev1.EventTypeWarning, events.EventReasonGetDependenciesFailed, err.Error())
    return reconcile.Result{}, err
    }
    d.EventRecorder.Eventf(workload, corev1.EventTypeNormal, events.EventReasonGetDependenciesSucceed, "Get dependencies(%+v) succeed.", dependencies)


oldStatus, _, _ := unstructured.NestedFieldNoCopy(resource.Object, "status")
newStatus, _, _ := unstructured.NestedFieldNoCopy(newObj.Object, "status")
Expand All @@ -159,14 +161,9 @@ func updateResourceStatus(
return err
}

eventRecorder.Event(resource, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, "Update Resource with AggregatedStatus successfully.")
klog.V(3).Infof("Update resource(%s/%s/%s) status successfully.", gvr, resource.GetNamespace(), resource.GetName())

return nil
}); err != nil {
if resource != nil {
eventRecorder.Event(resource, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error())
}
return err
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/detector/detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,8 +784,10 @@ func (d *ResourceDetector) BuildResourceBinding(object *unstructured.Unstructure
replicas, replicaRequirements, err := d.ResourceInterpreter.GetReplicas(object)
if err != nil {
klog.Errorf("Failed to customize replicas for %s(%s), %v", object.GroupVersionKind(), object.GetName(), err)
d.EventRecorder.Event(object, corev1.EventTypeWarning, events.EventReasonGetReplicasFailed, err.Error())
return nil, err
}
d.EventRecorder.Event(object, corev1.EventTypeNormal, events.EventReasonGetReplicasSucceed, "Get replicas of resource template successfully.")
propagationBinding.Spec.Replicas = replicas
propagationBinding.Spec.ReplicaRequirements = replicaRequirements
}
Expand Down Expand Up @@ -828,8 +830,10 @@ func (d *ResourceDetector) BuildClusterResourceBinding(object *unstructured.Unst
replicas, replicaRequirements, err := d.ResourceInterpreter.GetReplicas(object)
if err != nil {
klog.Errorf("Failed to customize replicas for %s(%s), %v", object.GroupVersionKind(), object.GetName(), err)
d.EventRecorder.Event(object, corev1.EventTypeWarning, events.EventReasonGetReplicasFailed, err.Error())
return nil, err
}
d.EventRecorder.Event(object, corev1.EventTypeNormal, events.EventReasonGetReplicasSucceed, "Get replicas of resource template successfully.")
binding.Spec.Replicas = replicas
binding.Spec.ReplicaRequirements = replicaRequirements
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/events/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ const (
EventReasonGetDependenciesSucceed = "GetDependenciesSucceed"
// EventReasonGetDependenciesFailed indicates get dependencies of resource template failed.
EventReasonGetDependenciesFailed = "GetDependenciesFailed"
// EventReasonGetReplicasSucceed indicates get replicas of resource template succeed.
EventReasonGetReplicasSucceed = "GetReplicasSucceed"
// EventReasonGetReplicasFailed indicates get replicas of resource template failed.
EventReasonGetReplicasFailed = "GetReplicasFailed"
// EventReasonPreemptPolicySucceed indicates policy preemption of resource template succeed.
EventReasonPreemptPolicySucceed = "PreemptPolicySucceed"
// EventReasonPreemptPolicyFailed indicates policy preemption of resource template failed.
Expand Down