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

Adding support to Fargate #51

Merged
merged 1 commit into from May 14, 2018

Conversation

@guilhermef

guilhermef commented Jan 18, 2018

This PR adds support for the newly introduced LaunchType, Fargate.

@guilhermef

This comment has been minimized.

guilhermef commented Jan 18, 2018

I've generated a binary to be installed on jenkins: https://github.com/guilhermef/amazon-ecs-plugin/releases/tag/beta1

@ccelebi

This comment has been minimized.

ccelebi commented Jan 18, 2018

@guilhermef I'll give this a try as well. I've been looking to do this myself and been wanting this support.

@lifeofguenter

This comment has been minimized.

lifeofguenter commented Jan 22, 2018

@guilhermef thank you! There might be however some difficulties getting this PR merged in due to inactivity. Looking forward to testing it though :)

@mchugh19

This comment has been minimized.

mchugh19 commented Jan 25, 2018

Fantastic! Any chance anyone could generate a build of this and #48?

@marktb1

This comment has been minimized.

marktb1 commented Jan 31, 2018

Loving this @guilhermef. I've been playing with it for a day, and can't seem to get an actual fargate instance to spin up. The error thrown suggests that the executionRoleArn is needed, but I don't see anywhere to specify that in the plugin fields.

com.amazonaws.services.ecs.model.ClientException: Fargate requires task definition to have execution role ARN to support ECR images. (Service: AmazonECS; Status Code: 400; Error Code: ClientException; Request ID: 8b9e014d-06c2-11e8-8dcc-0f51300037f1)

FYI my jenkins instance has version 1.11.264 of the AWS sdk installed.
Any help appreciated :)

@marktb1

This comment has been minimized.

marktb1 commented Jan 31, 2018

Oh I see what's up. I'm trying to use a custom image from ecr on my account.

@marktb1

This comment has been minimized.

marktb1 commented Jan 31, 2018

@mchugh19 I've merged 48, this pr, and a modification of my own to allow for ecr repo's (added executionRoleArn) to a release here:
https://github.com/marktb1/amazon-ecs-plugin/releases/tag/amazon-ecs-1.0-beta2

Working for me :)

@carlossg

This comment has been minimized.

Contributor

carlossg commented Feb 2, 2018

@roehrijn do you have time to maintain the plugin or you need collaborators?

@yrsurya

This comment has been minimized.

yrsurya commented Feb 14, 2018

https://groups.google.com/forum/#!searchin/jenkinsci-dev/amazon-ecs%7Csort:relevance/jenkinsci-dev/lkffA4lMn2I/JO5I0h1rDgAJ

please click above link if any one interested to take over this plugin as @roehrijn is not active for long days

@jsmickey

This comment has been minimized.

jsmickey commented Feb 16, 2018

@marktb1 Installed the plugin you provided but unable to get it working. Log shows com.amazonaws.services.ecs.model.InvalidParameterException: Container instance cannot be empty.

Could somebody provide a working config? I'm not sure if I have CPU/memory correct. And I'm unsure about the Assign Public IP function.

I am able to see the clusters in the drop down and think IAM is ready to go. I feel I have the config wrong.

Any help is appreciated

@marktb1

This comment has been minimized.

marktb1 commented Feb 22, 2018

@jsmickey I use the config below and can switch between 'ec2' and 'fargate' mode
screen shot 2018-02-22 at 9 32 40 am

@tylersmith34

This comment has been minimized.

tylersmith34 commented Feb 23, 2018

@marktb1 I cloned your code and I'm a little baffled. https://github.com/marktb1/amazon-ecs-plugin/blob/master/src/main/java/com/cloudbees/jenkins/plugins/amazonecs/ECSCloud.java#L101 References an ECSService class that isn't in your code, there is no import statement and no POM dependency that has this class. Where is its definition? The class is defined in this pull request so something appears wrong in your fork.

@Steed9

This comment has been minimized.

Steed9 commented Feb 27, 2018

