[JENKINS-59180] Allow AMI filtering instead of AMI ID #533

dbnicholson · 2020-11-16T22:37:43Z

Allow finding the image to launch by the various describeImages criteria instead of just a specific image ID. Currently you're required to reconfigure the plugin whenever the image is updated, which is inconvenient.

Fixes: https://issues.jenkins.io/browse/JENKINS-59180

In order to make the image ID determined by properties, it can no longer be a static string. In that case, including it in the class representation would require that it be queried or supplied by the caller. The AMI can be seen in the logs at other times, so just use the description instead.

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

froblesmartin

LGTM but I am still pretty new. Would like a review from @res0nance @fcojfernandez or @MRamonLeon

fcojfernandez

See my comment about changing a signature of a non-private method

Besides, if this PR intends to fix https://issues.jenkins.io/browse/JENKINS-59180, shouldn't be the ticket assigned to you and in progress?

src/main/java/hudson/plugins/ec2/EC2Filter.java

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

dbnicholson · 2020-11-18T16:16:15Z

Besides, if this PR intends to fix https://issues.jenkins.io/browse/JENKINS-59180, shouldn't be the ticket assigned to you and in progress?

I didn't think I could do anything more than make comments on there as an outside contributor. I've changed that now.

Pass the resolved image throughout the provisioning steps rather then querying AWS repeatedly. This has 2 benefits: * Speed up the process by redundant removing round trips to AWS * If the image ID comes from a filtered query rather than being static, it ensures that all of the provisioning steps are operating on the same image.

In order to make the AWS Filter class available in configurations, add a wrapper EC2Filter class that implements Descriptor. Ideally the values member would be `List<String>`, but currently jenkins doesn't have a usable way to offer those in forms. Instead the values are interpreted as a whitespace separated list.

Currently whenever the image is updated the template configuration must also be updated. Rather than requiring the AMI ID to be specified, allow providing filters that can be passed to `describeImages` so that the latest image matching some attributes can be used.

fcojfernandez

Thank you very much for your contribution @dbnicholson

I didn't think I could do anything more than make comments on there as an outside contributor. I've changed that now.

Please, feel free and assign any open ticket you decide to work on. That way you will let others
know you are on it.

