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.
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
feat: construct library for ECS #1058
feat: construct library for ECS #1058
Changes from 129 commits
939452c
7302a2f
4c7127a
98749ec
6d0942f
bddfe56
3574611
a63c377
b00e0dd
80ba2e4
0980f52
8648540
2d53d89
dbdab86
ee6b427
2b16dad
16a92c0
ae881c2
0672e6a
4f93a5b
9b61b89
98bf81b
ae1d525
23072a9
e5cc7f2
047eed8
65d3f79
11541d3
fb17532
be8cb86
b8cce3a
2741608
a901912
a91bda7
51adff5
70ae682
298e0a7
1541edd
a154d09
4b46685
9875c6e
1044d1c
e7f8000
e7f7b7d
dda869d
97083dc
a3e3615
b913f0e
fd21c0d
3b4695c
187f62b
6912e3f
2bf2d57
f477d8d
b947f51
ab83d35
30cb925
c64e184
566aac3
96e3105
48cd8d4
e311293
4ffed09
97d3db3
4f9b548
e8aa186
eb4a562
e06a3ec
95b8d84
5595707
a1b78f4
6f08406
5312362
6f42026
1983858
4ea6b8b
e34d515
bf90c27
ffbad9f
d1b22bd
7840634
d1ec640
36e1ba2
178898f
b73bdad
049557e
2295ef2
f34c23c
c491a1d
430f81e
dca6972
cf528d6
ef51786
c3b543a
64b6d06
af5a2a4
51097fd
a8f50e4
4baaeef
7b19dd2
71766e1
9bea6be
4ab455c
be9f447
2eab513
57857a9
4cf23fc
37d3c4e
1ad3669
78d2a49
ca61a74
98345b3
35ebd9c
7bbe9bb
09a182a
2fd2e7c
9825127
eea141e
67761f5
ccd8f34
15755d0
c28393a
7faa05d
4f561cd
47e5251
6152ddc
62ff3a4
b2fa1db
410d263
360d42f
8c76aa4
45a82c8
631c35d
013a52c
ce083db
73680ef
61c0edb
3a7778b
6e5c3b2
36687dc
ca9fcb3
0dbbe63
cc69519
645b9a0
4ed101b
54e4665
e2d338f
b0f54a8
8f0f6b1
15b8804
07f1f57
d883432
edd4d5f
ed0956a
33a35b9
2431901
7c0a148
32db35d
233a376
b616248
ea6eeae
98990b4
ffa7360
908db1f
64c37fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
From reading this, I don't understand why we need specialized classes for FargateTaskDefinition and FargateService. I am assuming I'll find out why later on, but intuitively I expected just the cluster to have a different concrete class.
The reason why this might be an issue for customers is if they want to port an ECS service/task-def from EC2 to Fargate. Ideally, if they could just change the cluster and it will Just Work, that would have been a nice capability.
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.
I know, and this was our original plan as well. But there are a number of differences that make this tricky:
After discussion we decided that task migration wasn't that likely, so it was okay if it didn't work out of the box.
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.
Can we document these differences in the README?
Still feels like we are missing something... For example, if a 3rd party library wants to vend a
TaskDefinition
for reuse, they will have to either vend two (one for Fargate and one for EC2) or users will not be able to use them interchangeably.Maybe what we can do is define a
TaskDefinition
which is the union of all options, so I can define a task definition that can be used across EC2/Fargate. Does that make any sense?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.
I don't think it should be broken up--we don't break it up in the console even. One of the things that we talk about is how you can easily migrate from EC2 to Fargate and vice versa. Also if you use awsvpc you can easily create a task definition that works for both. There isn't any field in a task definition that makes it Fargate or EC2 specific--just the values or IAM roles you give--but both can take the same things (sans task placement). Right now the only checks happen when you include the requires compatibilities field--which can take ec2 and fargate, not just one.
For service via the API it seems the differences are just giving fargate/ec2 as the launch type, platform version if fargate (and not the latest), and task placement (ec2).
I think breaking things up further than the API is a bit confusing.
Also for clusters, why not just one create cluster. And then an option to add instances as a separate function given into create cluster? Thinking this since any cluster you create as ec2 can be used for Fargate. Also if you create a cluster without instances for fargate, you can just use the addinstances function after the fact.
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.
@rix0rrr What do you mean you specify instance sizes in a task definition? You can only have task/container mem/cpu.
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.
@tiffanyfay brings up a fair point about ECS clusters that can contain both Fargate and EC2 instances in them.
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.
Begin able to add instances also sounds like a useful feature
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.
If you look at the current implementation in
Ec2Cluster
, that is currently what we are doing already, except via an ASG. @rix0rrr and I had already discussed this following this other comment regarding configurability of ASGs. The reason why we wanted to keep this separate was to hide any properties having to do with instances from the Fargate namespace.I'm not sure what you mean by
size
in youraddInstances
example -- thesize
field on theEc2Cluster
construct currently refers to the size of the cluster, e.g. number of instances. I assume you mean count?Finally, Rico and I had also already discussed that userData is something that you should be able to add separately -- I believe it was still in the context of an ASG.
In re: adding instances separately, do we actually have data supporting instances existing in clusters outside of an ASG? Both the console and the ecs-cli currently only provision capacity through ASGs today, which is why we modeled the
Ec2Cluster
construct this way. I'm not against the idea of adding an API that adds individual capacity, but it seems a little strange to want instances that isn't part of an ASG, but perhaps @tiffanyfay you have a concrete customer use case?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.
Yea, saw that and think that the ASG should be moved into the
addInstances
instead. So since ECS has a cluster which has both EC2 and Fargate vs them being individual clusters, I think there should be just oneCluster
construct.My thought of
addInstances
(or maybe it should be addInstanceASG or something) is not to scale existing types, but to add new types. Examples being:-Create cluster with size: 3 type: t2.micro and I want to add 4 t2.medium instances in an ASG.
-Create cluster without instances (e.g. Fargate in the current code) and add 4 t2.medium instances in an ASG.
Basically I want to easily be able to have different types of instances. So actually it would need to only allow one asg in order to return something you can scale individually. Right now it can be done by adding another autoscaling launch config and asg to your CloudFormation template and updating your stack.
Kind of like the RegisterContainerInstance API plus creating the asg/instances for you.
Oh, yes, count-- I had just copy pasted.
Would it be able to be added during instance/asg creation or only after the fact and updating?
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.
The
RegisterContainerInstance
is an internal API only used by the agent; it's not externally exposed to customers IIRC. I think having anaddCapacity
API isn't a bad idea, as long as it's well-defined and aligned with best practices. Currently, we have plans to allow instance auto-scaling (which @rix0rrr is working on).For me I am still not convinced there is good justification for not separating the cluster types. As previously mentioned, there is an extremely low number of customers today who run both kinds of tasks/services in the same cluster -- we originally supported it to ease migration, but in today's world where both services are available as options it seems like we should be supporting a more streamlined workflow. Do you happen to have concrete customer use cases for what kind of system is running both kinds of services?
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.
Didn't we merge Ec2TaskDefinition with FargateTaskDefinition?
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.
Maybe, we can add some sugar to make this a bit more symmetric and discoverable:
(I would still keep
repository.getImage(tag)
, but it would be nice if the usage from ECS's side would be symmetric)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.
I would remove
repository.getImage(tag)
to avoid the dependency between ECR => ECS (that's the wrong direction)