having an issue with jenkins finding the label. Before the fargate addin, I would occasionally get this error when soft mem was too high but fargate shouldnt have that issue. Anyone else hit anything similar?

@bakerlee

This comment has been minimized.

bakerlee commented Feb 27, 2018

@Steed9 This won't help you, but I have also hit this issue. For me, it seems to be transient.

@Steed9

This comment has been minimized.

Steed9 commented Feb 27, 2018

@marktb1 did you have to specify any advanced properties as well? I removed the docker.sock mount point I had but that didnt seem to do anything.

@jcragg

This comment has been minimized.

jcragg commented Mar 7, 2018

@jsmickey I produced a build with the code of #51 and #48 merged as well, since the above 'beta-2' was also giving me the same error. It looks like there was an if statement that wasn't merged re: Fargate in @marktb1's build. I've got Fargate slave builds working now with this build:

https://github.com/jcragg/amazon-ecs-plugin/releases/tag/fargate-plus-multislave-1

@mchugh19

This comment has been minimized.

mchugh19 commented Mar 9, 2018

@jcragg Are you able to use custom images from ecr with your fargate build? We had the same issue mentioned in #51 (comment)

@jcragg

This comment has been minimized.

jcragg commented Mar 9, 2018

@mchugh19 Yep, I have been using private ECR images, however I did need to add support for Execution Role ARN. I created a second build/tag with that change as well:

https://github.com/jcragg/amazon-ecs-plugin/releases/tag/fargate-plus-multislave-2

I've also PR'ed the Task Execution Role support:
#53

@tecnobrat

This comment has been minimized.

tecnobrat commented Mar 23, 2018

Sounds like #48, #51 and #53 are all needed for a truly comprehensive feature set, is that correct? Would it make sense to get a single PR which adds all of these together to streamline getting this merged in?

@Eli-Goldberg

This comment has been minimized.

Eli-Goldberg commented Apr 3, 2018

Can anyone help with merging it?
We could really use this.
I would love to help myself, I just don't know how :(

@tylersmith34

This comment has been minimized.

tylersmith34 commented Apr 3, 2018

@Eli-Goldberg @tecnobrat Yes they can be merged and submitted, but the problem is that there's no one managing this repo anymore to merge PRs.

@carlossg

This comment has been minimized.

Contributor

carlossg commented Apr 3, 2018

@pgarbe and Douglas Manley are now committers
https://issues.jenkins-ci.org/browse/INFRA-1501

so I suggest you just add 👍 to the issue if you have tried it so it can be merged with confidence sooner than later

@pgarbe

This comment has been minimized.

Contributor

pgarbe commented Apr 9, 2018

Hi @guilhermef, could you please rebase you fork to fix the conflicts?

@guilhermef

This comment has been minimized.

guilhermef commented Apr 10, 2018

Hey @pgarbe, I've rebased the PR.

@joshcurago

This comment has been minimized.

joshcurago commented Apr 27, 2018

Fargate added support for more regions!

https://aws.amazon.com/about-aws/whats-new/2018/04/aws-fargate-now-available-in-ohio--oregon--and-ireland-regions/

Would love to have this feature added in

@mhelgren

This comment has been minimized.

mhelgren commented May 5, 2018

So I'm also getting the error:
com.amazonaws.services.ecs.model.InvalidParameterException: Container instance cannot be empty. Looking at the code from master it keeps calling getEcsService().waitForSufficientClusterResources() which is blowing up because I dont have any dedicated container instances. From my reading of ECS fargate container instances are not necessary and are supposed to be dynamic - kind of the whole point :).
Forgive me if this fix/change is already in this PR, either way it would be nice to get this change into master.

@pgarbe

This comment has been minimized.

Contributor

pgarbe commented May 7, 2018

What is needed to support Fargate, if #45 or #50 is merged? As far as I understand, waitForSufficientClusterResources shouldn't be called. But why is this needed anyways?

@guilhermef

This comment has been minimized.

guilhermef commented May 7, 2018

