-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add spotVM and maxRunDuration feature to VM provisioning #492
base: develop
Are you sure you want to change the base?
Add spotVM and maxRunDuration feature to VM provisioning #492
Conversation
src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java
Outdated
Show resolved
Hide resolved
...es/com/google/jenkins/plugins/computeengine/InstanceConfiguration/help-provisioningType.html
Outdated
Show resolved
Hide resolved
src/main/resources/com/google/jenkins/plugins/computeengine/InstanceConfiguration/config.jelly
Outdated
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/SpotVm.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/SpotVm.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/Standard.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/Standard.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/Utils.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/Utils.java
Fixed
Show fixed
Hide fixed
recording and screenshots moved to the summary at the top ⬆️ |
I will work on the testing as,
Edit: all these points are completed ✔️ (automated tests are written both unittests and integration test). When maxRunDuration deletes the VM in GCP side, the controller also deletes the slave, which is correct no issues for builds ✅ (already covered in the integration test) |
@@ -58,7 +58,7 @@ public static void teardown() throws IOException { | |||
|
|||
@Test | |||
public void testGetImage() throws Exception { | |||
Image image = client.getImage("debian-cloud", "debian-9-stretch-v20180820"); | |||
Image image = client.getImage("debian-cloud", "debian-12-bookworm-v20241210"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debian-9
is already deprecated, this is throwing 404 image not found errors,
➜ ~ gcloud compute images list --project=debian-cloud --no-standard-images --show-deprecated | grep debian-9-stretch-v20180820
debian-9-stretch-v20180820 debian-cloud debian-9 DEPRECATED READY
? String.format( | ||
"projects/%s/global/images/%s", BOOT_DISK_PROJECT_ID, System.getenv("GOOGLE_BOOT_DISK_IMAGE_NAME")) | ||
: "projects/debian-cloud/global/images/family/debian-9"; | ||
: "projects/debian-cloud/global/images/family/debian-12"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to support the GOOGLE_BOOT_DISK_PROJECT_ID
and GOOGLE_BOOT_DISK_IMAGE_NAME
even for linux too, because the base debian images don't have java installed.
See the new document file being added in this PR integration-tests.md
...n/resources/com/google/jenkins/plugins/computeengine/ui/helpers/Standard/config-detail.jelly
Outdated
Show resolved
Hide resolved
…plugin into add-max-run-duration-to-vm-provisioning
} | ||
return null; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test credentials are created using this constructor, where it is passing an empty id
.
https://github.com/jenkinsci/google-oauth-plugin/blob/5367a8a1b9c97e14a5188150f897fa91da1d9777/src/main/java/com/google/jenkins/plugins/credentials/oauth/GoogleRobotPrivateKeyCredentials.java#L65-L70
which ends up at https://github.com/jenkinsci/credentials-plugin/blob/1e306e8f6c54f8eeef973b3387b8c223bd60ae1c/src/main/java/com/cloudbees/plugins/credentials/common/IdCredentials.java#L87-L96
thus having a random uuid.
seems like at somepoint the google-credential-plugin might have got this behavior (or sending blank id, instead of projectid).
As the integration tests in this repository are not run often, this never got brought up..
👋 Completed the PR from draft to ready, plz help review @jglick , @Vlatombe , @nevingeorgesunny |
(@Artmorse has also worked on this plugin recently.) |
...m/google/jenkins/plugins/computeengine/integration/ComputeEngineCloudRestartPreemptedIT.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/ProvisioningTypeValue.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/jenkins/plugins/computeengine/ConfigAsCodeTest.java
Outdated
Show resolved
Hide resolved
...m/google/jenkins/plugins/computeengine/integration/ComputeEngineCloudRestartPreemptedIT.java
Outdated
Show resolved
Hide resolved
…eConfiguration.java Co-authored-by: Vincent Latombe <vincent@latombe.net>
…eConfiguration.java Co-authored-by: Vincent Latombe <vincent@latombe.net>
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/SpotVm.java
Outdated
Show resolved
Hide resolved
import org.jvnet.hudson.test.JenkinsRule; | ||
import org.jvnet.hudson.test.recipes.WithTimeout; | ||
|
||
/** | ||
* Integration test suite for {@link ComputeEngineCloud}. Verifies that instances with preempted | ||
* flag will be restarted when preempted. | ||
*/ | ||
@Log | ||
public class ComputeEngineCloudRestartPreemptedIT { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there is more existing integration tests that are failing because of reasons like,
- they are depending on the debian-9 image (in the casc bundle the image path is there like that)
- and they are depending the JDK8 whose deb repository is not existing and giving 404 errors
etc like that..
So I will create a separate PR and fix all the integration tests. I will remove the existing integration test fixing code from here.
Once I file the other PR, I will link back in this comment's follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integration test update PR is created here
#495
--> | ||
<div> | ||
The maximum duration (in seconds) after which the VM will be automatically deleted by GCP. | ||
See <a href="https://cloud.google.com/compute/docs/instances/limit-vm-runtime">Limit the run time of a VM</a> for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, links in help files for config forms automatically open in a new tab/window as per some magic in Jenkins core—there is no need to manually right-click and select Open in new tab or similar.
...le/jenkins/plugins/computeengine/integration/SpotVmProvisioningWithMaxRunDurationCasCIT.java
Outdated
Show resolved
Hide resolved
...le/jenkins/plugins/computeengine/integration/SpotVmProvisioningWithMaxRunDurationCasCIT.java
Outdated
Show resolved
Hide resolved
docs/integration-tests.md
Outdated
``` | ||
* Execute an integration test (example) | ||
```bash | ||
mvn clean test -Dtest=ComputeEngineCloudRestartPreemptedIT#testIfNodeWasPreempted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that instead of the Failsafe plugin and the *IT.java
scheme, some plugins just use Surefire and use Assume
to automatically skip tests requiring some environmental setup that has not been provided.
src/main/java/com/google/jenkins/plugins/computeengine/ui/helpers/PreemptibleVm.java
Outdated
Show resolved
Hide resolved
@@ -120,7 +124,7 @@ public class InstanceConfiguration implements Describable<InstanceConfiguration> | |||
private String machineType; | |||
private String numExecutorsStr; | |||
private String startupScript; | |||
private boolean preemptible; | |||
private ProvisioningType provisioningType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: this class is using Lombok (not a good idea!):
Lines 88 to 89 in 8d7f256
@Getter | |
@Setter(onMethod = @__(@DataBoundSetter)) |
src/main/java/com/google/jenkins/plugins/computeengine/configs/SpotVm.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/configs/SpotVm.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/configs/Standard.java
Fixed
Show fixed
Hide fixed
src/main/java/com/google/jenkins/plugins/computeengine/configs/Standard.java
Fixed
Show fixed
Hide fixed
} | ||
|
||
@SuppressWarnings("unused") // jelly | ||
public FormValidation doCheckMaxRunDurationSeconds(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing POST/RequirePOST annotation Warning
} | ||
|
||
@SuppressWarnings("unused") // jelly | ||
public FormValidation doCheckMaxRunDurationSeconds(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing permission check Warning
} | ||
|
||
@SuppressWarnings("unused") // jelly | ||
public FormValidation doCheckMaxRunDurationSeconds(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing POST/RequirePOST annotation Warning
} | ||
|
||
@SuppressWarnings("unused") // jelly | ||
public FormValidation doCheckMaxRunDurationSeconds(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing permission check Warning
<groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
<artifactId>workflow-job</artifactId> | ||
<scope>test</scope> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These pom dependencies will be removed after merging #495
currently keeping here, otherwise compilation will fail in CI
Summary
This PR aims to enhance the VM provisioning capabilities of the
google-compute-engine-plugin
by introducing support for Spot VMs andmaxRunDuration
. This effort seeks to address the issues reported in #408, #473, and #358.Enhancements
maxRunDuration
feature, enabling the automatic deletion of VMs after a specified duration to optimize resource utilization and cost.Rationale
Although this plugin already has a background cleanup job for leftover orphan agent VMs, setting the
maxRunDuration
ensures that VMs deletion still happens even if the Jenkins controller itself has crashed. Besides this is an optional setting with no defaults enforced, so this feature is not trying to replace/interfere with the existing background job.Technical Background
The plugin now supports the following VM options:
maxRunDuration
.Why Maintain Support for Both Spot and Preemptible VMs?
Despite the similarities between Spot and Preemptible VMs, retaining support for both options is essential for the following reasons:
Testing Status
Manual Tests: Screen recording and screenshots
User experience configuration page
provisioning_type_impl_2.mov
VM Provisioned in GCP
→ Unit tests for the
scheduling()
method, ensuring the compatibility with the old and new configurations.New integration test for the Spot VM with Max Duration: logs
(cloudbees internal ticket link for back reference)
Submitter checklist