Skip to content

add dependency policy events #6795

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −1
Draft

add dependency policy events #6795

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
12 changes: 11 additions & 1 deletion pkg/dependenciesdistributor/dependencies_distributor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,12 +588,22 @@ func (d *DependenciesDistributor) createOrUpdateAttachedBinding(attachedBinding
// If the spec.Placement is nil, this means that existBinding is generated by the dependency mechanism.
// If the spec.Placement is not nil, then it must be generated by PropagationPolicy.
if existBinding.Spec.Placement == nil {
// dependency-generated RB: take values from the attached binding
existBinding.Spec.ConflictResolution = attachedBinding.Spec.ConflictResolution
existBinding.Spec.PreserveResourcesOnDeletion = attachedBinding.Spec.PreserveResourcesOnDeletion
} else {
// explicit RB: do not override conflict-related fields via dependency path
if attachedBinding.Spec.PreserveResourcesOnDeletion != nil || attachedBinding.Spec.ConflictResolution != "" {
Copy link

Choose a reason for hiding this comment

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

high

The current condition for triggering a warning event, attachedBinding.Spec.ConflictResolution != "", will always be true because ConflictResolution is a required field with a default value of "Abort". This will cause an event to be logged on every reconciliation for every dependency with an explicit policy, leading to excessive and noisy event logs.

A better approach is to check if the values from the dependency path (attachedBinding) actually differ from what's already on the explicit ResourceBinding (existBinding). This ensures that a warning is only emitted when a meaningful configuration difference is being ignored.

Suggested change
if attachedBinding.Spec.PreserveResourcesOnDeletion != nil || attachedBinding.Spec.ConflictResolution != "" {
if !equality.Semantic.DeepEqual(attachedBinding.Spec.PreserveResourcesOnDeletion, existBinding.Spec.PreserveResourcesOnDeletion) || attachedBinding.Spec.ConflictResolution != existBinding.Spec.ConflictResolution {

// notify that dependency path settings are ignored due to explicit policy ownership
d.EventRecorder.Eventf(existBinding, corev1.EventTypeWarning, events.EventReasonDependencyOverriddenByExplicitPolicy,
"Ignore dependency settings for %s/%s because it is governed by explicit policy.", existBinding.Namespace, existBinding.Name)
Copy link

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

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

The error message format uses string formatting with %s placeholders but the message text says "Ignore" which is grammatically awkward. Consider changing to "Ignoring dependency settings for %s/%s because it is governed by explicit policy."

Suggested change
"Ignore dependency settings for %s/%s because it is governed by explicit policy.", existBinding.Namespace, existBinding.Name)
"Ignoring dependency settings for %s/%s because it is governed by explicit policy.", existBinding.Namespace, existBinding.Name)

Copilot uses AI. Check for mistakes.

}
}

// Always merge relationship and labels
existBinding.Spec.RequiredBy = mergeBindingSnapshot(existBinding.Spec.RequiredBy, attachedBinding.Spec.RequiredBy)
existBinding.Labels = util.DedupeAndMergeLabels(existBinding.Labels, attachedBinding.Labels)
existBinding.Spec.Resource = attachedBinding.Spec.Resource
existBinding.Spec.PreserveResourcesOnDeletion = attachedBinding.Spec.PreserveResourcesOnDeletion

if err := d.Client.Update(context.TODO(), existBinding); err != nil {
klog.Errorf("Failed to update resourceBinding(%s): %v", bindingKey, err)
Expand Down
7 changes: 7 additions & 0 deletions pkg/events/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ const (
EventReasonSyncScheduleResultToDependenciesSucceed = "SyncScheduleResultToDependenciesSucceed"
// EventReasonSyncScheduleResultToDependenciesFailed indicates sync schedule result to attached bindings failed.
EventReasonSyncScheduleResultToDependenciesFailed = "SyncScheduleResultToDependenciesFailed"

// EventReasonDependencyPolicyConflict indicates multiple parents have divergent dependency policies.
EventReasonDependencyPolicyConflict = "DependencyPolicyConflict"
// EventReasonDependencyPolicyAggregated indicates aggregated dependency policies have been applied.
EventReasonDependencyPolicyAggregated = "DependencyPolicyAggregated"
// EventReasonDependencyOverriddenByExplicitPolicy indicates dependency path is overridden by explicit policy.
EventReasonDependencyOverriddenByExplicitPolicy = "DependencyOverriddenByExplicitPolicy"
)

// Define events for ResourceBinding, ClusterResourceBinding objects and their associated resources.
Expand Down
Loading