From 79bf1bc606130fd4647007271cf9b1d3388fa723 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 12:01:36 -0400 Subject: [PATCH 1/8] Avoid printing stack traces during normal test operation. --- .../artifact_manager_jclouds/MockBlobStore.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java index 673511b5..9503dd29 100644 --- a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java +++ b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java @@ -34,6 +34,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.commons.io.IOUtils; +import org.apache.http.ConnectionClosedException; import org.apache.http.HttpEntity; import org.apache.http.HttpEntityEnclosingRequest; import org.apache.http.HttpRequest; @@ -101,7 +102,7 @@ public synchronized BlobStoreContext getContext() throws IOException { Integer failure = fails.remove(method + ":" + key); if (failure != null) { if (failure == 0) { - throw new IllegalStateException("Refusing to even send a status code for " + container + ":" + key); + throw new ConnectionClosedException("Refusing to even send a status code for " + container + ":" + key); } response.setStatusLine(new BasicStatusLine(HttpVersion.HTTP_1_0, failure, "simulated " + failure + " failure")); response.setEntity(new StringEntity("Detailed explanation of " + failure + ".")); @@ -136,7 +137,13 @@ public synchronized BlobStoreContext getContext() throws IOException { } } }). - setExceptionLogger(x -> LOGGER.log(Level.INFO, "error thrown in HTTP service", x)). + setExceptionLogger(x -> { + if (x instanceof ConnectionClosedException) { + LOGGER.info(x.toString()); + } else { + LOGGER.log(Level.INFO, "error thrown in HTTP service", x); + } + }). create(); server.start(); baseURL = new URL("http://" + server.getInetAddress().getHostName() + ":" + server.getLocalPort() + "/"); From 5ed1dc562bb3614addec1b30095994fabf369e0a Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 12:05:49 -0400 Subject: [PATCH 2/8] Do not print stack traces merely due to failing HTTP response codes. --- .../JCloudsArtifactManager.java | 13 ++++++++++--- .../artifact_manager_jclouds/NetworkTest.java | 3 +++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java index a542a6ef..95109f2e 100644 --- a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java +++ b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java @@ -74,7 +74,6 @@ import jenkins.model.ArtifactManager; import jenkins.util.VirtualFile; import org.apache.commons.io.IOUtils; -import org.apache.http.client.HttpResponseException; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -345,6 +344,14 @@ public Void invoke(File f, VirtualChannel channel) throws IOException, Interrupt } } + private static final class HTTPAbortException extends AbortException { + final int code; + HTTPAbortException(int code, String message) { + super(message); + this.code = code; + } + } + /** * Upload a file to a URL */ @@ -352,7 +359,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException, Interrupt private static void uploadFile(Path f, URL url, final TaskListener listener) throws IOException { String urlSafe = url.toString().replaceFirst("[?].+$", "?…"); try { - Predicate nonfatal = x -> x instanceof IOException && (!(x instanceof HttpResponseException) || ((HttpResponseException) x).getStatusCode() >= 500); + Predicate nonfatal = x -> x instanceof IOException && (!(x instanceof HTTPAbortException) || ((HTTPAbortException) x).code >= 500); RetryerBuilder.newBuilder(). retryIfException(nonfatal). withRetryListener(new RetryListener() { @@ -385,7 +392,7 @@ public void onRetry(Attempt attempt) { try (InputStream err = connection.getErrorStream()) { diag = err != null ? IOUtils.toString(err, connection.getContentEncoding()) : null; } - throw new HttpResponseException(responseCode, String.format("Failed to upload %s to %s, response: %d %s, body: %s", f.toAbsolutePath(), urlSafe, responseCode, connection.getResponseMessage(), diag)); + throw new HTTPAbortException(responseCode, String.format("Failed to upload %s to %s, response: %d %s, body: %s", f.toAbsolutePath(), urlSafe, responseCode, connection.getResponseMessage(), diag)); } return null; }); diff --git a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java index 6683d36a..7d1bc0ce 100644 --- a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java +++ b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java @@ -76,6 +76,7 @@ public void unrecoverableExceptionArchiving() throws Exception { WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)); r.assertLogContains("/container/p/1/artifacts/f?…, response: 403 simulated 403 failure, body: Detailed explanation of 403.", b); r.assertLogNotContains("Retrying upload", b); + r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); } @Test @@ -87,6 +88,7 @@ public void recoverableExceptionArchiving() throws Exception { WorkflowRun b = r.buildAndAssertSuccess(p); r.assertLogContains("/container/p/1/artifacts/f?…, response: 500 simulated 500 failure, body: Detailed explanation of 500.", b); r.assertLogContains("Retrying upload", b); + r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); } @Test @@ -98,6 +100,7 @@ public void networkExceptionArchiving() throws Exception { WorkflowRun b = r.buildAndAssertSuccess(p); // currently prints a ‘java.net.SocketException: Connection reset’ but not sure if we really care r.assertLogContains("Retrying upload", b); + r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); } } From 6f2aad9e2bb63e077bb5fb0b8a6b82782d78a636 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 15:14:28 -0400 Subject: [PATCH 3/8] Logic about printing retry message was still not correct if we hit maximum attempts. --- .../JCloudsArtifactManager.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java index 95109f2e..f3cf0595 100644 --- a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java +++ b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java @@ -55,7 +55,6 @@ import com.github.rholder.retry.RetryerBuilder; import com.github.rholder.retry.StopStrategies; import com.github.rholder.retry.WaitStrategies; -import com.google.common.base.Predicate; import hudson.AbortException; import hudson.EnvVars; import hudson.FilePath; @@ -70,6 +69,7 @@ import hudson.util.io.ArchiverFactory; import io.jenkins.plugins.artifact_manager_jclouds.BlobStoreProvider.HttpMethod; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import jenkins.MasterToSlaveFileCallable; import jenkins.model.ArtifactManager; import jenkins.util.VirtualFile; @@ -359,17 +359,14 @@ private static final class HTTPAbortException extends AbortException { private static void uploadFile(Path f, URL url, final TaskListener listener) throws IOException { String urlSafe = url.toString().replaceFirst("[?].+$", "?…"); try { - Predicate nonfatal = x -> x instanceof IOException && (!(x instanceof HTTPAbortException) || ((HTTPAbortException) x).code >= 500); + AtomicReference lastError = new AtomicReference<>(); RetryerBuilder.newBuilder(). - retryIfException(nonfatal). + retryIfException(x -> x instanceof IOException && (!(x instanceof HTTPAbortException) || ((HTTPAbortException) x).code >= 500)). withRetryListener(new RetryListener() { @Override public void onRetry(Attempt attempt) { if (attempt.hasException()) { - Throwable t = attempt.getExceptionCause(); - if (nonfatal.apply(t)) { - listener.getLogger().println("Retrying upload after: " + t); - } + lastError.set(attempt.getExceptionCause()); } } }). @@ -379,6 +376,10 @@ public void onRetry(Attempt attempt) { withWaitStrategy(WaitStrategies.exponentialWait(100, 5, TimeUnit.MINUTES)). // TODO withAttemptTimeLimiter(…). build().call(() -> { + Throwable t = lastError.get(); + if (t != null) { + listener.getLogger().println("Retrying upload after: " + (t instanceof AbortException ? t.getMessage() : t.toString())); + } HttpURLConnection connection = (HttpURLConnection) url.openConnection(); connection.setDoOutput(true); connection.setRequestMethod("PUT"); From 2b27105060070caa44c782baba0537ad0d7c73db Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 15:15:09 -0400 Subject: [PATCH 4/8] Making upload parameters tunable. --- .../JCloudsArtifactManager.java | 77 ++++++++++++------- 1 file changed, 49 insertions(+), 28 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java index f3cf0595..dc312b68 100644 --- a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java +++ b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java @@ -137,6 +137,32 @@ public void archive(FilePath workspace, Launcher launcher, BuildListener listene listener.getLogger().printf("Uploaded %s artifact(s) to %s%n", artifactUrls.size(), provider.toURI(provider.getContainer(), getBlobPath("artifacts/"))); } + private static class UploadToBlobStorage extends MasterToSlaveFileCallable { + private static final long serialVersionUID = 1L; + + private final Map artifactUrls; // e.g. "target/x.war", "http://..." + private final TaskListener listener; + // Bind when constructed on the master side; on the agent side, deserialize those values. + private final int stopAfterAttemptNumber = UPLOAD_STOP_AFTER_ATTEMPT_NUMBER; + private final long waitMultiplier = UPLOAD_WAIT_MULTIPLIER; + private final long waitMaximum = UPLOAD_WAIT_MAXIMUM; + + UploadToBlobStorage(Map artifactUrls, TaskListener listener) { + this.artifactUrls = artifactUrls; + this.listener = listener; + } + + @Override + public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { + for (Map.Entry entry : artifactUrls.entrySet()) { + Path local = f.toPath().resolve(entry.getKey()); + URL url = entry.getValue(); + uploadFile(local, url, listener, stopAfterAttemptNumber, waitMultiplier, waitMaximum); + } + return null; + } + } + @Override public boolean delete() throws IOException, InterruptedException { String blobPath = getBlobPath(""); @@ -196,6 +222,9 @@ private static final class Stash extends MasterToSlaveFileCallable { private final boolean useDefaultExcludes; private final String tempDir; private final TaskListener listener; + private final int stopAfterAttemptNumber = UPLOAD_STOP_AFTER_ATTEMPT_NUMBER; + private final long waitMultiplier = UPLOAD_WAIT_MULTIPLIER; + private final long waitMaximum = UPLOAD_WAIT_MAXIMUM; Stash(URL url, String includes, String excludes, boolean useDefaultExcludes, String tempDir, TaskListener listener) throws IOException { this.url = url; @@ -221,7 +250,7 @@ public Integer invoke(File f, VirtualChannel channel) throws IOException, Interr throw new IOException(e); } if (count > 0) { - uploadFile(tmp, url, listener); + uploadFile(tmp, url, listener, stopAfterAttemptNumber, waitMultiplier, waitMaximum); } return count; } finally { @@ -322,28 +351,6 @@ private BlobStoreContext getContext() throws IOException { return provider.getContext(); } - private static class UploadToBlobStorage extends MasterToSlaveFileCallable { - private static final long serialVersionUID = 1L; - - private final Map artifactUrls; // e.g. "target/x.war", "http://..." - private final TaskListener listener; - - UploadToBlobStorage(Map artifactUrls, TaskListener listener) { - this.artifactUrls = artifactUrls; - this.listener = listener; - } - - @Override - public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { - for (Map.Entry entry : artifactUrls.entrySet()) { - Path local = f.toPath().resolve(entry.getKey()); - URL url = entry.getValue(); - uploadFile(local, url, listener); - } - return null; - } - } - private static final class HTTPAbortException extends AbortException { final int code; HTTPAbortException(int code, String message) { @@ -352,11 +359,27 @@ private static final class HTTPAbortException extends AbortException { } } + /** + * Number of upload attempts of nonfatal errors before giving up. + */ + static int UPLOAD_STOP_AFTER_ATTEMPT_NUMBER = Integer.getInteger(JCloudsArtifactManager.class.getName() + ".UPLOAD_STOP_AFTER_ATTEMPT_NUMBER", 10); + /** + * Initial number of milliseconds between first and second upload attempts. + * Subsequent ones increase exponentially. + * Note that this is not a randomized exponential backoff; + * and the base of the exponent is hard-coded to 2. + */ + static long UPLOAD_WAIT_MULTIPLIER = Long.getLong(JCloudsArtifactManager.class.getName() + ".UPLOAD_WAIT_MULTIPLIER", 100); + /** + * Maximum number of seconds between upload attempts. + */ + static long UPLOAD_WAIT_MAXIMUM = Long.getLong(JCloudsArtifactManager.class.getName() + ".UPLOAD_WAIT_MAXIMUM", 300); + /** * Upload a file to a URL */ @SuppressWarnings("Convert2Lambda") // bogus use of generics (type variable should have been on class); cannot be made into a lambda - private static void uploadFile(Path f, URL url, final TaskListener listener) throws IOException { + private static void uploadFile(Path f, URL url, final TaskListener listener, int stopAfterAttemptNumber, long waitMultiplier, long waitMaximum) throws IOException { String urlSafe = url.toString().replaceFirst("[?].+$", "?…"); try { AtomicReference lastError = new AtomicReference<>(); @@ -370,10 +393,8 @@ public void onRetry(Attempt attempt) { } } }). - // TODO all scalars configurable via system property - withStopStrategy(StopStrategies.stopAfterAttempt(10)). - // Note that this is not a _randomized_ exponential backoff; and the base of the exponent is hard-coded to 2. - withWaitStrategy(WaitStrategies.exponentialWait(100, 5, TimeUnit.MINUTES)). + withStopStrategy(StopStrategies.stopAfterAttempt(stopAfterAttemptNumber)). + withWaitStrategy(WaitStrategies.exponentialWait(waitMultiplier, waitMaximum, TimeUnit.SECONDS)). // TODO withAttemptTimeLimiter(…). build().call(() -> { Throwable t = lastError.get(); From dcf8f8923d4f4ecc7a6584074172fa63167379f6 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 15:15:49 -0400 Subject: [PATCH 5/8] Testing behavior of repeated upload errors. --- .../MockBlobStore.java | 26 +++++------ .../artifact_manager_jclouds/NetworkTest.java | 43 +++++++++++++++++-- 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java index 9503dd29..8492dae0 100644 --- a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java +++ b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/MockBlobStore.java @@ -39,13 +39,11 @@ import org.apache.http.HttpEntityEnclosingRequest; import org.apache.http.HttpRequest; import org.apache.http.HttpResponse; -import org.apache.http.HttpVersion; import org.apache.http.entity.ByteArrayEntity; -import org.apache.http.entity.StringEntity; import org.apache.http.impl.bootstrap.HttpServer; import org.apache.http.impl.bootstrap.ServerBootstrap; -import org.apache.http.message.BasicStatusLine; import org.apache.http.protocol.HttpContext; +import org.apache.http.protocol.HttpRequestHandler; import org.jclouds.ContextBuilder; import org.jclouds.blobstore.BlobStore; import org.jclouds.blobstore.BlobStoreContext; @@ -73,17 +71,17 @@ public String getPrefix() { public String getContainer() { return "container"; } - - private static final Map fails = new ConcurrentHashMap<>(); + + private static final Map specialHandlers = new ConcurrentHashMap<>(); /** - * Requests that the next HTTP access to a particular presigned URL should fail with a 4xx/5xx error. + * Requests that the next HTTP access to a particular presigned URL should behave specially. * @param method upload or download * @param key the blob’s {@link StorageMetadata#getName} - * @param code the status code, or 0 to just make the request fail without sending a proper response + * @param handler what to do instead */ - static void failIn(HttpMethod method, String key, int code) { - fails.put(method + ":" + key, code); + static void speciallyHandle(HttpMethod method, String key, HttpRequestHandler handler) { + specialHandlers.put(method + ":" + key, handler); } @Override @@ -99,13 +97,9 @@ public synchronized BlobStoreContext getContext() throws IOException { } String container = m.group(1); String key = m.group(2); - Integer failure = fails.remove(method + ":" + key); - if (failure != null) { - if (failure == 0) { - throw new ConnectionClosedException("Refusing to even send a status code for " + container + ":" + key); - } - response.setStatusLine(new BasicStatusLine(HttpVersion.HTTP_1_0, failure, "simulated " + failure + " failure")); - response.setEntity(new StringEntity("Detailed explanation of " + failure + ".")); + HttpRequestHandler specialHandler = specialHandlers.remove(method + ":" + key); + if (specialHandler != null) { + specialHandler.handle(request, response, _context); return; } BlobStore blobStore = context.getBlobStore(); diff --git a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java index 7d1bc0ce..d6256d27 100644 --- a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java +++ b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java @@ -25,6 +25,10 @@ import hudson.model.Result; import jenkins.model.ArtifactManagerConfiguration; +import org.apache.http.ConnectionClosedException; +import org.apache.http.HttpVersion; +import org.apache.http.entity.StringEntity; +import org.apache.http.message.BasicStatusLine; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; @@ -71,9 +75,10 @@ public void configureManager() throws Exception { public void unrecoverableExceptionArchiving() throws Exception { WorkflowJob p = r.createProject(WorkflowJob.class, "p"); r.createSlave("remote", null, null); - MockBlobStore.failIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f", 403); + failIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f", 403, 0); p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f', text: '.'; archiveArtifacts 'f'}", true)); WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)); + r.assertLogContains("ERROR: Failed to upload", b); r.assertLogContains("/container/p/1/artifacts/f?…, response: 403 simulated 403 failure, body: Detailed explanation of 403.", b); r.assertLogNotContains("Retrying upload", b); r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); @@ -83,7 +88,7 @@ public void unrecoverableExceptionArchiving() throws Exception { public void recoverableExceptionArchiving() throws Exception { WorkflowJob p = r.createProject(WorkflowJob.class, "p"); r.createSlave("remote", null, null); - MockBlobStore.failIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f", 500); + failIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f", 500, 0); p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f', text: '.'; archiveArtifacts 'f'}", true)); WorkflowRun b = r.buildAndAssertSuccess(p); r.assertLogContains("/container/p/1/artifacts/f?…, response: 500 simulated 500 failure, body: Detailed explanation of 500.", b); @@ -95,7 +100,7 @@ public void recoverableExceptionArchiving() throws Exception { public void networkExceptionArchiving() throws Exception { WorkflowJob p = r.createProject(WorkflowJob.class, "p"); r.createSlave("remote", null, null); - MockBlobStore.failIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f", 0); + failIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f", 0, 0); p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f', text: '.'; archiveArtifacts 'f'}", true)); WorkflowRun b = r.buildAndAssertSuccess(p); // currently prints a ‘java.net.SocketException: Connection reset’ but not sure if we really care @@ -103,4 +108,36 @@ public void networkExceptionArchiving() throws Exception { r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); } + @Test + public void repeatedRecoverableExceptionArchiving() throws Exception { + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + r.createSlave("remote", null, null); + int origStopAfterAttemptNumber = JCloudsArtifactManager.UPLOAD_STOP_AFTER_ATTEMPT_NUMBER; + JCloudsArtifactManager.UPLOAD_STOP_AFTER_ATTEMPT_NUMBER = 3; + try { + failIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f", 500, JCloudsArtifactManager.UPLOAD_STOP_AFTER_ATTEMPT_NUMBER); + p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f', text: '.'; archiveArtifacts 'f'}", true)); + WorkflowRun b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)); + r.assertLogContains("ERROR: Failed to upload", b); + r.assertLogContains("/container/p/1/artifacts/f?…, response: 500 simulated 500 failure, body: Detailed explanation of 500.", b); + r.assertLogContains("Retrying upload", b); + r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); + } finally { + JCloudsArtifactManager.UPLOAD_STOP_AFTER_ATTEMPT_NUMBER = origStopAfterAttemptNumber; + } + } + + private static void failIn(BlobStoreProvider.HttpMethod method, String key, int code, int repeats) { + MockBlobStore.speciallyHandle(method, key, (request, response, context) -> { + if (repeats > 0) { + failIn(method, key, code, repeats - 1); + } + if (code == 0) { + throw new ConnectionClosedException("Refusing to even send a status code for " + key); + } + response.setStatusLine(new BasicStatusLine(HttpVersion.HTTP_1_0, code, "simulated " + code + " failure")); + response.setEntity(new StringEntity("Detailed explanation of " + code + ".")); + }); + } + } From d47333af616c483c7a30a2b1bc8047a3f9e5d7ba Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 15:45:45 -0400 Subject: [PATCH 6/8] Apply a timeout to each upload attempt. --- .../JCloudsArtifactManager.java | 24 +++++++++++++---- .../artifact_manager_jclouds/NetworkTest.java | 27 +++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java index dc312b68..d2d98158 100644 --- a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java +++ b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java @@ -25,6 +25,7 @@ package io.jenkins.plugins.artifact_manager_jclouds; import com.github.rholder.retry.Attempt; +import com.github.rholder.retry.AttemptTimeLimiters; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -55,12 +56,14 @@ import com.github.rholder.retry.RetryerBuilder; import com.github.rholder.retry.StopStrategies; import com.github.rholder.retry.WaitStrategies; +import com.google.common.util.concurrent.UncheckedTimeoutException; import hudson.AbortException; import hudson.EnvVars; import hudson.FilePath; import hudson.Launcher; import hudson.Util; import hudson.model.BuildListener; +import hudson.model.Computer; import hudson.model.Run; import hudson.model.TaskListener; import hudson.remoting.VirtualChannel; @@ -68,10 +71,13 @@ import hudson.util.DirScanner; import hudson.util.io.ArchiverFactory; import io.jenkins.plugins.artifact_manager_jclouds.BlobStoreProvider.HttpMethod; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import jenkins.MasterToSlaveFileCallable; import jenkins.model.ArtifactManager; +import jenkins.util.JenkinsJVM; import jenkins.util.VirtualFile; import org.apache.commons.io.IOUtils; import org.kohsuke.accmod.Restricted; @@ -146,6 +152,7 @@ private static class UploadToBlobStorage extends MasterToSlaveFileCallable private final int stopAfterAttemptNumber = UPLOAD_STOP_AFTER_ATTEMPT_NUMBER; private final long waitMultiplier = UPLOAD_WAIT_MULTIPLIER; private final long waitMaximum = UPLOAD_WAIT_MAXIMUM; + private final long timeout = UPLOAD_TIMEOUT; UploadToBlobStorage(Map artifactUrls, TaskListener listener) { this.artifactUrls = artifactUrls; @@ -157,7 +164,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException, Interrupt for (Map.Entry entry : artifactUrls.entrySet()) { Path local = f.toPath().resolve(entry.getKey()); URL url = entry.getValue(); - uploadFile(local, url, listener, stopAfterAttemptNumber, waitMultiplier, waitMaximum); + uploadFile(local, url, listener, stopAfterAttemptNumber, waitMultiplier, waitMaximum, timeout); } return null; } @@ -225,6 +232,7 @@ private static final class Stash extends MasterToSlaveFileCallable { private final int stopAfterAttemptNumber = UPLOAD_STOP_AFTER_ATTEMPT_NUMBER; private final long waitMultiplier = UPLOAD_WAIT_MULTIPLIER; private final long waitMaximum = UPLOAD_WAIT_MAXIMUM; + private final long timeout = UPLOAD_TIMEOUT; Stash(URL url, String includes, String excludes, boolean useDefaultExcludes, String tempDir, TaskListener listener) throws IOException { this.url = url; @@ -250,7 +258,7 @@ public Integer invoke(File f, VirtualChannel channel) throws IOException, Interr throw new IOException(e); } if (count > 0) { - uploadFile(tmp, url, listener, stopAfterAttemptNumber, waitMultiplier, waitMaximum); + uploadFile(tmp, url, listener, stopAfterAttemptNumber, waitMultiplier, waitMaximum, timeout); } return count; } finally { @@ -374,17 +382,23 @@ private static final class HTTPAbortException extends AbortException { * Maximum number of seconds between upload attempts. */ static long UPLOAD_WAIT_MAXIMUM = Long.getLong(JCloudsArtifactManager.class.getName() + ".UPLOAD_WAIT_MAXIMUM", 300); + /** + * Number of seconds to permit a single upload attempt to take. + */ + static long UPLOAD_TIMEOUT = Long.getLong(JCloudsArtifactManager.class.getName() + ".UPLOAD_TIMEOUT", /* 15m */15 * 60); + + private static final ExecutorService executors = JenkinsJVM.isJenkinsJVM() ? Computer.threadPoolForRemoting : Executors.newCachedThreadPool(); /** * Upload a file to a URL */ @SuppressWarnings("Convert2Lambda") // bogus use of generics (type variable should have been on class); cannot be made into a lambda - private static void uploadFile(Path f, URL url, final TaskListener listener, int stopAfterAttemptNumber, long waitMultiplier, long waitMaximum) throws IOException { + private static void uploadFile(Path f, URL url, final TaskListener listener, int stopAfterAttemptNumber, long waitMultiplier, long waitMaximum, long timeout) throws IOException { String urlSafe = url.toString().replaceFirst("[?].+$", "?…"); try { AtomicReference lastError = new AtomicReference<>(); RetryerBuilder.newBuilder(). - retryIfException(x -> x instanceof IOException && (!(x instanceof HTTPAbortException) || ((HTTPAbortException) x).code >= 500)). + retryIfException(x -> x instanceof IOException && (!(x instanceof HTTPAbortException) || ((HTTPAbortException) x).code >= 500) || x instanceof UncheckedTimeoutException). withRetryListener(new RetryListener() { @Override public void onRetry(Attempt attempt) { @@ -395,7 +409,7 @@ public void onRetry(Attempt attempt) { }). withStopStrategy(StopStrategies.stopAfterAttempt(stopAfterAttemptNumber)). withWaitStrategy(WaitStrategies.exponentialWait(waitMultiplier, waitMaximum, TimeUnit.SECONDS)). - // TODO withAttemptTimeLimiter(…). + withAttemptTimeLimiter(AttemptTimeLimiters.fixedTimeLimit(timeout, TimeUnit.SECONDS, executors)). build().call(() -> { Throwable t = lastError.get(); if (t != null) { diff --git a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java index d6256d27..44ad490d 100644 --- a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java +++ b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java @@ -127,6 +127,23 @@ public void repeatedRecoverableExceptionArchiving() throws Exception { } } + @Test + public void hangArchiving() throws Exception { + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + r.createSlave("remote", null, null); + long origTimeout = JCloudsArtifactManager.UPLOAD_TIMEOUT; + JCloudsArtifactManager.UPLOAD_TIMEOUT = 5; + try { + hangIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f"); + p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f', text: '.'; archiveArtifacts 'f'}", true)); + WorkflowRun b = r.buildAndAssertSuccess(p); + r.assertLogContains("Retrying upload", b); + r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); + } finally { + JCloudsArtifactManager.UPLOAD_TIMEOUT = origTimeout; + } + } + private static void failIn(BlobStoreProvider.HttpMethod method, String key, int code, int repeats) { MockBlobStore.speciallyHandle(method, key, (request, response, context) -> { if (repeats > 0) { @@ -140,4 +157,14 @@ private static void failIn(BlobStoreProvider.HttpMethod method, String key, int }); } + private static void hangIn(BlobStoreProvider.HttpMethod method, String key) { + MockBlobStore.speciallyHandle(method, key, (request, response, context) -> { + try { + Thread.sleep(Long.MAX_VALUE); + } catch (InterruptedException x) { + assert false : x; // on the server side, should not happen + } + }); + } + } From d28438e58db1457d9010a0ece3fd3176e03634fd Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 16:01:38 -0400 Subject: [PATCH 7/8] Also verify timeouts from uploads on the master node, since this uses a different executor service. --- .../plugins/artifact_manager_jclouds/NetworkTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java index 44ad490d..ed5a289c 100644 --- a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java +++ b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java @@ -139,6 +139,12 @@ public void hangArchiving() throws Exception { WorkflowRun b = r.buildAndAssertSuccess(p); r.assertLogContains("Retrying upload", b); r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); + // Also from master: + hangIn(BlobStoreProvider.HttpMethod.PUT, "p/2/artifacts/f"); + p.setDefinition(new CpsFlowDefinition("node('master') {writeFile file: 'f', text: '.'; archiveArtifacts 'f'}", true)); + b = r.buildAndAssertSuccess(p); + r.assertLogContains("Retrying upload", b); + r.assertLogNotContains("\tat hudson.tasks.ArtifactArchiver.perform", b); } finally { JCloudsArtifactManager.UPLOAD_TIMEOUT = origTimeout; } From d8ca445ffe40022b7cf84f5329e35276b827e6fc Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 31 May 2018 16:53:44 -0400 Subject: [PATCH 8/8] Proper handling of build interrupts. --- pom.xml | 4 ++-- .../JCloudsArtifactManager.java | 4 +++- .../artifact_manager_jclouds/NetworkTest.java | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 8fb8ff53..b6d60b65 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.jenkins-ci.plugins plugin - 3.11 + 3.12 io.jenkins.plugins @@ -191,7 +191,7 @@ org.jenkins-ci.plugins.workflow workflow-step-api - 2.15 + 2.16-rc310.04f07c15faaf test diff --git a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java index d2d98158..a6664754 100644 --- a/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java +++ b/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java @@ -393,7 +393,7 @@ private static final class HTTPAbortException extends AbortException { * Upload a file to a URL */ @SuppressWarnings("Convert2Lambda") // bogus use of generics (type variable should have been on class); cannot be made into a lambda - private static void uploadFile(Path f, URL url, final TaskListener listener, int stopAfterAttemptNumber, long waitMultiplier, long waitMaximum, long timeout) throws IOException { + private static void uploadFile(Path f, URL url, final TaskListener listener, int stopAfterAttemptNumber, long waitMultiplier, long waitMaximum, long timeout) throws IOException, InterruptedException { String urlSafe = url.toString().replaceFirst("[?].+$", "?…"); try { AtomicReference lastError = new AtomicReference<>(); @@ -438,6 +438,8 @@ public void onRetry(Attempt attempt) { throw (IOException) x2; } else if (x2 instanceof RuntimeException) { throw (RuntimeException) x2; + } else if (x2 instanceof InterruptedException) { + throw (InterruptedException) x2; } else { // Error? throw new RuntimeException(x); } diff --git a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java index ed5a289c..fc195901 100644 --- a/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java +++ b/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/NetworkTest.java @@ -32,6 +32,7 @@ import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.jenkinsci.plugins.workflow.steps.TimeoutStepExecution; import org.junit.ClassRule; import org.junit.Test; import org.junit.Before; @@ -150,6 +151,24 @@ public void hangArchiving() throws Exception { } } + @Test + public void interruptedArchiving() throws Exception { + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + r.createSlave("remote", null, null); + hangIn(BlobStoreProvider.HttpMethod.PUT, "p/1/artifacts/f"); + p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f', text: '.'; archiveArtifacts 'f'}", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + r.waitForMessage("Archiving artifacts", b); + Thread.sleep(2000); // wait for hangIn to sleep; OK if occasionally it has not gotten there yet, we still expect the same result + b.getExecutor().interrupt(); + r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b)); + // Currently prints a stack trace of java.lang.InterruptedException; good enough. + // Check the same from a timeout within the build, rather than a user abort, and also from master just for fun: + hangIn(BlobStoreProvider.HttpMethod.PUT, "p/2/artifacts/f"); + p.setDefinition(new CpsFlowDefinition("node('master') {writeFile file: 'f', text: '.'; timeout(time: 3, unit: 'SECONDS') {archiveArtifacts 'f'}}", true)); + r.assertLogContains(new TimeoutStepExecution.ExceededTimeout().getShortDescription(), r.assertBuildStatus(Result.ABORTED, p.scheduleBuild2(0))); + } + private static void failIn(BlobStoreProvider.HttpMethod method, String key, int code, int repeats) { MockBlobStore.speciallyHandle(method, key, (request, response, context) -> { if (repeats > 0) {