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

Use Patch instead of Update for Deployment scaling #1634

Merged
merged 1 commit into from May 22, 2024

Conversation

relu
Copy link
Member

@relu relu commented Apr 24, 2024

This should avoid frequent "Operation cannot be fulfilled" errors from polluting Canary resource events and logs.

@relu relu requested a review from stefanprodan as a code owner April 24, 2024 11:23
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 34.09%. Comparing base (5e6815d) to head (553184b).
Report is 1 commits behind head on main.

Files Patch % Lines
pkg/canary/deployment_controller.go 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1634      +/-   ##
==========================================
- Coverage   34.10%   34.09%   -0.01%     
==========================================
  Files         282      282              
  Lines       20557    20554       -3     
==========================================
- Hits         7011     7008       -3     
  Misses      12616    12616              
  Partials      930      930              

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

@stefanprodan stefanprodan changed the title Use Patch instead of Update in the deployment_controller when scaling Use Patch instead of Update for Deployment scaling Apr 24, 2024
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @relu

Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

if its not too much trouble, could you make the same change for the Daemonset controller?

@relu
Copy link
Member Author

relu commented Apr 30, 2024

@aryan9600 I pushed an attempt to solve that for the DS controller, but I'm not convinced it's the right way to go. Since we don't have a replicas field on the DaemonSet, we're working with the nodeSelector instead. Because that's a map, the only options to patch are via StrategicMerge (nullifying the values) or JSONPatch.

@aryan9600
Copy link
Member

okay, then lets keep that out of this PR. we can look into it outside of here.

@relu
Copy link
Member Author

relu commented May 15, 2024

Cool. I've removed the commit with the changes to the DS controller and we can revisit it in a separate PR.

Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

lgtm!

This should avoid frequent "Operation cannot be fulfilled" errors from
polluting Canary resource events and logs.

Signed-off-by: Aurel Canciu <[email protected]>
@aryan9600 aryan9600 merged commit 04f5c68 into fluxcd:main May 22, 2024
17 checks passed
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.

None yet

4 participants