bugfix: resource binding is not created when creating resources and propagation policies at the same time #1199

dddddai · 2021-12-31T07:59:38Z

What type of PR is this?
/kind bug
/kind flake

What this PR does / why we need it:
The root cause is as the follow process:

We must add the resource to waiting list before looking up the matched propagation policy, so that the matched resource can always be found when reconciling propagation policies

Which issue(s) this PR fixes:
Fixes #1195

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

pkg/detector/detector.go

XiShanYongYe-Chang · 2022-01-04T01:41:05Z

pkg/detector/detector.go

@@ -955,7 +958,6 @@ func (d *ResourceDetector) HandlePropagationPolicyCreation(policy *policyv1alpha
 	}

 	for _, key := range matchedKeys {
-		d.RemoveWaiting(key)


I didn't understand why this line was deleted.

Because the waiting obj would be removed when reconciling, as above shows
Is there any good reason to keep it here?

Reconciling will add the key again, keeping it here feels fine and easier to understand.

Reconciling will add the key again, keeping it here feels fine and easier to understand.

karmada/pkg/detector/detector.go

Line 74 in 7224234

waitingObjects map[keys.ClusterWideKey]struct{}

The waitingObjects is a map, so it won't matter if an obj is added multiple times

If you mean it's for better understanding, I will revert it though

XiShanYongYe-Chang · 2022-01-04T02:59:34Z

The current processing logic can cover the scenario.

dddddai · 2022-01-04T03:33:14Z

@XiShanYongYe-Chang Sorry... I didn't get the meaning of the picture

XiShanYongYe-Chang · 2022-01-04T03:37:31Z

@XiShanYongYe-Chang Sorry... I didn't get the meaning of the picture

I didn't draw it well. I'm just trying to explain that the current way can overwrite the previous error.

XiShanYongYe-Chang · 2022-01-04T08:04:38Z

/lgtm

/cc @RainbowMango

RainbowMango · 2022-01-05T02:00:13Z

Good job @dddddai .
This patch definitely can solve the corner case. The side-effect I can tell is the resource template might be got in and out the waitting list many times.

Can you run a test to observe the logs:

karmada/pkg/detector/detector.go

Line 707 in 8b3aefd

    
           klog.V(1).Infof("Add object(%s) to waiting list, length of list is: %d", objectKey.String(), len(d.waitingObjects))

Let's see how many times a resource template would be put in waitting list in a propagation process.

dddddai · 2022-01-05T02:38:32Z

Thanks @RainbowMango, here's the log
If creating resource before the propagation policy:

I0105 10:29:38.611012       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:29:46.133486       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:29:46.177437       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:29:46.208280       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:29:46.618257       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:29:46.633569       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:29:46.733210       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:29:48.428089       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160

The resource was added to waiting list 8 times

If creating the propagation policy before resource:

I0105 10:32:25.143445       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:32:25.165382       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:32:25.202162       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:32:25.576123       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:32:25.816949       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:32:25.930986       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160
I0105 10:32:26.822290       1 detector.go:710] Add object(apps/v1, kind=Deployment, default/nginx) to waiting list, length of list is: 160

The resource was added to waiting list 7 times

The side-effect I can tell is the resource template might be got in and out the waitting list many times.

Yes, but this is the simplest way I could figure out to fix the bug, I'd appreciate it if there is any better idea

RainbowMango · 2022-01-05T06:26:03Z

I've no idea yet. Since it's the corner case, so let's do it slowly and correctly.

@iawia002 any comments?

iawia002 · 2022-01-05T07:04:08Z

I've no idea yet. Since it's the corner case, so let's do it slowly and correctly.

@iawia002 any comments?

Actually I have no better idea, maybe we can make a judgment before adding keys to the waiting list:

+	if _, ok := d.waitingObjects[objectKey]; ok {
+		return
+	}
	d.waitingObjects[objectKey] = struct{}{}
	klog.V(1).Infof("Add object(%s) to waiting list, length of list is: %d", objectKey.String(), len(d.waitingObjects))

dddddai · 2022-01-05T09:36:01Z

@iawia002 , Thanks for your suggestion, but in most cases it's unlikely for objects to stay in waiting list before adding, and the judgment can make AddWaiting slower

XiShanYongYe-Chang

What's going on with this pull request?

