[JENKINS-49635] Permitting actual copying to take place entirely on t…

…he agent.
jenkinsci · Feb 21, 2018 · 422eea3 · 422eea3
1 parent ccee158
commit 422eea3
Show file tree

Hide file tree

Showing 2 changed files with 220 additions and 45 deletions.
diff --git a/src/main/java/hudson/plugins/copyartifact/CopyArtifact.java b/src/main/java/hudson/plugins/copyartifact/CopyArtifact.java
@@ -42,6 +42,7 @@
 import hudson.maven.MavenModuleSetBuild;
 import hudson.model.*;
 import hudson.model.listeners.ItemListener;
+import hudson.remoting.VirtualChannel;
 import hudson.security.ACL;
 import hudson.security.SecurityRealm;
 import hudson.tasks.BuildStepDescriptor;
@@ -51,6 +52,7 @@
 import hudson.util.FormValidation;
 import hudson.util.VariableResolver;
 import hudson.util.XStream2;
+import java.io.File;
 
 import java.io.IOException;
 import java.io.InputStream;
@@ -86,10 +88,9 @@
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
+import jenkins.MasterToSlaveFileCallable;
 import jenkins.util.VirtualFile;
 import org.apache.commons.io.IOUtils;
-import org.apache.tools.ant.DirectoryScanner;
-import org.apache.tools.ant.types.selectors.SelectorUtils;
 
 /**
  * Build step to copy artifacts from another project.
@@ -527,63 +528,98 @@ private boolean perform(Run src, Run<?,?> dst, String expandedFilter, @CheckForN
         if (srcDir == null) {
             return isOptional();  // Fail build unless copy is optional
         }
-
-        Map<String, String> fingerprints;
-        MessageDigest md5;
-        if (isFingerprintArtifacts()) {
-            fingerprints = new HashMap<>();
-            try {
-                md5 = MessageDigest.getInstance("MD5");
-            } catch (NoSuchAlgorithmException x) {
-                throw new AssertionError(x);
-            }
-        } else {
-            fingerprints = null;
-            md5 = null;
-        }
+        Map<String, String> fingerprints = null; // entry → MD5
         try {
-            targetDir.mkdirs();  // Create target if needed
-            Collection<String> list = srcDir.list(expandedFilter.replace('\\', '/'), expandedExcludes != null ? expandedExcludes.replace('\\', '/') : null, false);
-            for (String file : list) {
-                copyOne(src, dst, fingerprints, srcDir.child(file), new FilePath(targetDir, isFlatten() ? file.replaceFirst(".+/", "") : file), md5, listener);
+            if (targetDir.isRemote()) {
+                VirtualFile srcDirRemote = srcDir.asRemotable();
+                if (srcDirRemote != null) {
+                    // Perform listing and copying on the agent side so we do not need to stream data from the master.
+                    fingerprints = targetDir.act(new Copy(targetDir, srcDirRemote, expandedFilter, expandedExcludes, isFingerprintArtifacts(), listener, isFlatten()));
+                } else {
+                    fingerprints = new Copy(targetDir, srcDir, expandedFilter, expandedExcludes, isFingerprintArtifacts(), listener, isFlatten()).invoke(null, null);
+                }
+            } else {
+                fingerprints = new Copy(targetDir, srcDir, expandedFilter, expandedExcludes, isFingerprintArtifacts(), listener, isFlatten()).invoke(null, null);
             }
-            int cnt = list.size();
+            int cnt = fingerprints.size();
             console.println(Messages.CopyArtifact_Copied(cnt, HyperlinkNote.encodeTo('/'+ src.getParent().getUrl(), src.getParent().getFullDisplayName()),
                     HyperlinkNote.encodeTo('/'+src.getUrl(), Integer.toString(src.getNumber()))));
             // Fail build if 0 files copied unless copy is optional
             return cnt > 0 || isOptional();
         } finally {
             if (fingerprints != null) {
+                Map<String, String> fingerprintsShallow = new HashMap<>();
+                FingerprintMap map = Jenkins.get().getFingerprintMap();
+                for (Map.Entry<String, String> entry : fingerprints.entrySet()) {
+                    String name = entry.getKey().replaceFirst(".+/", "");
+                    String digest = entry.getValue();
+                    if (digest == null) {
+                        continue;
+                    }
+                    fingerprintsShallow.put(name, digest);
+                    Fingerprint f = map.getOrCreate(src, name, digest);
+                    f.addFor(src);
+                    f.addFor(dst);
+                }
                 for (Run<?, ?> r : new Run<?, ?>[] {src, dst}) {
-                    if (fingerprints.size() > 0) {
-                        Fingerprinter.FingerprintAction fa = r.getAction(Fingerprinter.FingerprintAction.class);
-                        if (fa != null) {
-                            fa.add(fingerprints);
-                        } else {
-                            r.addAction(new Fingerprinter.FingerprintAction(r, fingerprints));
-                        }
+                    Fingerprinter.FingerprintAction fa = r.getAction(Fingerprinter.FingerprintAction.class);
+                    if (fa != null) {
+                        fa.add(fingerprintsShallow);
+                    } else {
+                        r.addAction(new Fingerprinter.FingerprintAction(r, fingerprintsShallow));
                     }
                 }
             }
         }
     }
 
-    /** Similar to a method in {@link DirectoryScanner}. */
-    private static String normalizePattern(String p) {
-        String pattern = p.replace('\\', '/'); // we only deal with forward slashes here
-        if (pattern.endsWith("/")) {
-            pattern += SelectorUtils.DEEP_TREE_MATCH;
+    private static final class Copy extends MasterToSlaveFileCallable<Map<String, String>> {
+        private static final long serialVersionUID = 1;
+        private final FilePath targetDir;
+        private final VirtualFile srcDir;
+        private final String expandedFilter;
+        private final String expandedExcludes;
+        private final boolean fingerprint;
+        private final TaskListener listener;
+        private final boolean flatten;
+        Copy(FilePath targetDir, VirtualFile srcDir, String expandedFilter, String expandedExcludes, boolean fingerprint, TaskListener listener, boolean flatten) {
+            this.targetDir = targetDir;
+            this.srcDir = srcDir;
+            this.expandedFilter = expandedFilter;
+            this.expandedExcludes = expandedExcludes;
+            this.fingerprint = fingerprint;
+            this.listener = listener;
+            this.flatten = flatten;
+        }
+        @Override
+        public Map<String, String> invoke(File _file, VirtualChannel _vc) throws IOException, InterruptedException {
+            targetDir.mkdirs();  // Create target if needed
+            Collection<String> list = srcDir.list(expandedFilter.replace('\\', '/'), expandedExcludes != null ? expandedExcludes.replace('\\', '/') : null, false);
+            Map<String, String> fingerprints = new HashMap<>();
+            MessageDigest md5;
+            if (fingerprint) {
+                try {
+                    md5 = MessageDigest.getInstance("MD5");
+                } catch (NoSuchAlgorithmException x) {
+                    throw new AssertionError(x);
+                }
+            } else {
+                md5 = null;
+            }
+            for (String entry : list) {
+                String digest = copyOne(srcDir.child(entry), new FilePath(targetDir, flatten ? entry.replaceFirst(".+/", "") : entry), md5, listener);
+                fingerprints.put(entry, digest);
+            }
+            return fingerprints;
         }
-        return pattern;
     }
 
-    private static void copyOne(Run<?,?> src, Run<?,?> dst, Map<String, String> fingerprints, VirtualFile s, FilePath d, MessageDigest md5, TaskListener listener) throws IOException, InterruptedException {
-        assert (fingerprints == null) == (md5 == null);
+    private static String copyOne(VirtualFile s, FilePath d, @CheckForNull MessageDigest md5, TaskListener listener) throws IOException, InterruptedException {
         String link = s.readLink();
         if (link != null) {
             d.getParent().mkdirs();
             d.symlinkTo(link, listener);
-            return;
+            return null;
         }
         try {
             try (InputStream is = s.open(); OutputStream os = d.write()) {
@@ -606,14 +642,7 @@ private static void copyOne(Run<?,?> src, Run<?,?> dst, Map<String, String> fing
             if (mode != -1) {
                 d.chmod(mode);
             }
-            if (fingerprints != null) {
-                String digest = Util.toHexString(md5.digest());
-                FingerprintMap map = Jenkins.getActiveInstance().getFingerprintMap();
-                Fingerprint f = map.getOrCreate(src, s.getName(), digest);
-                f.addFor(src);
-                f.addFor(dst);
-                fingerprints.put(s.getName(), digest);
-            }
+            return md5 != null ? Util.toHexString(md5.digest()) : null;
         } catch (IOException e) {
             throw new IOException("Failed to copy " + s + " to " + d, e);
         }

diff --git a/src/test/java/hudson/plugins/copyartifact/CopyArtifactTest.java b/src/test/java/hudson/plugins/copyartifact/CopyArtifactTest.java
@@ -92,14 +92,25 @@
 import com.cloudbees.hudson.plugins.folder.Folder;
 import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
 import com.google.common.collect.Sets;
+import hudson.Util;
+import hudson.remoting.Callable;
+import java.io.InputStream;
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collection;
+import jenkins.model.ArtifactManager;
 import jenkins.model.ArtifactManagerConfiguration;
+import jenkins.model.ArtifactManagerFactory;
+import jenkins.model.ArtifactManagerFactoryDescriptor;
+import jenkins.util.VirtualFile;
 import static org.hamcrest.Matchers.*;
 import org.jenkinsci.plugins.compress_artifacts.CompressingArtifactManagerFactory;
 
 import org.jvnet.hudson.test.TestBuilder;
 
 import static org.junit.Assert.*;
 import static org.junit.Assume.*;
+import org.jvnet.hudson.test.TestExtension;
 
 /**
  * Test interaction of copyartifact plugin with Jenkins core.
@@ -2118,4 +2129,139 @@ public void compressArtifacts() throws Exception {
         assertFile(true, "deepfoo/a/b/c.log", b);
     }
 
+    @Issue("JENKINS-49635")
+    @Test
+    public void directDownload() throws Exception {
+        ArtifactManagerConfiguration.get().getArtifactManagerFactories().add(new DirectArtifactManagerFactory());
+        FreeStyleProject other = createArtifactProject();
+        FreeStyleBuild s = rule.buildAndAssertSuccess(other);
+        FreeStyleProject p = createProject(other.getName(), null, "", "", false, false, false, true);
+        p.setAssignedNode(rule.createSlave());
+        FreeStyleBuild b = rule.buildAndAssertSuccess(p);
+        for (String file : new String[] {"foo.txt", "subdir/subfoo.txt", "deepfoo/a/b/c.log"}) {
+            assertFile(true, file, b);
+            String digest = b.getWorkspace().child(file).digest();
+            Fingerprint f = Jenkins.get().getFingerprintMap().get(digest);
+            assertSame(f.getOriginal().getRun(), s);
+            assertTrue(f.getRangeSet(p).includes(b.getNumber()));
+        }
+    }
+    public static final class DirectArtifactManagerFactory extends ArtifactManagerFactory {
+        @Override
+        public ArtifactManager managerFor(Run<?, ?> build) {
+            return new DirectArtifactManager(build);
+        }
+        @TestExtension("directDownload")
+        public static final class DescriptorImpl extends ArtifactManagerFactoryDescriptor {}
+    }
+    private static final class DirectArtifactManager extends ArtifactManager {
+        private File dir;
+        DirectArtifactManager(Run<?, ?> build) {
+            onLoad(build);
+        }
+        @Override
+        public void archive(FilePath workspace, Launcher launcher, BuildListener listener, Map<String, String> artifacts) throws IOException, InterruptedException {
+            workspace.copyRecursiveTo(new FilePath.ExplicitlySpecifiedDirScanner(artifacts), new FilePath(dir), "copying");
+        }
+        @Override
+        public VirtualFile root() {
+            return new RemotableVF(VirtualFile.forFile(dir), false);
+        }
+        @Override
+        public void onLoad(Run<?, ?> build) {
+            dir = new File(Jenkins.get().getRootDir(), Util.getDigestOf(build.getExternalizableId()));
+        }
+        @Override
+        public boolean delete() throws IOException, InterruptedException {
+            return false;
+        }
+    }
+    private static final class RemotableVF extends VirtualFile {
+        private final VirtualFile delegate;
+        private final boolean remoted;
+        RemotableVF(VirtualFile delegate, boolean remoted) {
+            this.delegate = delegate;
+            this.remoted = remoted;
+        }
+        @Override
+        public VirtualFile asRemotable() {
+            assertFalse(remoted);
+            return new RemotableVF(delegate, true);
+        }
+        private void remoteOnly() {
+            assertTrue(remoted);
+        }
+        @Override
+        public String getName() {
+            return delegate.getName();
+        }
+        @Override
+        public URI toURI() {
+            return delegate.toURI();
+        }
+        @Override
+        public VirtualFile getParent() {
+            return new RemotableVF(delegate.getParent(), remoted);
+        }
+        @Override
+        public boolean isDirectory() throws IOException {
+            return delegate.isDirectory();
+        }
+        @Override
+        public boolean isFile() throws IOException {
+            return delegate.isFile();
+        }
+        @Override
+        public String readLink() throws IOException {
+            return delegate.readLink();
+        }
+        @Override
+        public boolean exists() throws IOException {
+            return delegate.exists();
+        }
+        @Override
+        public VirtualFile[] list() throws IOException {
+            remoteOnly();
+            return Arrays.stream(delegate.list()).map(f -> new RemotableVF(f, remoted)).toArray(VirtualFile[]::new);
+        }
+        @Override
+        public Collection<String> list(String includes, String excludes, boolean useDefaultExcludes) throws IOException {
+            remoteOnly();
+            return delegate.list(includes, excludes, useDefaultExcludes);
+        }
+        @Override
+        public VirtualFile child(String string) {
+            return new RemotableVF(delegate.child(string), remoted);
+        }
+        @Override
+        public long length() throws IOException {
+            remoteOnly();
+            return delegate.length();
+        }
+        @Override
+        public long lastModified() throws IOException {
+            remoteOnly();
+            return delegate.lastModified();
+        }
+        @Override
+        public int mode() throws IOException {
+            remoteOnly();
+            return delegate.mode();
+        }
+        @Override
+        public boolean canRead() throws IOException {
+            remoteOnly();
+            return delegate.canRead();
+        }
+        @Override
+        public InputStream open() throws IOException {
+            remoteOnly();
+            return delegate.open();
+        }
+        @Override
+        public <V> V run(Callable<V, IOException> clbl) throws IOException {
+            return delegate.run(clbl);
+        }
+    }
+
 }