Skip to content

Conversation

ademidoff
Copy link
Member

@ademidoff ademidoff commented Aug 19, 2025

  • Refactored and improved the spot instance launch logic, including a new or updated runSpotInstance.groovy with significant optimizations, such as removal of the second volume which was not being mounted and used.
  • Adjusted pmm3-aws-staging-start.groovy with various fixes, cleanups, and improvements.
  • Removed the comment from pmm3-release-candidate.groovy.
  • Added or updated helper scripts: getPMMBuildParams.groovy (new), getSSHKeys.groovy (updated), and a minor fix in getSHHKeysPMM.groovy.
  • The changes include both code cleanups and functional improvements, especially around library usage and spot instance management.

@ademidoff ademidoff force-pushed the PMM-7-refactor-launch-spot-instance-fn branch 10 times, most recently from ce35638 to ccfe591 Compare August 19, 2025 22:11
@ademidoff ademidoff force-pushed the PMM-7-refactor-launch-spot-instance-fn branch from ccfe591 to fb4906a Compare August 19, 2025 22:19
@ademidoff ademidoff force-pushed the PMM-7-refactor-launch-spot-instance-fn branch 3 times, most recently from 935f364 to eaebd5f Compare August 21, 2025 09:13
@ademidoff ademidoff force-pushed the PMM-7-refactor-launch-spot-instance-fn branch from eaebd5f to bb25f9b Compare August 21, 2025 09:17
@ademidoff ademidoff marked this pull request as ready for review August 21, 2025 09:30
@ademidoff ademidoff requested review from puneet0191 and a team as code owners August 21, 2025 09:30
@nogueiraanderson
Copy link
Contributor

nogueiraanderson commented Aug 21, 2025

@ademidoff The DAYS variable is used in runSpotInstance.groovy at line 117 but it's not passed as a parameter to the function.

Key=stop-after-days,Value=${DAYS} ```

This variable appears to be expected from the calling context, but it would be better to pass it as an explicit parameter to the `runSpotInstance` function for clarity and to avoid potential undefined variable errors.

@nogueiraanderson
Copy link
Contributor

The library reference needs to be updated before merging:

library changelog: false, identifier: 'v3lib@PMM-7-refactor-launch-spot-instance-fn', retriever: modernSCM(

This should be changed back to 'v3lib@master' at line 6 in pmm/v3/pmm3-aws-staging-start.groovy before merging this PR, otherwise the pipeline will fail when the feature branch is deleted.

@ademidoff
Copy link
Member Author

The library reference needs to be updated before merging:

library changelog: false, identifier: 'v3lib@PMM-7-refactor-launch-spot-instance-fn', retriever: modernSCM(

This should be changed back to 'v3lib@master' at line 6 in pmm/v3/pmm3-aws-staging-start.groovy before merging this PR, otherwise the pipeline will fail when the feature branch is deleted.

Done.

@ademidoff
Copy link
Member Author

@ademidoff The DAYS variable is used in runSpotInstance.groovy at line 117 but it's not passed as a parameter to the function.

Key=stop-after-days,Value=${DAYS} ```

This variable appears to be expected from the calling context, but it would be better to pass it as an explicit parameter to the `runSpotInstance` function for clarity and to avoid potential undefined variable errors.

We have a number of global variables, that are necessary to run such pipelines - VM_NAME, DAYS, and OWNER. They are used in many places and therefore fixing the function signatures would be out of scope of this ticket.

I do agree that explicitly passing the variables is much easier to reason about and support, but that would be require us to make changes to almost every other pipeline and then test them all.

@ademidoff ademidoff requested review from Copilot and removed request for Copilot August 21, 2025 13:17
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