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

Jump to bottom

feat: When a resource is created for the first time, it should also be considred as a modification. #4583

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

feat: When a resource is created for the first time, it should also be considred as a modification. #4583

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
10 changes: 4 additions & 6 deletions pkg/detector/detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,17 +340,15 @@ func (d *ResourceDetector) OnUpdate(oldObj, newObj interface{}) {
return
}

if !eventfilter.SpecificationChanged(unstructuredOldObj, unstructuredNewObj) {
klog.V(4).Infof("Ignore update event of object (kind=%s, %s/%s) as specification no change", unstructuredOldObj.GetKind(), unstructuredOldObj.GetNamespace(), unstructuredOldObj.GetName())
return
}
chaunceyjiang marked this conversation as resolved.
Show resolved Hide resolved
newRuntimeObj, ok := newObj.(runtime.Object)
if !ok {
klog.Errorf("Failed to assert newObj as runtime.Object")
return
}

if !eventfilter.SpecificationChanged(unstructuredOldObj, unstructuredNewObj) {
klog.V(4).Infof("Ignore update event of object (kind=%s, %s/%s) as specification no change", unstructuredOldObj.GetKind(), unstructuredOldObj.GetNamespace(), unstructuredOldObj.GetName())
return
}

resourceChangeByKarmada := eventfilter.ResourceChangeByKarmada(unstructuredOldObj, unstructuredNewObj)

resourceItem := ResourceItem{
Expand Down
4 changes: 2 additions & 2 deletions pkg/detector/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (d *ResourceDetector) propagateResource(object *unstructured.Unstructured,
}
d.RemoveWaiting(objectKey)
metrics.ObserveFindMatchedPolicyLatency(start)
return d.ApplyPolicy(object, objectKey, resourceChangeByKarmada, propagationPolicy)
return d.ApplyPolicy(object, objectKey, false, propagationPolicy)
Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @RainbowMango @chaosi-zju

My core idea is: Whether PP is Lazy or immediate, it should be immediate when the resources have not yet been bound by PP.

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 believe Lazy should only take effect after the PP is bound with resources.

Copy link
Member

Choose a reason for hiding this comment

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

does this PR want to resolve the problem #4577 (comment) you memtioned here?

Copy link
Member Author

Choose a reason for hiding this comment

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

does this PR want to resolve the problem #4577 (comment) you memtioned here?

No, these are two different issues.

  1. The issue in Introduced a lazy activation preference to PropagationPolicy/ClusterPropagationPolicy #4577 (comment) is that using a single PP to select two resources with different effects.

  2. This PR wants to introduce "Lazy should only take effect after the PP is bound with resources."

Copy link
Member

@chaosi-zju chaosi-zju Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Assuming that a resource has been distributed according to PolicyA.

Then PolicyA deleted, or changed resourceSelector and no longer match to the resource.

The detecor will remove the policy label in resource immediately.

This will also triggle resoure updated.

In this case, resouce will look for a new policy (PolicyB), if PolicyB is Lazy, the binding should be unchanged.

The related pods of the reosource running just fine, we shouldn't break it just because policy change.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, resouce will look for a new policy, if this policy is Lazy, the binding should be unchanged.

Good question, your question introduces a new thought, whether the binding of PP and resources is Lazy or Immediate.

Copy link
Member

Choose a reason for hiding this comment

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

Lazy should only take effect after the PP is bound with resources.

This principle is what I want too, and I couldn't agree more.

However, there is an ambiguity in our understanding of buond. I understand bound to mean that a resource is bound when it is labelled with a policy, whereas you think that a resource is bound when it is refreshed with a binding following a policy.

Copy link
Member

Choose a reason for hiding this comment

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

your question introduces a new thought, whether the binding of PP and resources is Lazy or Immediate.

So the core of my proposal is The impact of Policy changes on the bondage relationship between resources and Policy is always immediate, but whether the resource needs to synchronize the latest placement to the binding immediately is determined by this field.

Pay attention to the bondage relationship between resources and Policy is always immediate, when a resource is labelled with a policy, just finish label, it finished the bound.

after bound, there's only ever one definitive policy for this resource, if this policy is lazy, don't change the resource's binding.

This would be the clearest behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pay attention to the bondage relationship between resources and Policy is always immediate, when a resource is labelled with a policy, just finish label, it finished the bound.

I got you.
However, most resources, once created, will not be changed.

As i said:

then resources like GatewayClass, IPPool can never be propagated to member clusters. Because once they are created, they will never be modified.

}

// 4. reaching here means there is no appropriate PropagationPolicy, attempt to match a ClusterPropagationPolicy.
Expand All @@ -86,7 +86,7 @@ func (d *ResourceDetector) propagateResource(object *unstructured.Unstructured,
}
d.RemoveWaiting(objectKey)
metrics.ObserveFindMatchedPolicyLatency(start)
return d.ApplyClusterPolicy(object, objectKey, resourceChangeByKarmada, clusterPolicy)
return d.ApplyClusterPolicy(object, objectKey, false, clusterPolicy)
}

if d.isWaiting(objectKey) {
Expand Down
Loading