waitForSufficientClusterResources isn't called on this PR, if it's a fargate container: https://github.com/jenkinsci/amazon-ecs-plugin/pull/51/files#diff-6dc5ac83fb2f7e028bd535ecaaf7c0d3R241

@Thadir

This comment has been minimized.

Thadir commented May 9, 2018

@pgarbe and @tekkamanendless what is the state of this pull request. We are using a beta build bc we want to use fare gate. But it would be great if it came from the plugin itself and not a side build.

@pgarbe

This comment has been minimized.

Contributor

pgarbe commented May 13, 2018

I've merged #50 before, to be able to re-use existing task definitions. That should make it easier and more flexible to support Fargate tasks. Can you please rebase again, @guilhermef ?

@guilhermef

This comment has been minimized.

guilhermef commented May 14, 2018

I've rebased it again.

@pgarbe pgarbe merged commit a1c7584 into jenkinsci:master May 14, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@ccelebi

This comment has been minimized.

ccelebi commented May 16, 2018

I believe #53 is still required. It's part of the task definition. I'm still in need of executionRoleArn in my task definition as I require ECR access. How else is anyone getting this to work without executionRoleArn?

@phr3nzii

This comment has been minimized.

phr3nzii commented May 21, 2018

What release should this be part of? I've installed the latest, 1.14, and the "Launch Type" field does not exist

@pgarbe

This comment has been minimized.

Contributor

pgarbe commented May 23, 2018

It's part of the upcoming v1.15. I didn't release it so far, as I found a bug, that it fails with a NullPointerException if the config does not contain any information about the new introduced launchType.
I tried to fix it here and here but it didn't work. Maybe @guilhermef can have a look?

@guilhermef

This comment has been minimized.

guilhermef commented May 23, 2018

@pgarbe, I'll try to fix it

@guilhermef

This comment has been minimized.

guilhermef commented May 23, 2018

@pgarbe, I've just tested the master and it worked.
Did you just update the plugin and used the old cloud configuration ?
The plugin sets EC2 as a default value for the slave template, so if you just save it again, it should work.

@pgarbe

This comment has been minimized.

Contributor

pgarbe commented May 23, 2018

When you open the configuration and click on save, everything work as expected. The problem comes when you update the plugin and the config.xml hasn't been updated.

@guilhermef

This comment has been minimized.

guilhermef commented May 23, 2018

#61 should fix it.

@pgarbe

This comment has been minimized.

Contributor

pgarbe commented May 23, 2018

looks good! preparing the release now

@squalou

This comment has been minimized.

squalou commented May 25, 2018

Hi guys,
I'm still facing this error : com.amazonaws.services.ecs.model.ClientException: Fargate requires task definition to have execution role ARN to support ECR images.

I'm using plugin version 1.15.
In configuration I can see a Task Role ARN, but no execution Role ARN (like appears on screenshot above in this conversation).

Is this expected ?

I've put really permissive role there to give it a try, to no avail : I always get this error. I'm somewhat lost, any clue from someone having configured it successfully with ECR access would be welcome !

@pgarbe

This comment has been minimized.

Contributor

pgarbe commented May 29, 2018

@squalou The configuration of the ecs plugin, does not support the execution role.

The better way is to create your own task definition (with AWS Console or CloudFormation). In this task definition you can also define your execution role. Take the arn of the task definition and put it in Task Definition Override. That's it.

@squalou

This comment has been minimized.

squalou commented May 29, 2018

Hi, thanx for the suggetsion but I much prefer the pull request suggested here :
#62
For 3 reasons
1 : it works like a charm !
2 : it's "closer" to the notions and exceptions being raised by AWS, which make it easier to configure and debug. I mean, the exception is basically "you need and execution role", not "you need to override a task definition". Doing what you suggest may work, but honestly by what miracle a user could guess it ? (not to be rude but, ... documentation is light, and the '?' button text near the field is not helpful). Having the execution role as a dedicated field is self-explicit.
3 : the whole point of the plugin is to work without having to deal with task definitions.

dgr237 pushed a commit to dgr237/amazon-ecs-plugin that referenced this pull request Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment