-
Notifications
You must be signed in to change notification settings - Fork 867
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
fix: change defaultModelSorting as local variable #4176
Conversation
Signed-off-by: jwcesign <jwcesign@gmail.com>
@jwcesign: GitHub didn't allow me to request PR reviews from the following users: halfrost. Note that only karmada-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
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.
LGTM
Have you found the root cause? |
@chaunceyjiang The root case is the same as this(#3854 (comment)). |
@@ -14,15 +13,11 @@ import ( | |||
clusterapis "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1" | |||
) | |||
|
|||
var ( | |||
mu sync.Mutex | |||
defaultModelSorting []clusterapis.ResourceName |
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.
The resource models might be different from each other, for example, cluster 1's model could be:
resourceModels:
- grade: 0
ranges:
- max: "1"
min: "0"
name: cpu # Note that only declared cpu
and cluster 2's model could be
resourceModels:
- grade: 0
ranges:
- max: 4Gi
min: "0"
name: memory # Note that only declared memory
The global variable defaultModelSorting
actually shared by different clusters and might be overwritten during init the model at https://github.com/karmada-io/karmada/blob/3267155766f69083d211c4bc804e8bda4b18fd4d/pkg/controllers/status/cluster_status_controller.go#L595C35-L595C46.
@halfrost Please correct me if this is not the case.
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.
Yes, the resource model should be different for each cluster. But the original requirement seems to be that this modeling is to build a large resource model. Karmada manages the resource models of all clusters in a unified way. For this reason, it is taken out as a global variable.
tmpCrn := element.Value.(ClusterResourceNode) | ||
safeChangeNum(&tmpCrn.quantity, crn.quantity) |
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.
@halfrost Can you remind me why we need the lock when adding the resource quantity to the element
?
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.
+1 confused too
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.
This function AddToResourceSummary
will be called concurrently. If no locking is performed, the eventual consistency of num will be incorrect.
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.
@jwcesign I think the point where you're confused comes from whether it should be made a local variable. It is a global variable at first, so it needs to be locked 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.
Hi, @halfrost Thanks for your clarification!
So, this PR is reasonable and correct? Or there are some other things I may miss?
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.
Hi @jwcesign ,
Your changes are correct; there are no issues. With your modifications, each cluster now has its own modeling information.
There are no global variables left. In my previous PR, I didn't turn defaultModelSorting
into a member variable because I was under the impression that we needed a broader global modeling context.
So this PR is prefect now.
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.
Thank you @halfrost for the clarification.
I believe it reasonable for each cluster to have its resource models.
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.
OK, this pr makes sense. Thanks for @jwcesign work. 👍🏻
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.
/lgtm
/approve
I think this PR resolved a functionality issue and a potential panic.
@halfrost your comments and clarification are still welcome!
[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:
Approvers can indicate their approval by writing |
@RainbowMango These changes are both correct and reasonable. I've reviewed it carefully. 👍🏻 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Please check: #3854
Which issue(s) this PR fixes:
Part of #3854
Special notes for your reviewer:
none
Does this PR introduce a user-facing change?: