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: automatically scale down Deployment after migrating to Rollout #3111

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

balasoiuroxana
Copy link
Contributor

@balasoiuroxana balasoiuroxana commented Oct 17, 2023

fixes: #2748

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.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2023

Go Published Test Results

2 089 tests   2 089 ✔️  2m 48s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit b57a8fb.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2023

E2E Tests Published Test Results

    4 files      4 suites   3h 31m 22s ⏱️
106 tests   96 ✔️   6 💤   4
438 runs  400 ✔️ 24 💤 14

For more details on these failures, see this check.

Results for commit b57a8fb.

♻️ This comment has been updated with latest results.

@balasoiuroxana balasoiuroxana changed the title Automatically scale down Deployment after migrating to Rollout feat: automatically scale down Deployment after migrating to Rollout Oct 17, 2023
@balasoiuroxana balasoiuroxana force-pushed the deployments-scale-down branch 3 times, most recently from b283ca7 to f5b4bbf Compare October 18, 2023 14:14
@carlossg
Copy link
Contributor

fixes #2748

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (c067a69) 81.80% compared to head (b57a8fb) 81.81%.
Report is 2 commits behind head on master.

Files Patch % Lines
rollout/replicaset.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3111      +/-   ##
==========================================
+ Coverage   81.80%   81.81%   +0.01%     
==========================================
  Files         134      135       +1     
  Lines       20576    20621      +45     
==========================================
+ Hits        16832    16871      +39     
- Misses       2875     2880       +5     
- Partials      869      870       +1     

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

@dharma-rise
Copy link

Any updates?

@zachaller
Copy link
Collaborator

Can we add some documentation here and here on the behavior of all the scaledown options etc?

rollout/sync.go Outdated
@@ -378,6 +378,12 @@ func (c *rolloutContext) scaleReplicaSet(rs *appsv1.ReplicaSet, newScale int32,
revision, _ := replicasetutil.Revision(rs)
c.recorder.Eventf(rollout, record.EventOptions{EventReason: conditions.ScalingReplicaSetReason}, conditions.ScalingReplicaSetMessage, scalingOperation, rs.Name, revision, oldScale, newScale)
c.replicaSetInformer.GetIndexer().Update(rs)
if rollout.Spec.WorkloadRef != nil && rollout.Spec.WorkloadRef.ScaleDown == v1alpha1.ScaleDownProgressively {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that for the ScaleDownProgressively option the deployment will be scaled down without waiting for the rollout replicas to become available.
Do you have any suggestions where we can test that? We should scale down the deployment only after the replicas become available.
cc: @carlossg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the logic for the progressively scaleDown option, now the deployment is scaled down as the rollout is scaled up and its replicas become ready.

@zachaller
Copy link
Collaborator

zachaller commented Nov 22, 2023

Is this ready for a second review now?

@balasoiuroxana
Copy link
Contributor Author

@zachaller , yes it is ready

@zachaller zachaller added this to the v1.7 milestone Nov 22, 2023
@zachaller
Copy link
Collaborator

@balasoiuroxana looks good and ready to merge, can you fix the last small conflict?

Copy link

sonarcloud bot commented Dec 5, 2023

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 1 Code Smell

No Coverage information No Coverage information
1.0% 1.0% Duplication

@zachaller zachaller merged commit 0ec8519 into argoproj:master Dec 5, 2023
24 checks passed
ashutosh16 pushed a commit to ashutosh16/argo-rollouts that referenced this pull request Dec 8, 2023
…rgoproj#3111)

Automatically scale down Deployment after migrating to Rollout

Signed-off-by: balasoiu <[email protected]>
Co-authored-by: balasoiu <[email protected]>
Signed-off-by: ashutosh16 <[email protected]>
@Ahmed-Elkollaly
Copy link

@balasoiuroxana @zachaller Thanks for this awesome feature but I don't see it in the latest argo rollout version v1.6.4. when it's expected to be rolled out?

@zachaller
Copy link
Collaborator

It will be available in 1.7 we don't have a set date on that yet, I want to try and get step plugins in 1.7

@rmn-lux
Copy link

rmn-lux commented Feb 28, 2024

It will be available in 1.7 we don't have a set date on that yet, I want to try and get step plugins in 1.7

Hello, is there any information about the release date of v1.7?

@SleepyBrett
Copy link

Couldn't you also do this the other way as well.

On deletion of the rollout the reconciler takes the current replicas of the rollout and applies it to the referenced deployment (if available). Once the pods become healthy, delete the rollout's child replica-sets and remove the finalizer?

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.

Automatically scale down Deployment after migrating to Rollout
7 participants