-
Notifications
You must be signed in to change notification settings - Fork 883
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.
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
feat: When a resource is created for the first time, it should also be considred as a modification. #4583
base: master
Are you sure you want to change the base?
feat: When a resource is created for the first time, it should also be considred as a modification. #4583
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @RainbowMango @chaosi-zju PTAL. |
What do you mean |
First time creating to k8s |
…e considered as a modification. Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
af5550a
to
0028f79
Compare
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My core idea is: Whether PP is Lazy or immediate, it should be immediate when the resources have not yet been bound by PP.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
-
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.
-
This PR wants to introduce "Lazy should only take effect after the PP is bound with resources."
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4583 +/- ##
==========================================
+ Coverage 51.69% 51.71% +0.01%
==========================================
Files 247 247
Lines 24418 24418
==========================================
+ Hits 12624 12627 +3
+ Misses 11106 11104 -2
+ Partials 688 687 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
After discussions in the community, it is currently considered appropriate to introduce a new type of Lazy. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
If the first creation is not considered as a modification, then resources like GatewayClass, IPPool can never be propagated to member clusters. Because once they are created, they will never be modified.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: