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.

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

Fixed occasional errors with clusteraffinities #3518

Conversation

XiShanYongYe-Chang
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang commented May 11, 2023

What type of PR is this?

/kind flake

What this PR does / why we need it:

In the karmada-scheduler, the patch is used to update the rb status. Before the update, deep copy is performed on the rb. As a result, the subsequent rb objects are still the old values, and patch errors may occur.

So I modified this to keep the rb object up-to-date after the update to avoid affecting subsequent patch operations.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/flake Categorizes issue or PR as related to a flaky test. label May 11, 2023
@XiShanYongYe-Chang XiShanYongYe-Chang marked this pull request as draft May 11, 2023 02:52
@karmada-bot karmada-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2023

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (d43f22f) 51.78% compared to head (7825b04) 51.79%.

Files Patch % Lines
pkg/scheduler/scheduler.go 30.00% 28 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3518   +/-   ##
=======================================
  Coverage   51.78%   51.79%           
=======================================
  Files         243      243           
  Lines       24112    24120    +8     
=======================================
+ Hits        12486    12492    +6     
- Misses      10945    10948    +3     
+ Partials      681      680    -1     
Flag Coverage Δ
unittests 51.79% <30.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the flake-test-with-clusteraffinities branch from ada4aac to 234eb45 Compare May 11, 2023 06:33
@karmada-bot karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 11, 2023
@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the flake-test-with-clusteraffinities branch 15 times, most recently from 1c7ea95 to 414f1a4 Compare May 18, 2023 02:22
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 18, 2023
@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the flake-test-with-clusteraffinities branch 2 times, most recently from 421cb78 to a0f3f81 Compare May 18, 2023 11:25
@karmada-bot karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 4, 2023
@XiShanYongYe-Chang XiShanYongYe-Chang force-pushed the flake-test-with-clusteraffinities branch 2 times, most recently from 5cf1d66 to 08cac3a Compare July 6, 2023 08:53
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2023
@XiShanYongYe-Chang
Copy link
Member Author

/cc @chaunceyjiang @RainbowMango

@chaunceyjiang
Copy link
Member

Have you already found the root cause of this issue? /cc @XiShanYongYe-Chang

@XiShanYongYe-Chang
Copy link
Member Author

Have you already found the root cause of this issue?

I caught an error log earlier, but I seem to have lost it. When checking logs, it is found that the scheduler updates the status of the rb to an incorrect old status during subsequent update. As a result, the eventhandler does not meet the conditions during check.

@RainbowMango
Copy link
Member

RainbowMango commented Jul 10, 2023

@RainbowMango
Copy link
Member

@XiShanYongYe-Chang
Copy link
Member Author

This PR is ready, can you help review it?
/cc @RainbowMango @chaunceyjiang

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/assign

pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved
pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved
pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from rainbowmango after the PR has been reviewed.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@XiShanYongYe-Chang
Copy link
Member Author

/hold
Let me think about whether it is possible to consolidate the update operations as much as possible, in order to reduce conflicts and impacts caused by multiple updates in different places.

@karmada-bot karmada-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2023
@XiShanYongYe-Chang
Copy link
Member Author

I think using the patch method in #4094 should be able to solve the current issue with patch errors. What do you think? @chaunceyjiang

@XiShanYongYe-Chang
Copy link
Member Author

It seems that the issue has not occurs for a long time.
/close

@karmada-bot
Copy link
Collaborator

@XiShanYongYe-Chang: Closed this PR.

In response to this:

It seems that the issue has not occurs for a long time.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/flake Categorizes issue or PR as related to a flaky test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants