Restart preempted jobs #33

ingwarsw · 2019-03-01T14:20:11Z

This PR is "working prototype" for restarting jobs after being preempted by google.

Im testing in on our system for more than 2 months and its working, Its restarting all jobs that was preempted, but there are few things that should be done to make it production ready.

Missing features:

Pass parameters to restarted jobs
Work with other types of jobs (only pipeline tested)
Pass original owner of build, and add some Cause about restart
Add checkbox in GUI to actually dont restart failed jobs
Tests

I can finish it but maybe you have some ideas how to do some of needed things.

rachely3n

Looks good so far. I just had a few comments.
Also, can we have method headers and descriptions for the functions/classes? Will create less confusion down the line. I will need some time to think about the use case and what other features to include.

src/main/java/com/google/jenkins/plugins/computeengine/InstanceConfiguration.java

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineRetentionStrategy.java

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineComputer.java

rachely3n · 2019-03-07T23:56:17Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineComputer.java

+
+    boolean getPreempted() {
+        try {
+            return preemptedFuture != null && preemptedFuture.isDone() && preemptedFuture.get();


Can you explain this logic a bit further? Just want to make sure we are both thinking the same thing.

It basically checks if node was preempted :)

First we check if we have feature (if node is not preemptive we may not have it set)
Then we check if feature is done.
And if its done check if its true == preempted

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineComputerListener.java

rachely3n · 2019-03-07T23:57:30Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineInstance.java

@@ -89,13 +89,10 @@ protected void _terminate(TaskListener listener) throws IOException, Interrupted

            // If the instance is running, attempt to terminate it. This is an asynch call and we
            // return immediately, hoping for the best.
-            cloud.client.terminateInstanceWithStatus(cloud.projectId, zone, name, "RUNNING");
+            cloud.getClient().terminateInstance(cloud.getProjectId(), zone, name);


Can you explain this change?

It fixes issue with leaving behind preempted instances.

https://issues.jenkins-ci.org/browse/JENKINS-52736?focusedCommentId=356668&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-356668

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineInstance.java

src/main/java/com/google/jenkins/plugins/computeengine/client/ComputeClient.java

src/main/java/com/google/jenkins/plugins/computeengine/PreemptedCheckCallable.java

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineRetentionStrategy.java

craigdbarber · 2019-03-08T02:15:50Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineRetentionStrategy.java

+     * @param retentionTimeMinutes number of minutes of idleness after which to kill the slave; serves a backup in case the strategy fails to detect the end of a task
+     * @param oneShot              create one shot instance strategy
+     */
+    ComputeEngineRetentionStrategy(int retentionTimeMinutes, boolean oneShot) {


Any reason this is package scoped and not public?

It dont need to be public? :)
Any reason to make it public?

It's been the convention throughout the rest of the code base. That said, I don't have any strong opinions one way or the other :)

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineRetentionStrategy.java

src/main/java/com/google/jenkins/plugins/computeengine/PreemptedCheckCallable.java

craigdbarber · 2019-03-08T02:23:49Z

Thanks for digging in to this. Would like to see test coverage for this.

craigdbarber

A few inline comments.

…gle-compute-engine-plugin into pr-preemptive-merge

craigdbarber · 2019-03-21T17:28:05Z

src/main/java/com/google/jenkins/plugins/computeengine/PreemptedCheckCallable.java

+     */
+    @Override
+    public Boolean call() throws IOException {
+        HttpTransport transport = new NetHttpTransport();


This method's content should exist within a client abstraction similar to the compute client. Also, it's client should be created with the ClientFactory. This de-duplicates much of this logic, and ensures the user agent string for the plugin is preserved. Also, a generated java client lib for the API should be used if it's available.

But we will never use this methods in any different places..
So we dont need extra (and rather dont want) client for it..
And the smaller its the faster it works, cause everything here must be serialised and send to slave node (cause its called from slave itself)

I didnt found any lib for that purpose.. I tried but i failed.. if you know any let me know..

I checked with our libraries team, and there currently doesn't exist a client lib for this API. That said, it would still be preferable to maintain the consistency of client abstractions we've established throughout the code base, and move this API logic into its own client class. It allows for better testing and separation of concerns.

craigdbarber · 2019-03-21T17:30:12Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineRetentionStrategy.java

+ * - setting one shot instances {@link OnceRetentionStrategy}
+ * - in case of preemption of GCP instance to restart preempted tasks
+ */
+public class ComputeEngineRetentionStrategy extends RetentionStrategy<ComputeEngineComputer> implements ExecutorListener {


Thanks for adding this class. We'll need to make sure it has proper unit test coverage.

craigdbarber · 2019-03-21T17:33:18Z

src/main/java/com/google/jenkins/plugins/computeengine/ComputeEngineRetentionStrategy.java

+        return parent;
+    }
+
+    private void checkPreempted(Executor executor, Queue.Task task) {


This method seems to encapsulate two separate concerns: 1. Checking if a task was preempted 2. Rescheduling the task if 1. is true. Suggest separating the concerns for improved orthogonality and easier testing.

craigdbarber · 2019-04-03T17:01:32Z

Pinging this to make sure it doesn't get dropped. Please feel free to reach out if there are any questions regarding the feedback.

ingwarsw · 2019-04-14T17:55:28Z

@craigatgoogle I didnt forget, just dont have time to finish it..
And first need to close #54
We should talk on slack how we would like to do it..
At least few concepts here are really helpful..
And there few ones I have no idea how to finish, but even partially working they should be helpfull..

joshk0 · 2019-05-25T23:10:29Z

Looks like #54 is merged. How can we help with this change? I could take a whirl at resolving the conflicts if that helps.

ingwarsw · 2019-06-14T08:58:31Z

New PR covering same #113 ..

ingwarsw added 30 commits December 3, 2018 14:59

One shot instances

5496e06

Fix merge commit

c600518

Cleaning lost nodes job

8f877a5

Do not try stop already stopping instances

bdb265f

Dont kill terminated

6a7ccf8

Post review changes

e5545af

Merge remote-tracking branch 'jenkins/master' into clean_lost_nodes_pr

c20b914

Javadoc for added methods

ade6622

Merge remote-tracking branch 'jenkins/master' into pr-one-shot-squash

44b5b7f

Tests for One shot instances

f3a46f2

Fix all tests

2fa4322

After review changes

b265d5d

Merge branch 'clean_lost_nodes_pr' into develop

31d9d27

Delete preemptied instances

6aec866

Add restarting tasks in case of preemption

80c9be2

Not working version because of lags in operations

70ea6c5

tests

0219669

Semi working

cfd851b

Ugly but working

7cc3292

Almost almost

6708acf

Minor fixes

9a576d4

Terminating preempted jobs

ecf5483

Nimor fixes

31cd6e0

Merge remote-tracking branch 'jenkins/master' into pr-preemptive

7a2cc4d

Merge conflicts

029486e

Cleanup

bcbe1ed

Fixes after merge

376a1fa

Remove bit of old logs

b607d99

Merge remote-tracking branch 'jenkins/master' into pr-preemptive-merge

ba1a795

Fix mirror issues

3fb8957

rachely3n reviewed Mar 8, 2019

View reviewed changes