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

FargateService cannot specify both launchType and capacityProviderStrategies #599

Closed
kennyjwilli opened this issue Nov 28, 2020 · 10 comments · Fixed by #644
Closed

FargateService cannot specify both launchType and capacityProviderStrategies #599

kennyjwilli opened this issue Nov 28, 2020 · 10 comments · Fixed by #644
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec
Milestone

Comments

@kennyjwilli
Copy link

The launchType is currently explicitly set to FARGATE. When using capacityProviderStrategies, you cannot also specify a launchType. If you try to, pulumi up will fail with this error.

InvalidParameterException: Specifying both a launch type and capacity provider strategy is not supported. Remove one and try again.

Here is a minimal repro.

import * as awsx from "@pulumi/awsx";

new awsx.ecs.Cluster("cluster", {
    capacityProviders: ["FARGATE", "FARGATE_SPOT"],
    defaultCapacityProviderStrategies: [{
        base: 0,
        capacityProvider: "FARGATE",
        weight: 0
    }, {
        base: 0,
        capacityProvider: "FARGATE_SPOT",
        weight: 1
    }],
})

new awsx.ecs.FargateService("service", {
    desiredCount: 1,
    taskDefinitionArgs: {
        containers: {
            nginx: {
                image: "nginx:latest",
                memory: 512
            }
        }
    },
    capacityProviderStrategies: [{
        capacityProvider: "FARGATE_SPOT",
        weight: 1
    }]
})

Explicitly specifying the launchType also prevents the cluster default capacityProviderStrategy from being applied.

From the RunTask API doc on capacityProviderStrategy:

If a capacityProviderStrategy is specified, the launchType parameter must be omitted. If no capacityProviderStrategy or launchType is specified, the defaultCapacityProviderStrategy for the cluster is used.

To fix this, I suggest allowing the launchType parameter to be passed in to the FargateService.

@stack72 stack72 added the kind/bug Some behavior is incorrect or out of spec label Nov 29, 2020
@kennyjwilli
Copy link
Author

service.ts applies a default value when no launchType is specified. That will need to be fixed too.

@stack72
Copy link
Contributor

stack72 commented Jan 19, 2021

Hey @kennyjwilli

My apologies this has gone so long without being triaged / commented upon. So I believe the behaviour should be the following (for both EC2Service and FargateService)

  • If capacityProviderStrategies is specified then we should now add a default launchType

This will allow each of those service types to no longer pass both parameters

If you are happy with this, I can get this update and shipped

Paul

@kennyjwilli
Copy link
Author

Thanks for taking a look @stack72.

If capacityProviderStrategies is specified then we should now add a default launchType

I read "now" to be "not." This will not cover the case AWS defines as "If no capacityProviderStrategy or launchType is specified, the defaultCapacityProviderStrategy for the cluster is used.". These are the cases that are supported by AWS and I think should be supported by this API.

  • launchType specified. Either EC2 or FARGATE. When FARGATE, tasks are always launched on on-demand infrastructure.
  • capacityProviderStrategies is not specified and launchType is not specified: the defaultCapacityProviderStrategy for the cluster is used.
  • capacityProviderStrategies is specified and launchType is not specified: the specified capacityProviderStrategies is used.

The fix I originally suggested (allowing launchType to be specified) is a breaking change. It'd require removing the default value set for launchType thus implicitly changing the default behavior. If users were to update to this version and not know about this change, their tasks would be launched using the defaultCapacityProviderStrategy. This is probably not desired.

@stack72 and I discussed a different solution -- adding a new parameter launchTypeOverride. This parameter would take EC2 | FARGATE | EMPTY. When specified, it would override the default launchType previously specified by the library. EMPTY would represent setting launchType to undefined.

This change would not be breaking for users, which is good. It does add some complexity to the API. Users will likely need to know the motivation for this parameter and when to use it (whenever using capacityProviderStrategies or wanting to use defaultCapacityProviderStrategy). It's also worth noting that if AWS adds a value to their launchType named EMPTY, it will make this API more confusing since it will then need to support AWS's EMPTY and Pulumi's EMPTY.

There is also a case for making the breaking change. With the addition of cluster defaultCapacityProviderStrategy, setting launchType does not seem like a good default anymore.

@leezen
Copy link
Contributor

leezen commented Jan 19, 2021

Another approach would be to do the following:

  • For both Ec2Service and FargateService, if capacityProviderStrategies is specified, then omit launchType
  • Introduce a new class DefaultCapacityProviderService that doesn't specify either launchType or capacityProviderStrategies

