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

Fix Negative Requeue Duration in getStatusCheckDelay #1841

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

OdedViner
Copy link
Contributor

@OdedViner OdedViner commented Feb 18, 2025

This PR fixes an issue in the getStatusCheckDelay function where a negative requeue duration could be returned if the scheduled time had already passed. The fix ensures that if the computed remaining time is negative, the function returns 0 to trigger an immediate requeue instead of a negative duration.

Changes:

  • Added a check to prevent negative requeue durations.

  • If the computed duration is negative, it now returns 0.

This improves the stability of the reconciliation loop and prevents unexpected scheduling behavior.

Fix: #1838

Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

The fix looks good but the tests is 5 times more complicated than the code it tests and will be hard to maintain.

How controller runtime handle negative requeue time? maybe it already clip the value to 0 and does the right thing? If it does, we don't need to complicate the code.

Copy link
Member

Choose a reason for hiding this comment

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

This file is in the controller_test package, which is good for black box testing. For testing a private function we can use a test using the controllers package.

@nirs nirs requested a review from BenamarMk February 18, 2025 16:21
@nirs
Copy link
Member

nirs commented Feb 19, 2025

The fix is trivial but testing is it hard and will take time to get right. I would separate the test to another PR and merge the trivial fix now.

@OdedViner OdedViner force-pushed the fix_negative_requeue branch 6 times, most recently from 087ec23 to 97434e6 Compare February 20, 2025 11:55
@nirs
Copy link
Member

nirs commented Feb 20, 2025

@OdedViner please squash the commits

@OdedViner OdedViner force-pushed the fix_negative_requeue branch from 97434e6 to 0a2f658 Compare February 20, 2025 12:56
@OdedViner
Copy link
Contributor Author

@OdedViner please squash the commits

done

return 0
}

return remaining
Copy link
Member

Choose a reason for hiding this comment

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

We can simplify this to:

return max(0, remaining)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@OdedViner OdedViner force-pushed the fix_negative_requeue branch 2 times, most recently from b544346 to 81fc221 Compare February 20, 2025 15:14
@OdedViner OdedViner force-pushed the fix_negative_requeue branch from 81fc221 to 8939b10 Compare February 20, 2025 15:21
@BenamarMk BenamarMk merged commit a74b631 into RamenDR:main Feb 20, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negative Requeue Duration Returned by getStatusCheckDelay in DRPlacementControl Reconciler
3 participants