And let me another piece of advice: try to avoid the force-push command as much as you can. It makes harder review the new changes since the force-push hides them. There are even reviewers that stop reviewing the PRs after a force-push :(

@froblesmartin my concerns are addressed, so I'm approving the PR. I'll let you merge it just in case you want to have a second look at the latest changes.

res0nance

Untested with an aws account but overall looks good

res0nance · 2020-11-19T09:27:14Z

src/main/java/hudson/plugins/ec2/SlaveTemplate.java


-        throw new AmazonClientException("Unable to find AMI " + ami);
+        // Sort in reverse by creation date to get latest image


We should document this behaviour the the help text

It does say that in the form help for each field, but maybe you mean somewhere else?

dbnicholson · 2020-11-19T13:45:22Z

And let me another piece of advice: try to avoid the force-push command as much as you can. It makes harder review the new changes since the force-push hides them. There are even reviewers that stop reviewing the PRs after a force-push :(

It's unfortunately the only way to properly maintain commit history on GitHub. That or someone applies the fixups before merging. Do you have something that does that? Or maybe you do squashed merges? I'm fine following whatever flow you normally use. There is a link on the text that says force push that shows the differences from the previous state.

froblesmartin · 2020-11-20T11:54:15Z

@dbnicholson is there anything else you want to change before merging, or may I merge already? 😉

dbnicholson · 2020-11-20T13:33:08Z

Nothing more from me. Go ahead and merge 😺

aviadcye · 2020-12-30T13:16:14Z

Hi,
Sorry, but no matter what I do, I can't seem to get this new configuration to work. I always get the error "The request must contain the parameter ImageId". However if I set the ImageId, all other filters are ignored... Am I missing something?

Steps to reproduce.

Retrofitting an existing AMI configuration that uses a specific ImageId.
Removed the "AMI ID" set, so the filed is blank.
Set "AMI Owners" & "AMI Users" to self (as per example test in code).
Set "AMI Filters", Added just "Name" (also tried all lower case) with the value of the "Jenkins_docker_agent_base_-_7.12.20a". Also tired with "Jenkins_docker_agent_base*".
Click on "Check AMI".

Thank you, Aviad.

dbnicholson · 2020-12-30T13:57:26Z

Does your AWS account have an AMI named Jenkins_docker_agent_base_-_7.12.20a? Using an owner of self says your account is the owner of the AMI.

dbnicholson · 2020-12-30T13:58:56Z

Also, if you look in the logs, you should see the results of the DescribeImages call.

aviadcye · 2020-12-30T14:13:33Z

Does your AWS account have an AMI named Jenkins_docker_agent_base_-_7.12.20a? Using an owner of self says your account is the owner of the AMI.

Yes i have this AMI in my account.

Also, if you look in the logs, you should see the results of the DescribeImages call.

So when I use "Name" i get the error "The filter 'Name' is invalid" (the tag key is "Name", so I don't get why... but OK...
So i tried using "name", and I just didn't get a log entry.

dbnicholson · 2020-12-30T14:39:13Z

name and a tag named Name are different things. If you want to filter on a Name tag, you have to use a filter of tag:Name. This is in the AWS documentation linked to in the help text.

aviadcye · 2020-12-30T14:42:32Z

I tried that as well... didn't work... :/ Btw, I think an example in the plugin context documentation before linking to the aws docs would help...

…

On Wed, Dec 30, 2020, 16:39 Dan Nicholson ***@***.***> wrote: name and a tag named Name are different things. If you want to filter on a Name tag, you have to use a filter of tag:Name. This is in the AWS documentation linked to in the help text. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#533 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANUS57PUYTEJGYVTPZDNV3DSXM3R7ANCNFSM4TXZGD4Q> .

jenkinscye · 2020-12-31T07:11:31Z

No rush, but any thoughts?
Thank you.

dbnicholson · 2020-12-31T13:48:06Z

Can you post the logs with the DescribeImages request and response? Have you tried looking up this combination with some like awscli?

aviadcye · 2020-12-31T14:16:17Z

As I said, when I use the "check ami" button, there are no logs unless it has a filter error, and I'm not even sure ab that. I'll try to create the definition, and simply attempt to create the agent... maybe that will give logs. About awscli, ill try that as soon as possible and report back.

…

On Thu, Dec 31, 2020, 15:48 Dan Nicholson ***@***.***> wrote: Can you post the logs with the DescribeImages request and response? Have you tried looking up this combination with some like awscli? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#533 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANUS57NPKLZFQ3RGERJFVIDSXR6KFANCNFSM4TXZGD4Q> .

dbnicholson · 2020-12-31T15:06:37Z

Sorry I missed that you were using the Check AMI button. The Check AMI button only works if you supply an image ID due to limitations in Jenkins. I thought that was clear that the button was not relevant when using filters.

aviadcye · 2020-12-31T15:15:04Z

Well this is interesting...
Even though the "Check AMI" button, still doesn't work as expected, trying to spawn an agent, worked on the first go...
That is, with simply leaving the imageid blank and adding the filter of "tag:Name = Jenkins_docker_agent_base", I was able to create an agent (logs below).

So this is just a lacking in the check AMI button functionality.
IMHO, it should both be able to do the same as spawning an agent can, and provide logs... because I gather now that the log I got about the filter "Name" no being valid, was after I had saved the configuration.

At any rate, this is totally usable as it is right now.
Thank you for an awesome feature, I'll start using it immediately :)

2020-12-31 15:01:36.746+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#getImage: Getting image for request {ExecutableUsers: [],Filters: [{Name: tag:Name,Values: [Jenkins_docker_agent_base*]}],ImageIds: [],Owners: []} 2020-12-31 15:01:37.557+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Considering launching 2020-12-31 15:01:37.557+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Setting Instance Initiated Shutdown Behavior : ShutdownBehavior.Terminate 2020-12-31 15:01:37.728+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Looking for existing instances with describe-instance: {Filters: [{Name: image-id,Values: [ami-yepyepeyep]}, {Name: instance-type,Values: [t1.micro]}, {Name: key-name,Values: [jenkins-slaves]}, {Name: tag:jenkins_server_url,Values: [https://jenkins.dev.example.com/]}, {Name: tag:jenkins_slave_type,Values: [demand_test]}],InstanceIds: [],} 2020-12-31 15:01:39.154+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Return instance: {AmiLaunchIndex: 0,ImageId: ami-yepyepeyep,InstanceId: i-idididid,InstanceType: t1.micro,KeyName: jenkins-slaves,LaunchTime: Thu Dec 31 15:01:38 GMT 2020,Monitoring: {State: disabled},Placement: {AvailabilityZone: eu-west-1a,GroupName: ,Tenancy: default,},PrivateDnsName: ip-172-31-19-43.eu-west-1.compute.internal,PrivateIpAddress: 172.31.19.43,ProductCodes: [],PublicDnsName: ,State: {Code: 0,Name: pending},StateTransitionReason: ,SubnetId: subnet-09271c40,VpcId: vpc-031c1064,Architecture: x86_64,BlockDeviceMappings: [],ClientToken: 59eb1abe-d117-4183-bfff-b674b60fb26f,EbsOptimized: false,EnaSupport: true,Hypervisor: xen,ElasticGpuAssociations: [],ElasticInferenceAcceleratorAssociations: [],NetworkInterfaces: [{Attachment: {AttachTime: Thu Dec 31 15:01:38 GMT 2020,AttachmentId: eni-attach-045b1c07fe3064f89,DeleteOnTermination: true,DeviceIndex: 0,Status: attaching},Description: ,Groups: [{GroupName: default,GroupId: sg-c86505b3}],Ipv6Addresses: [],MacAddress: 06:54:38:0d:a6:0f,NetworkInterfaceId: eni-0e558a4d70a8bd16f,OwnerId: actactact,PrivateDnsName: ip-172-31-19-43.eu-west-1.compute.internal,PrivateIpAddress: 172.31.19.43,PrivateIpAddresses: [{Primary: true,PrivateDnsName: ip-172-31-19-43.eu-west-1.compute.internal,PrivateIpAddress: 172.31.19.43}],SourceDestCheck: true,Status: in-use,SubnetId: subnet-09271c40,VpcId: vpc-031c1064,InterfaceType: interface}],RootDeviceName: /dev/sda1,RootDeviceType: ebs,SecurityGroups: [{GroupName: default,GroupId: sg-c86505b3}],SourceDestCheck: true,StateReason: {Code: pending,Message: pending},Tags: [{Key: jenkins_slave_type,Value: demand_test}, {Key: jenkins_server_url,Value: https://jenkins.dev.example.com/}],VirtualizationType: hvm,CpuOptions: {CoreCount: 1,ThreadsPerCore: 1},CapacityReservationSpecification: {CapacityReservationPreference: open,},Licenses: [],MetadataOptions: {State: pending,HttpTokens: optional,HttpPutResponseHopLimit: 1,HttpEndpoint: enabled}} 2020-12-31 15:01:39.249+0000 [id=140760] INFO h.p.ec2.EC2RetentionStrategy#start: Start requested for EC2 (AWS-connector--eu-west-1(Ireland)((DEV)) - test (i-idididid) 2020-12-31 15:01:39.249+0000 [id=151193] INFO hudson.plugins.ec2.EC2Cloud#log: Launching instance: i-idididid

jenkinscye · 2020-12-31T15:24:59Z

Not clear enough apparently :/ Sorry

…

On Thu, Dec 31, 2020, 17:15 'aviadcye' via CYE_DevOps ***@***.***> wrote: Well this is interesting... Even though the "Check AMI" button, still doesn't work as expected, trying to spawn an agent, worked on the first go... That is, with simply leaving the imageid blank and adding the filter of "tag:Name = Jenkins_docker_agent_base", I was able to create an agent (logs below). So this is just a lacking in the check AMI button functionality. IMHO, it should both be able to do the same as spawning an agent can, and provide logs... because I gather now that the log I got about the filter "Name" no being valid, was after I had saved the configuration. At any rate, this is totally usable as it is right now. Thank you for an awesome feature, I'll start using it immediately :) 2020-12-31 15:01:36.746+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#getImage: Getting image for request {ExecutableUsers: [],Filters: [{Name: tag:Name,Values: [Jenkins_docker_agent_base*]}],ImageIds: [],Owners: []} 2020-12-31 15:01:37.557+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Considering launching 2020-12-31 15:01:37.557+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Setting Instance Initiated Shutdown Behavior : ShutdownBehavior.Terminate 2020-12-31 15:01:37.728+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Looking for existing instances with describe-instance: {Filters: [{Name: image-id,Values: [ami-yepyepeyep]}, {Name: instance-type,Values: [t1.micro]}, {Name: key-name,Values: [jenkins-slaves]}, {Name: tag:jenkins_server_url,Values: [ https://jenkins.dev.example.com/]}, {Name: tag:jenkins_slave_type,Values: [demand_test]}],InstanceIds: [],} 2020-12-31 15:01:39.154+0000 [id=140760] INFO hudson.plugins.ec2.SlaveTemplate#logProvisionInfo: SlaveTemplate{description='test', labels=''}. Return instance: {AmiLaunchIndex: 0,ImageId: ami-yepyepeyep,InstanceId: i-idididid,InstanceType: t1.micro,KeyName: jenkins-slaves,LaunchTime: Thu Dec 31 15:01:38 GMT 2020,Monitoring: {State: disabled},Placement: {AvailabilityZone: eu-west-1a,GroupName: ,Tenancy: default,},PrivateDnsName: ip-172-31-19-43.eu-west-1.compute.internal,PrivateIpAddress: 172.31.19.43,ProductCodes: [],PublicDnsName: ,State: {Code: 0,Name: pending},StateTransitionReason: ,SubnetId: subnet-09271c40,VpcId: vpc-031c1064,Architecture: x86_64,BlockDeviceMappings: [],ClientToken: 59eb1abe-d117-4183-bfff-b674b60fb26f,EbsOptimized: false,EnaSupport: true,Hypervisor: xen,ElasticGpuAssociations: [],ElasticInferenceAcceleratorAssociations: [],NetworkInterfaces: [{Attachment: {AttachTime: Thu Dec 31 15:01:38 GMT 2020,AttachmentId: eni-attach-045b1c07fe3064f89,DeleteOnTermination: true,DeviceIndex: 0,Status: attaching},Description: ,Groups: [{GroupName: default,GroupId: sg-c86505b3}],Ipv6Addresses: [],MacAddress: 06:54:38:0d:a6:0f,NetworkInterfaceId: eni-0e558a4d70a8bd16f,OwnerId: actactact,PrivateDnsName: ip-172-31-19-43.eu-west-1.compute.internal,PrivateIpAddress: 172.31.19.43,PrivateIpAddresses: [{Primary: true,PrivateDnsName: ip-172-31-19-43.eu-west-1.compute.internal,PrivateIpAddress: 172.31.19.43}],SourceDestCheck: true,Status: in-use,SubnetId: subnet-09271c40,VpcId: vpc-031c1064,InterfaceType: interface}],RootDeviceName: /dev/sda1,RootDeviceType: ebs,SecurityGroups: [{GroupName: default,GroupId: sg-c86505b3}],SourceDestCheck: true,StateReason: {Code: pending,Message: pending},Tags: [{Key: jenkins_slave_type,Value: demand_test}, {Key: jenkins_server_url,Value: https://jenkins.dev.example.com/}],VirtualizationType: hvm,CpuOptions: {CoreCount: 1,ThreadsPerCore: 1},CapacityReservationSpecification: {CapacityReservationPreference: open,},Licenses: [],MetadataOptions: {State: pending,HttpTokens: optional,HttpPutResponseHopLimit: 1,HttpEndpoint: enabled}} 2020-12-31 15:01:39.249+0000 [id=140760] INFO h.p.ec2.EC2RetentionStrategy#start: Start requested for EC2 (AWS-connector--eu-west-1(Ireland)((DEV)) - test (i-idididid) 2020-12-31 15:01:39.249+0000 [id=151193] INFO hudson.plugins.ec2.EC2Cloud#log: Launching instance: i-idididid — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#533 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOKUJ6NX7IPQQOSBNU34Z3TSXSIQLANCNFSM4TXZGD4Q> .

froblesmartin requested changes Nov 17, 2020

View reviewed changes

src/main/java/hudson/plugins/ec2/SlaveTemplate.java Outdated Show resolved Hide resolved

dbnicholson force-pushed the ami-filter branch from eb2d8b3 to e721dd1 Compare November 17, 2020 13:30

dbnicholson requested a review from froblesmartin November 17, 2020 13:31

froblesmartin approved these changes Nov 17, 2020

View reviewed changes

fcojfernandez requested changes Nov 18, 2020

View reviewed changes

dbnicholson added 3 commits November 18, 2020 09:45

dbnicholson force-pushed the ami-filter branch from e721dd1 to 759e1fd Compare November 18, 2020 20:11

dbnicholson requested a review from fcojfernandez November 18, 2020 20:11

fcojfernandez approved these changes Nov 19, 2020

View reviewed changes

res0nance approved these changes Nov 19, 2020

View reviewed changes

res0nance added the enhancement Feature additions or enhancements label Nov 19, 2020

froblesmartin merged commit 30ca96f into jenkinsci:master Nov 20, 2020

dbnicholson deleted the ami-filter branch November 23, 2020 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-59180] Allow AMI filtering instead of AMI ID #533

[JENKINS-59180] Allow AMI filtering instead of AMI ID #533

dbnicholson commented Nov 16, 2020

froblesmartin left a comment

fcojfernandez left a comment

dbnicholson commented Nov 18, 2020

fcojfernandez left a comment

res0nance left a comment

res0nance Nov 19, 2020

dbnicholson Nov 19, 2020

dbnicholson commented Nov 19, 2020

froblesmartin commented Nov 20, 2020

dbnicholson commented Nov 20, 2020

aviadcye commented Dec 30, 2020

dbnicholson commented Dec 30, 2020

dbnicholson commented Dec 30, 2020

aviadcye commented Dec 30, 2020

dbnicholson commented Dec 30, 2020

aviadcye commented Dec 30, 2020 via email

jenkinscye commented Dec 31, 2020

dbnicholson commented Dec 31, 2020

aviadcye commented Dec 31, 2020 via email

dbnicholson commented Dec 31, 2020 •

edited

Loading

aviadcye commented Dec 31, 2020

jenkinscye commented Dec 31, 2020 via email


		throw new AmazonClientException("Unable to find AMI " + ami);
		// Sort in reverse by creation date to get latest image

[JENKINS-59180] Allow AMI filtering instead of AMI ID #533

[JENKINS-59180] Allow AMI filtering instead of AMI ID #533

Conversation

dbnicholson commented Nov 16, 2020

froblesmartin left a comment

Choose a reason for hiding this comment

fcojfernandez left a comment

Choose a reason for hiding this comment

dbnicholson commented Nov 18, 2020

fcojfernandez left a comment

Choose a reason for hiding this comment

res0nance left a comment

Choose a reason for hiding this comment

res0nance Nov 19, 2020

Choose a reason for hiding this comment

dbnicholson Nov 19, 2020

Choose a reason for hiding this comment

dbnicholson commented Nov 19, 2020

froblesmartin commented Nov 20, 2020

dbnicholson commented Nov 20, 2020

aviadcye commented Dec 30, 2020

dbnicholson commented Dec 30, 2020

dbnicholson commented Dec 30, 2020

aviadcye commented Dec 30, 2020

dbnicholson commented Dec 30, 2020

aviadcye commented Dec 30, 2020 via email

jenkinscye commented Dec 31, 2020

dbnicholson commented Dec 31, 2020

aviadcye commented Dec 31, 2020 via email

dbnicholson commented Dec 31, 2020 • edited Loading

aviadcye commented Dec 31, 2020

jenkinscye commented Dec 31, 2020 via email

dbnicholson commented Dec 31, 2020 •

edited

Loading