I think this would be preferred because it doesn't attempt to introduce having the user reason about launchType itself when the class is attempting to specify that directly. (It would be very odd to think about setting EC2 as a launchTypeOverride on FargateService)

@kennyjwilli
Copy link
Author

I like that approach @leezen. I might, however, suggest a different, more general name. This new class could avoid setting defaults and still provide the nice awsx API. Perhaps simply making this change to the underlying Service class. Notice that the underlying Service class sets the launchType to EC2 when undefined. That default seems a bit odd IMO.

I thought I'd offer one other alternative. We could keep the classes as is and add in a useDefaultClusterProviderStrategy: boolean parameter. When true, it would omit both launchType and capacityProviderStrategies. When undefined or false, we can infer the correct behavior from the args the user passes in. If the user specifies capacityProviderStrategies, do not specify launchType. If the user does not specify capacityProviderStrategies, set launchType to FARGATE for FargateService and EC2 for EC2Service.

@leezen leezen self-assigned this Jan 23, 2021
@leezen
Copy link
Contributor

leezen commented Jan 28, 2021

I like that approach @leezen. I might, however, suggest a different, more general name. This new class could avoid setting defaults and still provide the nice awsx API. Perhaps simply making this change to the underlying Service class. Notice that the underlying Service class sets the launchType to EC2 when undefined. That default seems a bit odd IMO.

Given Service is abstract at the moment, I think we'll just want a concrete class and then undo the EC2 default launch type in Service as you said.

I thought I'd offer one other alternative. We could keep the classes as is and add in a useDefaultClusterProviderStrategy: boolean parameter. When true, it would omit both launchType and capacityProviderStrategies. When undefined or false, we can infer the correct behavior from the args the user passes in. If the user specifies capacityProviderStrategies, do not specify launchType. If the user does not specify capacityProviderStrategies, set launchType to FARGATE for FargateService and EC2 for EC2Service.

My 2 cents: it feels like useDefaultClusterProviderStrategy would be quite similar to launchTypeOverride in that it's an extra parameter that forces the user to think about the interplay between the various arguments being passed into the particular class. The existing service classes more or less already encapsulate the idea that the launchType is going to be EC2 or Fargate so trying to also shim in the ability to ignore that on both and allow for using the default ends up with two ways to do the same thing. I think I'd prefer a new class that clearly doesn't specify a launchType and allows the user to set the capacityProviderStrategy appropriately (or omit it to get the cluster default).

@deevus
Copy link

deevus commented Feb 18, 2021

Is there a workaround, or do we have to wait for a proper fix?

@leezen
Copy link
Contributor

leezen commented Feb 21, 2021

@kennyjwilli Did you want to take a stab at the approach outlined above? Would be happy to review a PR.

@kennyjwilli
Copy link
Author

Realistically, I don't think I'm going to be able to get to it soon. I will add it to my list though to tackle if no one picks it up first.

@zvictor
Copy link

zvictor commented Sep 27, 2022

How can I rewrite the code posted by the author of this issue to fit the changes that were made to Pulumi.
In other words, how to make a service run as FARGATE_SPOT instead of FARGATE?

I am having a hard time to find any documentation on that. 🥹

I tried setting up defaultCapacityProviderStrategies in the cluster, but the service still runs as non-spot and ignore the capacity providers defined in the cluster.

    const cluster = new awsx.ecs.Cluster(
      'default-cluster',
      {
        capacityProviders: [
          // 'FARGATE',
          'FARGATE_SPOT',
        ],
        defaultCapacityProviderStrategies: [
          {
            weight: 100,
            capacityProvider: 'FARGATE_SPOT',
          },
          // {
          //   weight: 0,
          //   capacityProvider: 'FARGATE',
          // },
        ],
      }
    )

    const service = new awsx.ecs.FargateService(
      'meilisearch',
      {
        cluster,
        desiredCount: 1,
        taskDefinitionArgs: {
          container: {
            image: 'getmeili/meilisearch',
            cpu: 512,
            memory: 128,
            essential: true,
            environment: Object.entries({
              MEILI_MASTER_KEY: this.masterKey,
              MEILI_HTTP_ADDR: `0.0.0.0:${PORT}`,
              ...environment,
            }).map(([name, value]) => ({ name, value })),
          },
        },
      }
    )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec
Projects
None yet
5 participants