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

feat(analysis): Add ConsecutiveSuccessLimit feature to Analysis #3970

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

y-rabie
Copy link
Contributor

@y-rabie y-rabie commented Dec 1, 2024

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

This regards the enhancement proposal, see there for a brief description first.

First, regarding the fields and their possible values:

  • The value used for FailureLimit to disable it is -1. The default is left to be 0 as is currently the case. This preserves backward compatibility. The rationale for adding a value to disable FailureLimit is the convenience for a user who wants to only use ConsecutiveSuccessLimit (waiting on a condition to hold/event to happen), instead of forcing them to type an arbitrarily big value for the FailureLimit.

  • The default value for ConsecutiveSuccessLimit is 0, which makes it effectively disabled by default. This is explicitly an opt-in feature to use.

  • Both of them can be used side-by-side (ConsecutiveSuccessLimit > 0 and FailureLimit >= 0). Notes regarding behavior follow here shortly after.

  • They cannot be both disabled. Validation throws an error for that.


Second, a lot of the following resembles the way metric measurements themselves are evaluated in utils/evaluate/evaluate.go, with regards to FailureCondition and SuccessCondition. That is:

  • When FailureCondition is satisfied, it fails (even if SuccessCondition is specified separately and is satisifed).
  • When neither is satisfied, the measurement is inconclusive.

All possible scenarios should be obvious from reading the added unit tests, but the particular ones I think deserve mention are:

  • On terminating/cancelling analyses prematurely, that is, either any indefinite analysis, or limited analysis with count not yet reached

    They are always terminated with AnalysisPhaseSuccessful, UNLESS, it happens that FailureLimit is enabled and violated, then they terminate with AnalysisPhaseFailed.

    This is already the case, but my emphasis here is that ConsecutiveSuccessLimit changes nothing from that. Even if it was not yet reached, the analysis is terminated successfully.

  • For limited analysis (with count specified)

    When count is reached, the note above regarding similaritly to metric measurement evaluation comes into play.
    If both FailureLimit and ConsecutiveSuccessLimit are enabled, and

    • FailureLimit is violated and ConsecutiveSuccessLimit is not satisfied => AnalysisPhaseFailed
    • FailureLimit is violated and ConsecutiveSuccessLimit is satisfied => AnalysisPhaseFailed
    • FailureLimit is not violated and ConsecutiveSuccessLimit is satisfied => AnalysisPhaseSuccessful
    • FailureLimit is not violated and ConsecutiveSuccessLimit is not satisfied => AnalysisPhaseInconclusive

I've written unit tests for the added logic, and ran and tested the controller myself. The minor changes in the UI dashboard have not been tested (building and accessing it always cause the tab to load indefinitely for some reason, and I doubt that is related to my changes).

Copy link

codecov bot commented Dec 1, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.66%. Comparing base (5f59344) to head (defa236).

Files with missing lines Patch % Lines
analysis/analysis.go 86.11% 4 Missing and 1 partial ⚠️
utils/analysis/factory.go 85.18% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3970      +/-   ##
==========================================
- Coverage   82.69%   82.66%   -0.04%     
==========================================
  Files         163      163              
  Lines       22895    22944      +49     
==========================================
+ Hits        18934    18967      +33     
- Misses       3087     3100      +13     
- Partials      874      877       +3     

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

Copy link
Contributor

github-actions bot commented Dec 1, 2024

Published E2E Test Results

  4 files    4 suites   3h 14m 16s ⏱️
113 tests 106 ✅  7 💤 0 ❌
452 runs  424 ✅ 28 💤 0 ❌

Results for commit 979fa81.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Dec 1, 2024

Published Unit Test Results

2 293 tests   2 293 ✅  2m 59s ⏱️
  128 suites      0 💤
    1 files        0 ❌

Results for commit 979fa81.

♻️ This comment has been updated with latest results.

@kostis-codefresh kostis-codefresh added the analysis Related to Analysis CRD label Dec 4, 2024
@zachaller zachaller added this to the v1.8 milestone Dec 4, 2024
Signed-off-by: Youssef Rabie <youssef.rabie@procore.com>
@zachaller zachaller force-pushed the add-successlimit-analysis branch from defa236 to 979fa81 Compare December 5, 2024 16:01
Copy link

sonarqubecloud bot commented Dec 5, 2024

@zachaller zachaller merged commit 1c6a7ff into argoproj:master Dec 5, 2024
24 checks passed
@y-rabie y-rabie deleted the add-successlimit-analysis branch December 6, 2024 11:48
@zachaller
Copy link
Collaborator

zachaller commented Dec 9, 2024

@y-rabie I don't know how I missed this could you add some docs for the feature with a lot of the great info you have mentioned in this PR and proposal descriptions

@zachaller
Copy link
Collaborator

@y-rabie Probably something on this section of the docs. https://argo-rollouts.readthedocs.io/en/stable/features/analysis/

@y-rabie
Copy link
Contributor Author

y-rabie commented Dec 9, 2024

@zachaller Sure, will do and open a PR with them

Rizwana777 pushed a commit to Rizwana777/argo-rollouts that referenced this pull request Dec 12, 2024
…proj#3970)

Signed-off-by: Youssef Rabie <youssef.rabie@procore.com>
meeech pushed a commit to CircleCI-Public/argo-rollouts that referenced this pull request Feb 10, 2025
…proj#3970)

Signed-off-by: Youssef Rabie <youssef.rabie@procore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Related to Analysis CRD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants