Skip to content

Conversation

LaloBenitez
Copy link
Contributor

@LaloBenitez LaloBenitez commented Mar 18, 2025

Switching from using legacyInstanceId to the Jenkins controller URL for the jenkinsResourceTag value, similar to the ec2-plugin. This could be a good way to handle duplicate legacyInstanceIds. It seems like this change could benefit multiple people.

This came from an issue where the Cleanup task was deprovisioning VMs after exactly 2 hours, interrupting any on-going builds. This is because the cleanLeakedResouces() method looks for candidate nodes to removed based on legacyInstanceId.

Fixes #634

With this PR:

  • New Tags will be constructed using Jenkins URL <jenkinsURL>|<timestamp>
  • Cleanup tasks will look for this new format, and also old format <id>/<timestamp>

Testing done

  • Added unit tests for testing new format.
  • No evil Jenkins on mvn hpi:run.
  • Tested CleanupTask on old format: After building the plugin, I configured a Cloud, provisioned a node with a job, updated its tag in Azure to the 'id/ts' format (it correctly assigned the new "url|ts" format), and waited for the CleanupTask to remove it. The task successfully deleted the node.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@timja
Copy link
Member

timja commented Mar 18, 2025

Can you resolve conflicts please?

@LaloBenitez
Copy link
Contributor Author

Looking into the failing checks now.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Thanks looks great!

I've done a basic sanity test and it seems fine.

@timja timja merged commit b426077 into jenkinsci:master Mar 19, 2025
17 checks passed
@LaloBenitez
Copy link
Contributor Author

@timja Hey, I see the PR got merged, but there are some issues on the master branch. Are these stopping the new release? How can I help fix them?

@timja
Copy link
Member

timja commented Mar 20, 2025

I've triggered another build lets see if that works

@timja
Copy link
Member

timja commented Mar 20, 2025

@LaloBenitez LaloBenitez deleted the itef7u8_use_url_for_id branch March 20, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VMs getting terminated mid-build after exactly 2 hours - Same instance ID on multiple controllers
2 participants