Skip to content

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

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

Merged
merged 5 commits into from
Jun 11, 2025

Conversation

pwo3
Copy link
Contributor

@pwo3 pwo3 commented May 14, 2025

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.

@pwo3 pwo3 requested a review from a team as a code owner May 14, 2025 09:35
@pwo3 pwo3 force-pushed the fix-spot-tag-on-fallback branch 3 times, most recently from 2af3700 to 272fce0 Compare May 15, 2025 13:15
@r-bk
Copy link

r-bk commented May 20, 2025

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

@npalm
Copy link
Member

npalm commented May 20, 2025

I will have a look at the PR asap

@pwo3 pwo3 requested a review from npalm May 23, 2025 08:36
Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pwo3 force-pushed the fix-spot-tag-on-fallback branch from 56799e4 to cd31b74 Compare May 26, 2025 07:39
@pwo3
Copy link
Contributor Author

pwo3 commented May 26, 2025

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

@pwo3 pwo3 requested a review from npalm May 26, 2025 07:41
@pwo3
Copy link
Contributor Author

pwo3 commented Jun 4, 2025

Hi @npalm, can we merge this PR if everything is OK with you? Thanks

Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply the change to the default example is trigger a change. This bcase the default for enable_on_demand_failover_for_errors is []. Can you include an empty array check in the if.

Sorro for the long response time on the PR

@pwo3 pwo3 requested a review from npalm June 11, 2025 07:23
@npalm npalm merged commit ab8b15f into github-aws-runners:main Jun 11, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants