fix: only tag spot requests if no on-demand fallback

[pwo3]

Hi,

This PR prevents tagging spot instance requests when an on-demand fallback is configured.

It addresses my comment on this issue.

The previous fix was working only if the instance_target_capacity_type is set to on-demand but not in case of on-demand fallback from a spot request.

This approach isn’t ideal but I didn’t find a cleaner way to cancel the tagging directly in lambdas/functions/control-plane/src/aws/runners.ts when the on-demand fallback is triggered.

[npalm]

This will solve the problem, but will avoid to tag the spot request if ondeamdn failover is active. A better place would be the lambda in my point of view.

const instancesOnDemand = await createRunner({

Would you have time to provide a fix in the lambda?

[pwo3]

That was my initial approach but I couldn't find a way to overwrite the tags directly in the lambda.

I'll take a second look to see if I can find a solution

[pwo3]

I don’t think we can apply a simple fix on the Lambda

Since the tags are defined in the launch template, it’s not possible to override them using the CreateFleetCommand

We could remove the spot-instances-request tags from the launch template and set them only in the Lambda’s CreateFleetCommand, but in that case, we lose access to the tags defined in the Terraform configuration

I don’t see a simple solution for now, do you have any thoughts on this?

Thanks

[r-bk]

I arrived to the same conclusion when I investigated a workaround.

[npalm]

I think we should consider to move the tagging spot request the runner.ts instead of setting it in the template. In that case we can do it properly based if a spot instance is requested or not. Already some tags are set here. Would you like to give it a shot?

see

terraform-aws-github-runner/lambdas/functions/control-plane/src/aws/runners.ts

Lines 272 to 283 in 57fce77

	TagSpecifications: [
	{
	ResourceType: 'instance',
	Tags: tags,
	},
	{
	ResourceType: 'volume',
	Tags: tags,
	},
	],
	Type: 'instant',
	});

[pwo3]

@npalm I tried this approach, but we would lose all the tags defined in the Terraform module, only those tags would be applied

  const tags = [
    { Key: 'ghr:Application', Value: 'github-action-runner' },
    { Key: 'ghr:created_by', Value: runnerParameters.numberOfRunners === 1 ? 'scale-up-lambda' : 'pool-lambda' },
    { Key: 'ghr:Type', Value: runnerParameters.runnerType },
    { Key: 'ghr:Owner', Value: runnerParameters.runnerOwner },
  ];

However, I can try fetching the tags from the launch template first and then applying them to the spot-instances-request via the TagSpecifications

[pwo3]

I pushed the changes, but I’m not really sure how to test it in real conditions

[npalm]

I will run some tests to check spot request are correctly tagged. The case spot is not available is not testable as far I know.

[r-bk]

@pwo3 @npalm
Any updates on this PR?
Are there any outstanding blockers to merge it?

[npalm]

I will have a look at the PR asap

[npalm]

I have tested the PR, the code was not working do to the falt map was creating duplicated tags and missing describe permission for the lanunch template.

After making some changes the lambda was working. However no tags on the spot request. Changed several things. But did not got it working at all. Really strange, debug showed clearly the correct elements in the TagSpecification.

After updating to main, I got my tag on the spot request back. Maybe we should revert back to the previous approach. And make a not in the terraform code tht this should not be the place but tagging via the spotfleetrequest was not working at all.

[pwo3]

I have tested the PR, the code was not working do to the falt map was creating duplicated tags and missing describe permission for the lanunch template.

After making some changes the lambda was working. However no tags on the spot request. Changed several things. But did not got it working at all. Really strange, debug showed clearly the correct elements in the TagSpecification.

After updating to main, I got my tag on the spot request back. Maybe we should revert back to the previous approach. And make a not in the terraform code tht this should not be the place but tagging via the spotfleetrequest was not working at all.

Thanks for your tests @npalm, too bad it's not working... I reverted to the previous fix and added a comment in the code to explain why we're using this approach