Ignore desiredCount of ECS when it's 0 or not set for AutoScaling

[t-kikuc]

What this PR does / why we need it:

- Introduced a new config attribute ignoreDesiredCountOnUpdate, which ignores desiredCount when updating a service.

• [edited] Ignore desiredCount on updating a service if it's 0 or not set.
• The purpose is to avoid unintended scale-in when configuring Auto Scaling.

Which issue(s) this PR fixes:

Fixes #5027

Does this PR introduce a user-facing change?: Yes

• How are users affected by this change: They can use a new config attribute. no
• Is this breaking change: No
• How to migrate (if breaking change): N/A

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 22.46%. Comparing base (aff6609) to head (99060a0).
Report is 16 commits behind head on master.

Files	Patch %	Lines
pkg/app/piped/platformprovider/ecs/client.go	0.00%	17 Missing ⚠️
pkg/app/piped/executor/ecs/ecs.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5030      +/-   ##
==========================================
+ Coverage   22.42%   22.46%   +0.03%     
==========================================
  Files         522      522              
  Lines       56915    57056     +141     
==========================================
+ Hits        12766    12816      +50     
- Misses      43123    43213      +90     
- Partials     1026     1027       +1     

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

[khanhtc1202]

@t-kikuc I think when users tend to ignore expressly setting the desiredCount value, 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 the desiredCount value in service file but set this flag in the app configuration as true 🤔
For whatever cases, how about to change this logic as

	input := &ecs.UpdateServiceInput{
		Cluster:              service.ClusterArn,
		....
	}
	if service.DesiredCount != 0 {
		input.DesiredCount = aws.Int32(service.DesiredCount)
	}

[t-kikuc]

@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.

1. Even if a user truly wants to scale-in tasks to 0 by desiredCount: 0, desiredCount won't be 0.
   • e.g. When a user wants to keep the service but it does not need to serve.
2. DriftDetection will be inevitable because a piped cannot judge whether to ignore desiredCount.
   • case-a.
     1. current desiredCount= 5
     2. ->Scale up to 10 by AutoScaling
     3. -> DriftDetection will reconcile to 5.
   • case-b.
     1. current desiredCount= 5
     2. -> new desiredCount= 0
     3. -> DriftDetection will try to reconcile to 0 every time (but it won't actually be 0).

That's why I think the new config attribute is required.

[khanhtc1202]

@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 desiredCount to zero to remove the tasks (ref: https://pipecd.dev/docs-v0.48.x/user-guide/configuration-reference/#kubernetesquicksync).

[khanhtc1202]

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 😉 )

[t-kikuc]

@khanhtc1202

we don't support prune resource

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?

DriftDetection

I found a simpler way based on your code.

Let's ignore the drift If

• diff is only desiredCount
• desiredCount in new commit = 0

Then we can support both of two patterns:

• reconciling to desiredCount(>0) in the new commit
• skipping reconciling when desiredCount is 0 or not defined

In case I mentioned, a user should remove desiredCount from the serviceDef.

case-b.
current desiredCount= 5
-> new desiredCount= 0
-> DriftDetection will try to reconcile to 0 every time (but it won't actually be 0).

[t-kikuc]

@khanhtc1202
What do you think about recreating? It sets desiredCount as 0.

https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/executor/ecs/ecs.go#L277-L286

[khanhtc1202]

@t-kikuc Thanks for making it clear; I forgot about the recreating case 😢
Then, how about

• only allow desiredCount ignored for CreateService interface (aka. make a change to this interface: https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/platformprovider/ecs/client.go#L93)
• for UpdateService, only if the user directly sets desiredCount to 0 -> scales down in the size of the task to 0. If users don't mention desiredCount in the service manifest, just ignore it from the UpdateServiceInput
  wdyt?

[t-kikuc]

@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 prune feature like K8sApp.

[khanhtc1202]

Thank you 🙏

[khanhtc1202]

LGTM, thank you 🪨

[ffjlabo]

[Ask] When enabling ECS autoscaling, what should users do? In my understanding, they set desiredCount to 0 after setting up autoscaling. Is it right?

[Warashi]

Just to sure. I have the same question.

Users can follow the below steps to create an autoscaled service. Is this correct?

1. create a service with the desiredCount as 0 or not specified. if it is not specified, the desiredCount is treated as 0.
2. configure autoscaling, and autoscaling policy or something like this updates desiredCount.
3. when updating a service, piped ignores the desiredCount. So the count of tasks is not touched.

[Warashi]

LGTM

[ffjlabo]

LGTM 🆗