Skip to content
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

Align federated DaemonSet's observedGeneration semantics with its native #5165

Merged
merged 1 commit into from
Jul 13, 2024
Merged

Align federated DaemonSet's observedGeneration semantics with its native #5165

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
50 changes: 28 additions & 22 deletions pkg/resourceinterpreter/default/native/aggregatestatus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
policyv1 "k8s.io/api/policy/v1"
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -279,39 +280,44 @@ func aggregateDaemonSetStatus(object *unstructured.Unstructured, aggregatedStatu

oldStatus := &daemonSet.Status
newStatus := &appsv1.DaemonSetStatus{}
observedLatestResourceTemplateGenerationCount := 0
for _, item := range aggregatedStatusItems {
if item.Status == nil {
continue
}
temp := &appsv1.DaemonSetStatus{}
if err = json.Unmarshal(item.Status.Raw, temp); err != nil {
member := &WrappedDaemonSetStatus{}
if err = json.Unmarshal(item.Status.Raw, member); err != nil {
return nil, err
}
klog.V(3).Infof("Grab daemonSet(%s/%s) status from cluster(%s), currentNumberScheduled: %d, desiredNumberScheduled: %d, numberAvailable: %d, numberMisscheduled: %d, numberReady: %d, updatedNumberScheduled: %d, numberUnavailable: %d",
daemonSet.Namespace, daemonSet.Name, item.ClusterName, temp.CurrentNumberScheduled, temp.DesiredNumberScheduled, temp.NumberAvailable, temp.NumberMisscheduled, temp.NumberReady, temp.UpdatedNumberScheduled, temp.NumberUnavailable)
daemonSet.Namespace, daemonSet.Name, item.ClusterName, member.CurrentNumberScheduled, member.DesiredNumberScheduled, member.NumberAvailable, member.NumberMisscheduled, member.NumberReady, member.UpdatedNumberScheduled, member.NumberUnavailable)

// always set 'observedGeneration' with current generation(.metadata.generation)
// which is the generation Karmada 'observed'.
// The 'observedGeneration' is mainly used by GitOps tools(like 'Argo CD') to assess the health status.
// For more details, please refer to https://argo-cd.readthedocs.io/en/stable/operator-manual/health/.
// `memberStatus.ObservedGeneration >= memberStatus.Generation` means the member's status corresponds the latest spec revision of the member DaemonSet.
// `memberStatus.ResourceTemplateGeneration >= daemonSet.Generation` means the member DaemonSet has been aligned with the latest spec revision of federated DaemonSet.
// If both conditions are met, we consider the member's status corresponds the latest spec revision of federated DaemonSet.
if member.ObservedGeneration >= member.Generation &&
member.ResourceTemplateGeneration >= daemonSet.Generation {
observedLatestResourceTemplateGenerationCount++
}

newStatus.CurrentNumberScheduled += member.CurrentNumberScheduled
newStatus.DesiredNumberScheduled += member.DesiredNumberScheduled
newStatus.NumberAvailable += member.NumberAvailable
newStatus.NumberMisscheduled += member.NumberMisscheduled
newStatus.NumberReady += member.NumberReady
newStatus.UpdatedNumberScheduled += member.UpdatedNumberScheduled
newStatus.NumberUnavailable += member.NumberUnavailable
}

// The 'observedGeneration' is mainly used by GitOps tools(like 'Argo CD') to assess the health status.
// For more details, please refer to https://argo-cd.readthedocs.io/en/stable/operator-manual/health/.
if observedLatestResourceTemplateGenerationCount == len(aggregatedStatusItems) {
newStatus.ObservedGeneration = daemonSet.Generation
newStatus.CurrentNumberScheduled += temp.CurrentNumberScheduled
newStatus.DesiredNumberScheduled += temp.DesiredNumberScheduled
newStatus.NumberAvailable += temp.NumberAvailable
newStatus.NumberMisscheduled += temp.NumberMisscheduled
newStatus.NumberReady += temp.NumberReady
newStatus.UpdatedNumberScheduled += temp.UpdatedNumberScheduled
newStatus.NumberUnavailable += temp.NumberUnavailable
} else {
newStatus.ObservedGeneration = oldStatus.ObservedGeneration
}

if oldStatus.ObservedGeneration == newStatus.ObservedGeneration &&
oldStatus.CurrentNumberScheduled == newStatus.CurrentNumberScheduled &&
oldStatus.DesiredNumberScheduled == newStatus.DesiredNumberScheduled &&
oldStatus.NumberAvailable == newStatus.NumberAvailable &&
oldStatus.NumberMisscheduled == newStatus.NumberMisscheduled &&
oldStatus.NumberReady == newStatus.NumberReady &&
oldStatus.UpdatedNumberScheduled == newStatus.UpdatedNumberScheduled &&
oldStatus.NumberUnavailable == newStatus.NumberUnavailable {
if equality.Semantic.DeepEqual(oldStatus, newStatus) {
Copy link
Member

Choose a reason for hiding this comment

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

Other fields in the status may affect the current judgment.

Copy link
Member Author

Choose a reason for hiding this comment

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

other fields should be empty.

Copy link
Member

Choose a reason for hiding this comment

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

If other fields are added in the future, such as condition, the judgment here will be invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there will be this problem. even if a new field is added, DaemonSet of Karmada control plane will have this field at the end, and this judgment is still legal.

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that other types of resources can be treated the same way?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but I only modify this because of a lint error: cyclomatic complexity too high 😂

Copy link
Member

Choose a reason for hiding this comment

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

okay
/lgtm
/cc @yike21 @veophi

Copy link
Member

Choose a reason for hiding this comment

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

thanks!
/lgtm

klog.V(3).Infof("Ignore update daemonSet(%s/%s) status as up to date", daemonSet.Namespace, daemonSet.Name)
return object, nil
}
Expand Down
38 changes: 28 additions & 10 deletions pkg/resourceinterpreter/default/native/reflectstatus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,10 @@ func reflectDeploymentStatus(object *unstructured.Unstructured) (*runtime.RawExt
}

grabStatus := &WrappedDeploymentStatus{
Generation: object.GetGeneration(),
ResourceTemplateGeneration: resourceTemplateGenerationInt,
FederatedGeneration: FederatedGeneration{
Generation: object.GetGeneration(),
ResourceTemplateGeneration: resourceTemplateGenerationInt,
},
DeploymentStatus: appsv1.DeploymentStatus{
Replicas: deploymentStatus.Replicas,
UpdatedReplicas: deploymentStatus.UpdatedReplicas,
Expand Down Expand Up @@ -194,15 +196,31 @@ func reflectDaemonSetStatus(object *unstructured.Unstructured) (*runtime.RawExte
return nil, fmt.Errorf("failed to convert DaemonSetStatus from map[string]interface{}: %v", err)
}

grabStatus := appsv1.DaemonSetStatus{
CurrentNumberScheduled: daemonSetStatus.CurrentNumberScheduled,
DesiredNumberScheduled: daemonSetStatus.DesiredNumberScheduled,
NumberAvailable: daemonSetStatus.NumberAvailable,
NumberMisscheduled: daemonSetStatus.NumberMisscheduled,
NumberReady: daemonSetStatus.NumberReady,
UpdatedNumberScheduled: daemonSetStatus.UpdatedNumberScheduled,
NumberUnavailable: daemonSetStatus.NumberUnavailable,
resourceTemplateGenerationInt := int64(0)
resourceTemplateGenerationStr := util.GetAnnotationValue(object.GetAnnotations(), v1alpha2.ResourceTemplateGenerationAnnotationKey)
err = runtime.Convert_string_To_int64(&resourceTemplateGenerationStr, &resourceTemplateGenerationInt, nil)
if err != nil {
klog.Errorf("Failed to parse DaemonSet(%s/%s) generation from annotation(%s:%s): %v", object.GetNamespace(), object.GetName(), v1alpha2.ResourceTemplateGenerationAnnotationKey, resourceTemplateGenerationStr, err)
return nil, err
}

grabStatus := &WrappedDaemonSetStatus{
FederatedGeneration: FederatedGeneration{
Generation: object.GetGeneration(),
ResourceTemplateGeneration: resourceTemplateGenerationInt,
},
DaemonSetStatus: appsv1.DaemonSetStatus{
CurrentNumberScheduled: daemonSetStatus.CurrentNumberScheduled,
DesiredNumberScheduled: daemonSetStatus.DesiredNumberScheduled,
NumberAvailable: daemonSetStatus.NumberAvailable,
NumberMisscheduled: daemonSetStatus.NumberMisscheduled,
NumberReady: daemonSetStatus.NumberReady,
UpdatedNumberScheduled: daemonSetStatus.UpdatedNumberScheduled,
NumberUnavailable: daemonSetStatus.NumberUnavailable,
ObservedGeneration: daemonSetStatus.ObservedGeneration,
},
}

return helper.BuildStatusRawExtension(grabStatus)
}

Expand Down
19 changes: 15 additions & 4 deletions pkg/resourceinterpreter/default/native/status_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,24 @@ package native

import appsv1 "k8s.io/api/apps/v1"

// WrappedDeploymentStatus is a wrapper for appsv1.DeploymentStatus.
type WrappedDeploymentStatus struct {
appsv1.DeploymentStatus `json:",inline"`

// FederatedGeneration holds the generation of the same resource in the member cluster and the Karmada control plane.
type FederatedGeneration struct {
whitewindmills marked this conversation as resolved.
Show resolved Hide resolved
// Generation holds the generation(.metadata.generation) of resource running on member cluster.
Generation int64 `json:"generation,omitempty"`

// ResourceTemplateGeneration holds the value of annotation resourcetemplate.karmada.io/generation grabbed
// from resource running on member cluster.
ResourceTemplateGeneration int64 `json:"resourceTemplateGeneration,omitempty"`
}

// WrappedDeploymentStatus is a wrapper for appsv1.DeploymentStatus.
type WrappedDeploymentStatus struct {
FederatedGeneration `json:",inline"`
appsv1.DeploymentStatus `json:",inline"`
}

// WrappedDaemonSetStatus is a wrapper for appsv1.DaemonSetStatus.
type WrappedDaemonSetStatus struct {
FederatedGeneration `json:",inline"`
appsv1.DaemonSetStatus `json:",inline"`
}