RainbowMango · 2022-01-10T11:49:33Z

Looking forward to a better solution.
/priority important-soon

dddddai · 2022-01-13T10:26:25Z

To be honest I don't see any better fixes, maybe we can merge it for quick fix(when we are gonna release 1.1) and update this once there's a better solution.

If the logs are too noisy , we may change them from info to debug

RainbowMango · 2022-01-13T11:35:43Z

I'm thinking if we can merge the two queues into one. But haven't time to look at it yet.

It's not an urgent bug, so we can hold for a while. I think it will be included in v1.1.

RainbowMango · 2022-01-20T09:03:22Z

This problem is simply that two routines(resource template reconciler and PP/CPP reconciler) depend on the same mutex(waiting list).

So, how about merging PP/CPP reconciler to resource template reconciler? Thus, the waiting list will not be shared.
I mean the following reconcile could be removed.

karmada/pkg/detector/detector.go

Lines 57 to 65 in e3c8d39

    
           // policyReconcileWorker maintains a rate limited queue which used to store PropagationPolicy's key and 
        
           // a reconcile function to consume the items in queue. 
        
           policyReconcileWorker   util.AsyncWorker 
        
           propagationPolicyLister cache.GenericLister 
        
           // clusterPolicyReconcileWorker maintains a rate limited queue which used to store ClusterPropagationPolicy's key and 
        
           // a reconcile function to consume the items in queue. 
        
           clusterPolicyReconcileWorker   util.AsyncWorker 
        
           clusterPropagationPolicyLister cache.GenericLister

When reconciling there will be two routing path:

For PP/CPP, routing to ReconcilePropagationPolicy or ReconcileClusterPropagationPolicy.
For resource template, routing to Reconcile

There will be a little bit complex when dealing with the EventFilter.

dddddai · 2022-01-21T00:58:48Z

So, how about merging PP/CPP reconciler to resource template reconciler? Thus, the waiting list will not be shared.

Do you mean reconciling both PP/CPPs and resource templates in one goroutine?

XiShanYongYe-Chang · 2022-01-21T08:58:37Z

Do you mean reconciling both PP/CPPs and resource templates in one goroutine?

@dddddai I think it should be. Can you try to modify it? Today has two cases failed on the master branch for this reason.

RainbowMango · 2022-01-21T09:00:04Z

Do you mean reconciling both PP/CPPs and resource templates in one goroutine?

Yes, that's the idea.

pkg/detector/detector.go

RainbowMango

Pretty good now.

RainbowMango · 2022-01-22T06:46:08Z

pkg/detector/detector.go

@@ -206,6 +191,19 @@ func (d *ResourceDetector) Reconcile(key util.QueueKey) error {
 	}
 	klog.Infof("Reconciling object: %s", clusterWideKey)


Can we move the log down? I don't want the ignored resources present in the logs by default.

RainbowMango · 2022-01-22T06:54:43Z

pkg/detector/detector.go

+	if clusterWideKey.Group == policyv1alpha1.GroupName {
+		switch clusterWideKey.Kind {
+		case "PropagationPolicy":
+			return d.ReconcilePropagationPolicy(key)
+		case "ClusterPropagationPolicy":
+			return d.ReconcileClusterPropagationPolicy(key)
+		}
+	}
+
+	if !d.EventFilter(clusterWideKey) {
+		return nil
+	}
+


The if clusterWideKey.Group == policyv1alpha1.GroupName block better to moved into the if !d.EventFilter(clusterWideKey), like:

if !d.EventFilter(clusterWideKey) { if clusterWideKey.Group == policyv1alpha1.GroupName { switch clusterWideKey.Kind { case "PropagationPolicy": return d.ReconcilePropagationPolicy(key) case "ClusterPropagationPolicy": return d.ReconcileClusterPropagationPolicy(key) } } klog.V(4).Infof("Ignored object(%s) from reconciling.", clusterWideKey.String()) return nil } klog.Infof("Reconciling object: %s", clusterWideKey)

RainbowMango · 2022-01-22T06:55:54Z

pkg/detector/detector.go

