Skip to content

fix: Fix calculating SelfHealBackOff delay when exceeding maximum (#20976)#20978

Merged
pasha-codefresh merged 2 commits intoargoproj:masterfrom
mrysavy:master
Dec 19, 2024
Merged

fix: Fix calculating SelfHealBackOff delay when exceeding maximum (#20976)#20978
pasha-codefresh merged 2 commits intoargoproj:masterfrom
mrysavy:master

Conversation

@mrysavy
Copy link
Copy Markdown
Contributor

@mrysavy mrysavy commented Nov 27, 2024

Fixes [ISSUE #20976]

This PR fixes calculating of SelfHealBackOff delay when the delay exceeds maximum

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@bunnyshell
Copy link
Copy Markdown

bunnyshell bot commented Nov 27, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@mrysavy mrysavy force-pushed the master branch 2 times, most recently from 916f553 to e249bae Compare November 27, 2024 16:56
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.99%. Comparing base (02d6866) to head (ebbf5c7).
Report is 513 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20978      +/-   ##
==========================================
- Coverage   55.03%   54.99%   -0.04%     
==========================================
  Files         324      324              
  Lines       55466    55467       +1     
==========================================
- Hits        30526    30506      -20     
- Misses      22330    22343      +13     
- Partials     2610     2618       +8     

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

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
@mrysavy mrysavy marked this pull request as ready for review November 29, 2024 12:06
@mrysavy mrysavy requested a review from a team as a code owner November 29, 2024 12:06
@michalrysavy-ext95730
Copy link
Copy Markdown
Contributor

Hi, is anything missing in this PR before it could be accepted?

backOff.Steps = int(app.Status.OperationState.Operation.Sync.SelfHealAttemptsCount)
var delay time.Duration
for backOff.Steps > 0 {
steps := backOff.Steps
Copy link
Copy Markdown
Member

@pasha-codefresh pasha-codefresh Dec 19, 2024

Choose a reason for hiding this comment

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

Yeah, it happens because

next = time.Duration(float64(duration) * factor)
		if cap > 0 && next > cap {
			next = cap
			steps = 0
		}

Inside backoff library, it is marking steps as 0 and setup next with correct value, and just because of backOff.Steps > 0 we never reach last iteration.

Just leaving as explanation for future

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You still potentially can pass max int value as backoff , but it will not have any performance impact, because here exit in the function if steps are 0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hovewer we can change the loop a bit, but i think it is overcomplicate it without significant benefit

for i := 0; i < steps; i++ {
	delay = backOff.Step()
	
	if i != steps - 1 && backOff.Steps == 0 {
		delay = backOff.Duration
		break
	}
	
}

@pasha-codefresh
Copy link
Copy Markdown
Member

LGTM, thank you

@pasha-codefresh pasha-codefresh merged commit 8841b0d into argoproj:master Dec 19, 2024
dudo pushed a commit to dudo/argo-cd that referenced this pull request Jan 18, 2025
…goproj#20976) (argoproj#20978)

* test: fix TestSelfHealExponentialBackoff to test exceeding Backoff.Cap

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

* fix: fix calculating SelfHealBackOff delay when exceeding maximum

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

---------

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Co-authored-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Signed-off-by: Brett C. Dudo <brett@dudo.io>
revitalbarletz pushed a commit to revitalbarletz/argo-cd that referenced this pull request Jan 20, 2025
…goproj#20976) (argoproj#20978)

* test: fix TestSelfHealExponentialBackoff to test exceeding Backoff.Cap

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

* fix: fix calculating SelfHealBackOff delay when exceeding maximum

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

---------

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Co-authored-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
vasilegroza pushed a commit to vasilegroza/argo-cd that referenced this pull request Feb 27, 2025
…goproj#20976) (argoproj#20978)

* test: fix TestSelfHealExponentialBackoff to test exceeding Backoff.Cap

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

* fix: fix calculating SelfHealBackOff delay when exceeding maximum

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

---------

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Co-authored-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Aaron-9900 pushed a commit to Aaron-9900/argo-cd that referenced this pull request Apr 7, 2025
…goproj#20976) (argoproj#20978)

* test: fix TestSelfHealExponentialBackoff to test exceeding Backoff.Cap

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

* fix: fix calculating SelfHealBackOff delay when exceeding maximum

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

---------

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Co-authored-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Aaron-9900 pushed a commit to Aaron-9900/argo-cd that referenced this pull request Apr 7, 2025
…goproj#20976) (argoproj#20978)

* test: fix TestSelfHealExponentialBackoff to test exceeding Backoff.Cap

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

* fix: fix calculating SelfHealBackOff delay when exceeding maximum

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>

Signed-off-by: Michal Ryšavý <mrysavy@users.noreply.github.com>

---------

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Co-authored-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
rumstead pushed a commit that referenced this pull request Apr 7, 2025
…22095, #20978) (#22583)

Signed-off-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Co-authored-by: Michal Ryšavý <mrysavy@users.noreply.github.com>
Co-authored-by: Michal Ryšavý <michal.rysavy@ext.csas.cz>
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
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.

3 participants