aws-ecs-patterns: add support for `pidMode` in `ApplicationLoadBalancedFargateService`

noseworthy commented 2 weeks ago

Describe the feature

I'd like to configure the pidMode for the fargate task definition that I'm building using the aws-ecs-patterns.ApplicationLoadBalancedFargateService construct. However, the construct takes a ApplicationLoadBalancedFargateServiceProps object which doesn't allow configuring pidMode.

It'd be great to add pidMode as an option to ApplicationLoadBalancedFargateServiceProps so that this can be configured without having to build a full FargateTaskDefinition construct.

Use Case

I need to configure pidMode so that I can monitor live process information my fagate ecs tasks with DataDog. Replacing my use of taskImageOptions with a new taskDefinition property will cause a full replacement of my task definition which i'm trying to avoid. I'm surprised that taskImageOptions doesn't support pidMode.

Proposed Solution

Add an optional pidMode property to FargateServiceBaseProps and if specified, provide it to the FargateTaskDefinition construct created in application-load-balanced-fargate-service.ts

...

      this.taskDefinition = new FargateTaskDefinition(this, 'TaskDef', {
        memoryLimitMiB: props.memoryLimitMiB,
        cpu: props.cpu,
        ephemeralStorageGiB: props.ephemeralStorageGiB,
        executionRole: taskImageOptions.executionRole,
        taskRole: taskImageOptions.taskRole,
        family: taskImageOptions.family,
        runtimePlatform: props.runtimePlatform,
        pidMode: props.pidMode,
      });

...

Other Information

Related Issues:

Acknowledgements

[X] I may be able to implement this feature request
[ ] This feature might incur a breaking change

CDK version used

2.148.1

Environment details (OS name and version, etc.)

macOS Sonoma 14.6.1, Macbook Pro 2021, Apple m1 Max

khushail commented 2 weeks ago

Hi @noseworthy , thanks for reaching out. L3 Constructs like aws-ecs-patterns are very much opinionated and usecase based. On checking CDK Docs, FargateTaskDefinition has Pidmode property - https://github.com/aws/aws-cdk/blob/aedf617a754fff87d3cde7efbb996bec6f0fa129/packages/aws-cdk-lib/aws-ecs/lib/fargate/fargate-task-definition.ts#L94 hence it makes sense to make it available here as well - https://github.com/aws/aws-cdk/blob/4b8714d7b499362f0b77ad9a615479227e339078/packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts#L74 Please feel free to contribute the PR. Team would be happy to review it. Thanks.

noseworthy commented 1 week ago

Thanks for reviewing the issue, @khushail!

I've submitted PR #31461 for review. I haven't added any integration tests as I'm not sure that it's required as it's just exposing an existing property of an underlying construct, and frankly it's going to be hard for me at the moment to build and snapshot the integration test. Hopefully I'm able to get an exemption, but if not, I'll just have to try and build it later when I get back to work.

khushail commented 1 week ago

Hi @noseworthy , thanks for your contribution. Appreciate your efforts! Please see that for PR approval, first it goes through community review. You can reach out to CDK.dev community on slack for seeking support for review for your PR. Once its get reviewed by community, it would be reviewed by the CDK Core team and afterwards it will get merged. let me know if you have any further questions !

aws / aws-cdk