-		return false
-	}
-
+func (d *ResourceDetector) EventFilter(clusterWideKey keys.ClusterWideKey) bool {


I guess the EventFilter should be renamed now, as it not handling the event anymore, but checks if the specified object should be prevented from propagating. So, how about SkippedFromPropagating?

Remember to update the comments and take care of the function body.

RainbowMango · 2022-01-24T01:59:11Z

pkg/detector/detector.go

-		return false
-	}
-
+func (d *ResourceDetector) SkippedFromPropagating(clusterWideKey keys.ClusterWideKey) bool {


After renaming the function, the implementation should be updated as well, when a resource is skipped, it should return true now.

Fixed, thanks for elaborating!

reconcile PP/CPPs and resource templates in one goroutine Signed-off-by: dddddai <[email protected]>

RainbowMango

/lgtm
/approve

Now we just started one worker, this risk still exists if there is more than one worker.

No other better solution yet, and this issue happens several times in our CI, merging this patch first.

karmada-bot · 2022-01-24T07:46:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dddddai · 2022-01-24T07:59:09Z

this risk still exists if there is more than one worker

That's exactly what I'm worried about, so honestly I'd lean towards the original solution

RainbowMango · 2022-01-24T08:07:18Z

so honestly I'd lean towards the original solution

...

The original solution(throw into waiting list) does not 100% solve the problem but reduces the probability significantly.

dddddai · 2022-01-24T08:14:15Z

Why? Could you please give an example?

Anyway I'm fine with both solutions

pigletfly · 2022-01-27T10:02:06Z

so honestly I'd lean towards the original solution

...

The original solution(throw into waiting list) does not 100% solve the problem but reduces the probability significantly.

we should start a seperate goroutine to consume waiting, if resource template and pp are created at the same time and miss the match(resource template is created and has no more event),then goroutine keeps find the match.

RainbowMango · 2022-01-28T01:33:06Z

we should start a seperate goroutine to consume waiting, if resource template and pp are created at the same time and miss the match(resource template is created and has no more event),then goroutine keeps find the match.

I'm also thinking of this approach.

karmada-bot added kind/bug Categorizes issue or PR as related to a bug. kind/flake Categorizes issue or PR as related to a flaky test. labels Dec 31, 2021

karmada-bot requested review from kevin-wangzefeng and RainbowMango December 31, 2021 07:59

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 31, 2021

dddddai changed the title ~~bugfix: resource binding is not created~~ bugfix: resource binding is not created when creating resources and propagation policies at the same time Dec 31, 2021

dddddai force-pushed the waiting branch from cddbe29 to 545dc57 Compare January 2, 2022 06:05

XiShanYongYe-Chang reviewed Jan 4, 2022

View reviewed changes

dddddai force-pushed the waiting branch from 545dc57 to cddbe29 Compare January 4, 2022 02:48

karmada-bot assigned XiShanYongYe-Chang Jan 4, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 4, 2022

XiShanYongYe-Chang reviewed Jan 10, 2022

View reviewed changes

karmada-bot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 10, 2022

RainbowMango added this to the v1.1 milestone Jan 13, 2022

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 21, 2022

dddddai force-pushed the waiting branch 2 times, most recently from e718e2c to 9f83682 Compare January 22, 2022 01:08

RainbowMango reviewed Jan 22, 2022

View reviewed changes

pkg/detector/detector.go Outdated Show resolved Hide resolved

pkg/detector/detector.go Outdated Show resolved Hide resolved

dddddai force-pushed the waiting branch 2 times, most recently from 19694c3 to c11fb12 Compare January 22, 2022 03:00

RainbowMango reviewed Jan 22, 2022

View reviewed changes

dddddai force-pushed the waiting branch from c11fb12 to 81b7045 Compare January 22, 2022 14:22

RainbowMango reviewed Jan 24, 2022

View reviewed changes

dddddai force-pushed the waiting branch 3 times, most recently from 76a23ce to c770cc1 Compare January 24, 2022 02:13

bugfix: resource binding is not created

c770cc1

reconcile PP/CPPs and resource templates in one goroutine Signed-off-by: dddddai <[email protected]>

RainbowMango approved these changes Jan 24, 2022

View reviewed changes

karmada-bot assigned RainbowMango Jan 24, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2022

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2022

karmada-bot merged commit e0e7a60 into karmada-io:master Jan 24, 2022

dddddai mentioned this pull request Jan 26, 2022

introduce concurrent flags to controllers #1321

Merged

RainbowMango mentioned this pull request Jan 28, 2022

[lfx-mentorship] Enhancement for controllers scalebility #1331

Closed

dddddai mentioned this pull request Feb 16, 2022

Rework "bugfix: resource binding not created occasionally" #1368

Merged

dddddai deleted the waiting branch March 1, 2022 00:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: resource binding is not created when creating resources and propagation policies at the same time #1199

bugfix: resource binding is not created when creating resources and propagation policies at the same time #1199

dddddai commented Dec 31, 2021

XiShanYongYe-Chang Jan 4, 2022

dddddai Jan 4, 2022 •

edited

Loading

XiShanYongYe-Chang Jan 4, 2022

dddddai Jan 4, 2022 •

edited

Loading

XiShanYongYe-Chang commented Jan 4, 2022 •

edited

Loading

dddddai commented Jan 4, 2022

XiShanYongYe-Chang commented Jan 4, 2022

XiShanYongYe-Chang commented Jan 4, 2022

RainbowMango commented Jan 5, 2022

dddddai commented Jan 5, 2022 •

edited

Loading

RainbowMango commented Jan 5, 2022

iawia002 commented Jan 5, 2022

dddddai commented Jan 5, 2022

XiShanYongYe-Chang left a comment

RainbowMango commented Jan 10, 2022

dddddai commented Jan 13, 2022

RainbowMango commented Jan 13, 2022

RainbowMango commented Jan 20, 2022

dddddai commented Jan 21, 2022

XiShanYongYe-Chang commented Jan 21, 2022

RainbowMango commented Jan 21, 2022

RainbowMango left a comment

RainbowMango Jan 22, 2022

RainbowMango Jan 22, 2022

RainbowMango Jan 22, 2022

RainbowMango Jan 24, 2022

dddddai Jan 24, 2022

RainbowMango left a comment

karmada-bot commented Jan 24, 2022

dddddai commented Jan 24, 2022 •

edited

Loading

RainbowMango commented Jan 24, 2022

dddddai commented Jan 24, 2022

pigletfly commented Jan 27, 2022

RainbowMango commented Jan 28, 2022

		@@ -206,6 +191,19 @@ func (d *ResourceDetector) Reconcile(key util.QueueKey) error {
		}
		klog.Infof("Reconciling object: %s", clusterWideKey)

bugfix: resource binding is not created when creating resources and propagation policies at the same time #1199

bugfix: resource binding is not created when creating resources and propagation policies at the same time #1199

Conversation

dddddai commented Dec 31, 2021

XiShanYongYe-Chang Jan 4, 2022

Choose a reason for hiding this comment

dddddai Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

XiShanYongYe-Chang Jan 4, 2022

Choose a reason for hiding this comment

dddddai Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Jan 4, 2022 • edited Loading

dddddai commented Jan 4, 2022

XiShanYongYe-Chang commented Jan 4, 2022

XiShanYongYe-Chang commented Jan 4, 2022

RainbowMango commented Jan 5, 2022

dddddai commented Jan 5, 2022 • edited Loading

RainbowMango commented Jan 5, 2022

iawia002 commented Jan 5, 2022

dddddai commented Jan 5, 2022

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango commented Jan 10, 2022

dddddai commented Jan 13, 2022

RainbowMango commented Jan 13, 2022

RainbowMango commented Jan 20, 2022

dddddai commented Jan 21, 2022

XiShanYongYe-Chang commented Jan 21, 2022

RainbowMango commented Jan 21, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango Jan 22, 2022

Choose a reason for hiding this comment

RainbowMango Jan 22, 2022

Choose a reason for hiding this comment

RainbowMango Jan 22, 2022

Choose a reason for hiding this comment

RainbowMango Jan 24, 2022

Choose a reason for hiding this comment

dddddai Jan 24, 2022

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Jan 24, 2022

dddddai commented Jan 24, 2022 • edited Loading

RainbowMango commented Jan 24, 2022

dddddai commented Jan 24, 2022

pigletfly commented Jan 27, 2022

RainbowMango commented Jan 28, 2022

dddddai Jan 4, 2022 •

edited

Loading

dddddai Jan 4, 2022 •

edited

Loading

XiShanYongYe-Chang commented Jan 4, 2022 •

edited

Loading

dddddai commented Jan 5, 2022 •

edited

Loading

dddddai commented Jan 24, 2022 •

edited

Loading