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 poll jittering in Object controller #274

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

aerfio
Copy link
Contributor

@aerfio aerfio commented Jul 23, 2024

Description of your changes

This PR fixes Object controller which sometimes returns a negative poll interval if the reconciled Object instance is not ready, which is then ignored by controller-runtime internals.

In my case this has been observed in a situation in which the k8s object from spec.forProvider.manifest takes a while to get ready, but under 10m which is the default pollInterval.

The logic which calculates the pollInterval does not take into account that the base pollInterval has been updated to 30 seconds if the reconciled resource is not ready. It took the base value of pollJitter which is 1 minute, and calculated next poll interval using this formula:

return pollInterval + time.Duration((rand.Float64()-0.5)*2*float64(pollJitter))

I guess that most users run provider-kubernetes without adjusting default poll interval, so if my math is right if rand.Float64 returns anything less then 0.25 (its documentations states: "// Float64 returns, as a float64, a pseudo-random number in the half-open interval [0.0,1.0)"), the returned value is less then 0. 25% chance to run into this bug. And if I understand the c-r source code right, it ignores negative RequeueAfter.

The solution is to recalculate the pollInterval taking into account the pollJitterPercentage - this means that with default value of poll jitter precentage the poll interval would be in half-open interval [27.0, 33.0) (seconds).

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Signed-off-by: Mateusz Puczyński <[email protected]>
@aerfio aerfio marked this pull request as ready for review July 23, 2024 09:55
Copy link
Collaborator

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Great catch 🙌 and awesome PR description 🙇‍♂️ Thanks a lot!

@turkenh turkenh merged commit 80eeec3 into crossplane-contrib:main Jul 26, 2024
7 checks passed
Copy link

Successfully created backport PR #275 for release-0.14.

@aerfio
Copy link
Contributor Author

aerfio commented Jul 29, 2024

@turkenh Hi, can I count on a patch release with this fix? 🙏🏻

@turkenh
Copy link
Collaborator

turkenh commented Jul 29, 2024

@aerfio
Copy link
Contributor Author

aerfio commented Jul 29, 2024

@turkenh Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants