-
Notifications
You must be signed in to change notification settings - Fork 336
Ignore desiredCount of ECS when it's 0 or not set for AutoScaling
#5030
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
Merged
Merged
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
6b2af75
add a flag: ignoreDesiredCountOnUpdate
t-kikuc cf3efbf
ignore desiredCount on updateService if needed
t-kikuc d17a9b7
Add a flag ignoreDesiredCountOnUpdate into each func
t-kikuc 5103158
Format func params and comment
t-kikuc dcd7dc9
add docs
t-kikuc 7d65824
fix tests: add a missing attribute
t-kikuc 2c8ef1a
recover desiredCount in CreateService (mistake)
t-kikuc a0f3459
add mentioning driftdetection
t-kikuc e556faf
Revert changes of adding ignoreDesiredCountOnUpdate
t-kikuc 8ae6647
Revert commits of adding ignoreDesiredCount"
t-kikuc 0bf97d5
Revert "add docs"
t-kikuc cd310a7
Revert "Format func params and comment"
t-kikuc 638c5f9
Revert "Add a flag ignoreDesiredCountOnUpdate into each func"
t-kikuc 936dfb2
Revert "ignore desiredCount on updateService if needed"
t-kikuc eb3bdc7
Revert "add a flag: ignoreDesiredCountOnUpdate"
t-kikuc 766debd
ignore updating desiredCount if it's 0 or not defined
t-kikuc aef6c41
Merge branch 'master' of https://github.com/pipe-cd/pipecd into ecs-i…
t-kikuc 434c3af
Use pruning when 'recreate' is on
t-kikuc 7b267a1
add docs
t-kikuc 33985f8
refine docs
t-kikuc e644dc0
Merge branch 'master' into ecs-ignore-desired-count
t-kikuc b58fb8c
Clarify procedure of configuring
t-kikuc 99060a0
Merge branch 'ecs-ignore-desired-count' of https://github.com/pipe-cd…
t-kikuc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@t-kikuc I think when users tend to ignore expressly setting the
desiredCountvalue, they will leave it empty (or remove that line from the manifests); having this flag added can lead to some confusing configuration when users set thedesiredCountvalue in service file but set this flag in the app configuration as true 🤔For whatever cases, how about to change this logic as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khanhtc1202
Thank you so much. Your code seems really simple and great, and I also think they will remove
desiredCount.However, there are two patterns we should consider.
desiredCount: 0,desiredCountwon't be 0.desiredCount.desiredCount= 5desiredCount= 5desiredCount= 0That's why I think the new config attribute is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@t-kikuc Across the Kubernetes QuickSync configuration of pipecd, I think currently we don't support prune resource (tasks) for ECS application, which mean we can forget about the behavior when users set
desiredCountto zero to remove the tasks (ref: https://pipecd.dev/docs-v0.48.x/user-guide/configuration-reference/#kubernetesquicksync).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I understand the need to support the prune resource on deployment, but I think it should be a separate change, not this PR. Also, in such cases, we can just add a new interface (not update this UpdateService) but call the new function like PruneService. Then, in the implementation, we set the desiredCount to zero.
My point is, we should not change the interface whenever we need a change that is not actually worth; creating a new one is a better choice 😄 (If follow the OOP principles - if you're a former java dev, you know what I mean 😉 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khanhtc1202
To be precise, this is not pruning, it's scaling-in.
Of course, I know the principle and won't use UpdateService() for pruning.
You mean scaling-in to 0 tasks is not necessary?
I found a simpler way based on your code.
Let's ignore the drift If
desiredCountdesiredCountin new commit = 0Then we can support both of two patterns:
desiredCount(>0) in the new commitdesiredCountis 0 or not definedIn case I mentioned, a user should remove
desiredCountfrom the serviceDef.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khanhtc1202
What do you think about recreating? It sets
desiredCountas 0.https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/executor/ecs/ecs.go#L277-L286
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@t-kikuc Thanks for making it clear; I forgot about the recreating case 😢
Then, how about
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khanhtc1202
Thanks,
Let's ignore "scaling-in to 0 tasks is not necessary" cases for simplicity.
It is not a common case for production-ready services.
If needed, let's support
prunefeature like K8sApp.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 🙏