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.

Sign up for GitHub

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

Jump to bottom

feat: configurable and more aggressive cleanup of old AnalysisRuns and Experiments #1342

Merged
merged 25 commits into from
Aug 5, 2021
Merged

feat: configurable and more aggressive cleanup of old AnalysisRuns and Experiments #1342

merged 25 commits into from
Aug 5, 2021

Conversation

perenesenko
Copy link
Member

@perenesenko perenesenko commented Jul 12, 2021 •
edited by jessesuen
Loading

Fixes #1214

Signed-off-by: Andrii Perenesenko andrii.perenesenko@gmail.com

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, (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.

@perenesenko perenesenko changed the title feat: More aggressive cleanup of old AnalysisRuns and Experiments feat: More aggressive cleanup of old AnalysisRuns and Experiments. Fixes #1214 Jul 12, 2021
@codecov
Copy link

codecov bot commented Jul 12, 2021 •
edited
Loading

Codecov Report

Merging #1342 (01b16c6) into master (e3dc6af) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1342      +/-   ##
==========================================
+ Coverage   81.26%   81.33%   +0.07%     
==========================================
  Files         108      108              
  Lines       10033     9955      -78     
==========================================
- Hits         8153     8097      -56     
+ Misses       1319     1306      -13     
+ Partials      561      552       -9
Impacted Files Coverage Δ
rollout/analysis.go 80.75% <100.00%> (+0.14%) ⬆️
rollout/experiment.go 85.71% <100.00%> (+0.18%) ⬆️
utils/analysis/filter.go 97.93% <100.00%> (+0.46%) ⬆️
utils/defaults/defaults.go 92.00% <100.00%> (+3.29%) ⬆️
utils/experiment/filter.go 100.00% <100.00%> (ø)
metricproviders/kayenta/kayenta.go 83.11% <0.00%> (-0.85%) ⬇️
rollout/bluegreen.go 70.91% <0.00%> (ø)
utils/replicaset/replicaset.go 89.58% <0.00%> (+0.10%) ⬆️
rollout/replicaset.go 68.42% <0.00%> (+0.89%) ⬆️
utils/replicaset/canary.go 82.56% <0.00%> (+0.91%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3dc6af...01b16c6. Read the comment docs.

huikang
huikang reviewed Jul 13, 2021
View reviewed changes
examples/analysis-templates.yaml Show resolved Hide resolved
pkg/apis/rollouts/v1alpha1/types.go Outdated Show resolved Hide resolved
utils/analysis/filter.go Outdated Show resolved Hide resolved
huikang
huikang reviewed Jul 14, 2021
View reviewed changes
utils/analysis/filter.go Outdated Show resolved Hide resolved
utils/analysis/filter.go Outdated Show resolved Hide resolved
utils/analysis/filter_test.go Show resolved Hide resolved
utils/defaults/defaults.go Outdated Show resolved Hide resolved
@perenesenko perenesenko mentioned this pull request Jul 14, 2021
Closed
huikang
huikang reviewed Jul 20, 2021
View reviewed changes
utils/defaults/defaults.go Outdated Show resolved Hide resolved
huikang
huikang reviewed Jul 20, 2021
View reviewed changes
Copy link
Member

@huikang huikang left a comment

Choose a reason for hiding this comment

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

Some minor comments; otherwise it looks good.

rollout/analysis.go Outdated Show resolved Hide resolved
@perenesenko
feat: More aggressive cleanup of old AnalysisRuns and Experiments (1214)
bb1d92b 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: Adding description for the analysis run history configuration t…
8d8bfac 
…o the specification.md

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: Adding description for the analysis run history configuration i…
46bc616 
…n defaults.go

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: Change the parameter olderRSs => allRSs
92c251f 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: Updating test, shuffle the ARs order
87429e0 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
Adding tests for AnalysisRun history limits
c4aa8a5 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
lint fix, adding new line for import
d39f4e0 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
launch codegen to pass the build checks
cfb6986 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
fix the tests, remove accidental modification
0dcb5a5 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
launch codegen
d37650d 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko perenesenko force-pushed the perenesenko/1214-cleanup-analysisrun branch from 2a3f0ad to d37650d Compare July 26, 2021 18:01
@perenesenko
Adding tests for AnalysisRun history limits
9ecdf29 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
lint fix sorting import
01536ea 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
jessesuen
jessesuen requested changes Aug 3, 2021
View reviewed changes
pkg/apis/rollouts/v1alpha1/types.go Outdated Show resolved Hide resolved
docs/features/specification.md Outdated Show resolved Hide resolved
pkg/apis/rollouts/v1alpha1/types.go Outdated Show resolved Hide resolved
@perenesenko
feat: More aggressive cleanup of old AnalysisRuns and Experiments. Re…
ebaaf3e 
…view fix: field renaming

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup of old AnalysisRuns and Experiments. Re…
6eeedbe 
…view fix: field renaming

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup of old AnalysisRuns and Experiments. Re…
d9ead0c 
…view fix: field renaming

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup for only 2 phases successful and failed
8858399 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup of old AnalysisRuns and Experiments. Re…
6e3374e 
…name Analysis => AnalysisStrategy

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
Copy link
Member Author

On standup was decided to reuse the same parameters analysis.* for the Experiments cleaning

@perenesenko
Copy link
Member Author

On a standup also discussed about the changing analysis.failed... => analysis.unsuccessful... and that parameter should includes all 3 phases: Failed, Error, Inconclusive

@perenesenko
feat: More aggressive cleanup of old AnalysisRuns and Experiments. Us…
eb3403e 
…e unsuccessful for 3 phases: error, failed and inconclusive

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup of old Experiments
5a8de9c 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup of old Experiments. Adding tests
43d6813 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
Change imports order to fix linter
24d3b46 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup of old Experiments. Default limits to 5
0a91596 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@perenesenko
feat: More aggressive cleanup of old Experiments. Codegen
720fdf8 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
huikang
huikang reviewed Aug 4, 2021
View reviewed changes
Copy link
Member

@huikang huikang left a comment

Choose a reason for hiding this comment

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

Just some minor questions. Thanks.

pkg/apis/rollouts/v1alpha1/types.go Outdated Show resolved Hide resolved
utils/analysis/filter.go Outdated Show resolved Hide resolved
@perenesenko
feat: More aggressive cleanup of old Experiments. Adding docs, rename…
d5a3c2c 
… AnalysisStrategy=>AnalysisRunStrategy

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@huikang
Copy link
Member

huikang commented Aug 4, 2021

LGTM

@perenesenko
sort imports
01b16c6 
Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jessesuen jessesuen changed the title feat: More aggressive cleanup of old AnalysisRuns and Experiments. Fixes #1214 feat: configurable and more aggressive cleanup of old AnalysisRuns and Experiments Aug 5, 2021
@jessesuen jessesuen merged commit ffe70da into argoproj:master Aug 5, 2021
mdb pushed a commit to mdb/argo-rollouts that referenced this pull request Aug 10, 2021
@perenesenko @mdb
feat: configurable and more aggressive cleanup of old AnalysisRuns an…
6b66e9d 
…d Experiments (argoproj#1342)

Signed-off-by: Andrii Perenesenko <andrii.perenesenko@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huikang huikang huikang left review comments

@jessesuen jessesuen jessesuen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

More aggressive cleanup of old AnalysisRuns and Experiments
3 participants
@perenesenko @huikang @jessesuen