[JENKINS-49635] VirtualFile support #67
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| buildPlugin() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,12 +26,16 @@ | |
|
|
||
| import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
| import hudson.AbortException; | ||
| import hudson.EnvVars; | ||
| import hudson.Extension; | ||
| import hudson.ExtensionList; | ||
| import hudson.ExtensionPoint; | ||
| import hudson.FilePath; | ||
| import hudson.Launcher; | ||
| import hudson.Launcher.LocalLauncher; | ||
| import hudson.Util; | ||
| import hudson.model.Computer; | ||
| import hudson.model.Node; | ||
| import hudson.model.Run; | ||
| import hudson.model.TaskListener; | ||
| import hudson.org.apache.tools.tar.TarInputStream; | ||
|
|
@@ -57,6 +61,7 @@ | |
| import org.apache.commons.io.IOUtils; | ||
| import org.apache.tools.tar.TarEntry; | ||
| import org.kohsuke.accmod.Restricted; | ||
| import org.kohsuke.accmod.restrictions.Beta; | ||
| import org.kohsuke.accmod.restrictions.DoNotUse; | ||
| import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
|
|
||
|
|
@@ -84,20 +89,35 @@ public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull | |
| stash(build, name, workspace, listener, includes, excludes, useDefaultExcludes, false); | ||
| } | ||
|
|
||
| @Deprecated | ||
| public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull TaskListener listener, | ||
| @CheckForNull String includes, @CheckForNull String excludes, boolean useDefaultExcludes, boolean allowEmpty) throws IOException, InterruptedException { | ||
| stash(build, name, workspace, launcherFor(workspace, listener), envFor(build, workspace, listener), listener, includes, excludes, useDefaultExcludes, allowEmpty); | ||
| } | ||
|
|
||
| /** | ||
| * Saves a stash of some files from a build. | ||
| * @param build a build to use as storage | ||
| * @param name a simple name to assign to the stash (must follow {@link Jenkins#checkGoodName} constraints) | ||
| * @param workspace a directory to use as a base | ||
| * @param launcher a way to launch processes, if required | ||
| * @param env environment to use when launching processes, if required | ||
| * @param listener a way to report progress or problems | ||
| * @param includes a set of Ant-style file includes, separated by commas; null/blank is allowed as a synonym for {@code **} (i.e., everything) | ||
| * @param excludes an optional set of Ant-style file excludes | ||
| * @param useDefaultExcludes whether to use Ant default excludes | ||
| * @param allowEmpty whether to allow an empty stash | ||
| * @see StashAwareArtifactManager#stash | ||
| */ | ||
| @SuppressFBWarnings(value="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE", justification="fine if mkdirs returns false") | ||
| public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener, | ||
|
Member
Author
There was a problem hiding this comment. @abayer FYI, this is the new overload which could plausibly be combined with a
Member
There was a problem hiding this comment. Not entirely clear on what you mean? I made a comment over at https://issues.jenkins-ci.org/browse/JENKINS-40912 wondering whether building support for listing stash files into
Member
There was a problem hiding this comment. Our set of parameters is starting to get unruly - should we consider simplifying to accept some sort of
Member
Author
There was a problem hiding this comment. Possible, though six are mandatory and only five optional, and there is only one caller so we are not really simplifying anything that way. Option builders are useful when you have an open-ended list of callers and most of them do not need to specify most of the options.
Member
There was a problem hiding this comment. I'll let it go for now -- you're right that the set of callers is currently pretty limited. |
||
| @CheckForNull String includes, @CheckForNull String excludes, boolean useDefaultExcludes, boolean allowEmpty) throws IOException, InterruptedException { | ||
| Jenkins.checkGoodName(name); | ||
| StashAwareArtifactManager saam = stashAwareArtifactManager(build); | ||
| if (saam != null) { | ||
| saam.stash(name, workspace, launcher, env, listener, includes, excludes, useDefaultExcludes, allowEmpty); | ||
| return; | ||
| } | ||
| File storage = storage(build, name); | ||
| storage.getParentFile().mkdirs(); | ||
| if (storage.isFile()) { | ||
|
|
@@ -115,49 +135,88 @@ public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull | |
| } | ||
| } | ||
|
|
||
| @Deprecated | ||
| public static void unstash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull TaskListener listener) throws IOException, InterruptedException { | ||
| unstash(build, name, workspace, launcherFor(workspace, listener), envFor(build, workspace, listener), listener); | ||
| } | ||
|
|
||
| /** | ||
| * Restores a stash of some files from a build. | ||
| * @param build a build used as storage | ||
| * @param name a name passed previously to {@link #stash} | ||
| * @param workspace a directory to copy into | ||
| * @param launcher a way to launch processes, if required | ||
| * @param env environment to use when launching processes, if required | ||
| * @param listener a way to report progress or problems | ||
| * @throws AbortException in case there is no such saved stash | ||
| * @see StashAwareArtifactManager#unstash | ||
| */ | ||
| public static void unstash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws IOException, InterruptedException { | ||
| Jenkins.checkGoodName(name); | ||
| StashAwareArtifactManager saam = stashAwareArtifactManager(build); | ||
| if (saam != null) { | ||
| saam.unstash(name, workspace, launcher, env, listener); | ||
| return; | ||
| } | ||
| File storage = storage(build, name); | ||
| if (!storage.isFile()) { | ||
| throw new AbortException("No such saved stash ‘" + name + "’"); | ||
| } | ||
| new FilePath(storage).untar(workspace, FilePath.TarCompression.GZIP); | ||
| } | ||
|
|
||
| @Deprecated | ||
| public static void clearAll(@Nonnull Run<?,?> build) throws IOException { | ||
| try { | ||
| clearAll(build, TaskListener.NULL); | ||
| } catch (InterruptedException x) { | ||
| throw new IOException(x); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Delete any and all stashes in a build. | ||
| * @param build a build possibly passed to {@link #stash} in the past | ||
| * @param listener a way to report progress or problems | ||
| * @see StashAwareArtifactManager#clearAllStashes | ||
| */ | ||
| public static void clearAll(@Nonnull Run<?,?> build, @Nonnull TaskListener listener) throws IOException, InterruptedException { | ||
| StashAwareArtifactManager saam = stashAwareArtifactManager(build); | ||
| if (saam != null) { | ||
| saam.clearAllStashes(listener); | ||
| return; | ||
| } | ||
| Util.deleteRecursive(storage(build)); | ||
| } | ||
|
|
||
| @Deprecated | ||
| public static void maybeClearAll(@Nonnull Run<?,?> build) throws IOException { | ||
| try { | ||
| maybeClearAll(build, TaskListener.NULL); | ||
| } catch (InterruptedException x) { | ||
| throw new IOException(x); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Delete any and all stashes in a build unless told otherwise. | ||
| * {@link StashBehavior#shouldClearAll} may cancel this. | ||
| * @param build a build possibly passed to {@link #stash} in the past | ||
| * @see #clearAll(Run, TaskListener) | ||
| */ | ||
| public static void maybeClearAll(@Nonnull Run<?,?> build, @Nonnull TaskListener listener) throws IOException, InterruptedException { | ||
| for (StashBehavior behavior : ExtensionList.lookup(StashBehavior.class)) { | ||
| if (!behavior.shouldClearAll(build)) { | ||
| return; | ||
| } | ||
| } | ||
| clearAll(build, listener); | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated without replacement; only used from {@link CopyStashesAndArtifacts} anyway | ||
| */ | ||
| @Deprecated | ||
| public static void copyAll(@Nonnull Run<?,?> from, @Nonnull Run<?,?> to) throws IOException { | ||
| File fromStorage = storage(from); | ||
| if (!fromStorage.isDirectory()) { | ||
|
|
@@ -166,7 +225,7 @@ public static void copyAll(@Nonnull Run<?,?> from, @Nonnull Run<?,?> to) throws | |
| FileUtils.copyDirectory(fromStorage, storage(to)); | ||
| } | ||
|
|
||
| @Restricted(DoNotUse.class) // just for tests, and incompatible with StashAwareArtifactManager | ||
| @SuppressFBWarnings(value="DM_DEFAULT_ENCODING", justification="test code") | ||
| public static Map<String,Map<String,String>> stashesOf(@Nonnull Run<?,?> build) throws IOException { | ||
| Map<String,Map<String,String>> result = new TreeMap<String,Map<String,String>>(); | ||
|
|
@@ -196,11 +255,12 @@ public static Map<String,Map<String,String>> stashesOf(@Nonnull Run<?,?> build) | |
| return result; | ||
| } | ||
|
|
||
| private static @Nonnull File storage(@Nonnull Run<?,?> build) throws IOException { | ||
| assert stashAwareArtifactManager(build) == null; | ||
| return new File(build.getRootDir(), "stashes"); | ||
| } | ||
|
|
||
| private static @Nonnull File storage(@Nonnull Run<?,?> build, @Nonnull String name) throws IOException { | ||
| File dir = storage(build); | ||
| File f = new File(dir, name + SUFFIX); | ||
| if (!f.getParentFile().equals(dir)) { | ||
|
|
@@ -229,16 +289,89 @@ public boolean shouldClearAll(@Nonnull Run<?,?> build) { | |
|
|
||
| } | ||
|
|
||
| /** | ||
| * Mixin interface for an {@link ArtifactManager} which supports specialized stash behavior as well. | ||
| * <p>The recommended standard implementation is {@code JCloudsArtifactManager} | ||
|
||
| * in the plugin currently named {@code artifact-manager-s3}. | ||
| * This in turn supports extensibility to various cloud providers, | ||
| * and handles all aspects of making cloud artifact storage work smoothly in Jenkins | ||
| * including the {@link VirtualFile} implementation, robust network error handling, | ||
| * overall configuration UI, and more. | ||
| * Implement this interface directly at your own risk. | ||
| * @see <a href="https://github.com/jenkinsci/jep/blob/master/jep/202/README.adoc">JEP-202</a> | ||
| */ | ||
| @Restricted(Beta.class) | ||
| public interface StashAwareArtifactManager /* extends ArtifactManager */ { | ||
|
Member
There was a problem hiding this comment. 🐛 I think this API design is not generally safe if it's being leveraged for external artifact storage: the API is exposed directly but it relies on the invoker or implementation to provide all necessary timeouts, retries, etc that you'd have for networked operations to render them safe. This leaves me deeply concerned, because this is an extension point we would expect multiple implementations of (one for each artifact/stash store) and it would be very easy for users with poor implementations to break Jenkins. In fact I expect that most implementations will not account for network hangs/disconnects/etc due to the complexity. In my opinion this needs to include JENKINS-50597 as part of its design at the API level, one way or another. Either we wrap One might ask "but won't we be including network protections for local operations???" -- and to that I say "local isn't always as local as you think -- consider NFS mounts which appear to be local storage." Plus applying the same options to local disk actions may catch and prevent some issues we see due to flaky storage (again, usually networked).
Member
Author
There was a problem hiding this comment. There is only one foreseen implementation of this beta SPI in Essentials: Nor is baking protections into the API feasible. The specifics of JClouds semantics are interleaved with Remoting boundaries. Modifying behavior of local artifact storage is out of scope. No one that I know of has complained about such issues with the current implementation, this PR does not touch it, and our policy moving forward will be to recommend external storage anyway.
Member
There was a problem hiding this comment. @jglick Some points:
This assumption raises a red flag, because if feels like we're getting tunnel vision about shipping one implementation and setting up for future problems. I can totally understand the desire to just release, but if you want external storage to be broadly used, the APIs for it should be built for the future uses. If you only want one API consumer for this API, it should be locked down as much as possible to prevent others and at the very least documented that "you should extend this other thing here, extending this is dangerous". Otherwise the API should be built in a way that is appropriate to handle extension. Now, if the expected behavior is that plugin authors will always extend off JCloud APIs for external storage implementations, I have no problem with making that the case, but again we need to make that clear and do what we can to keep this locked-down -- and ensure again that failures are propagated.
That is going to need a proper explanation, I'm afraid -- on the surface it smells to me like we're letting an implementation detail dictate API design rather than trying to build for robustness and then figure out implementation.
You mean, aside from general broken-ness with IO operations when an NFS server gets overloaded? I don't think anybody is saying that's a hard requirement in this work, just that if our protections extend to that as a side-effect it won't be a problem and in fact may be beneficial.
Which will require a rich ecosystem of implementations for different technologies, given how many different Jenkins deployment architectures there are. Which again means that we need to build those base APIs to be reusable and safe for extension. |
||
|
|
||
| /** @see StashManager#stash(Run, String, FilePath, Launcher, EnvVars, TaskListener, String, String, boolean, boolean) */ | ||
| void stash(@Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener, @CheckForNull String includes, @CheckForNull String excludes, boolean useDefaultExcludes, boolean allowEmpty) throws IOException, InterruptedException; | ||
|
Member
There was a problem hiding this comment. Again, we should really be looking at an options object to reduce the parameter complexity. |
||
|
|
||
| /** @see StashManager#unstash(Run, String, FilePath, Launcher, EnvVars, TaskListener) */ | ||
| void unstash(@Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws IOException, InterruptedException; | ||
|
|
||
| /** @see StashManager#clearAll(Run, TaskListener) */ | ||
| void clearAllStashes(@Nonnull TaskListener listener) throws IOException, InterruptedException; | ||
|
|
||
| /** | ||
| * Copy all stashes and artifacts from one build to another. | ||
| * The {@link ArtifactManager} configuration will be as of the origin build. | ||
| * If the implementation cannot handle {@code to} for whatever reason, it may throw {@link AbortException}. | ||
| * @see CopyStashesAndArtifacts | ||
| */ | ||
| void copyAllArtifactsAndStashes(@Nonnull Run<?,?> to, @Nonnull TaskListener listener) throws IOException, InterruptedException; | ||
|
|
||
| } | ||
|
|
||
| private static @CheckForNull StashAwareArtifactManager stashAwareArtifactManager(@Nonnull Run<?, ?> build) throws IOException { | ||
| ArtifactManager am = build.pickArtifactManager(); | ||
| return am instanceof StashAwareArtifactManager ? (StashAwareArtifactManager) am : null; | ||
| } | ||
|
|
||
| @Deprecated | ||
| private static @Nonnull Launcher launcherFor(@Nonnull FilePath workspace, @Nonnull TaskListener listener) { | ||
|
Member
There was a problem hiding this comment. Question: are there extension points around Launcher creation that we might want to support interaction with?
Member
Author
There was a problem hiding this comment. Yes, handled by |
||
| Computer c = workspace.toComputer(); | ||
| if (c != null) { | ||
| Node n = c.getNode(); | ||
| if (n != null) { | ||
| return n.createLauncher(listener); | ||
| } else { | ||
| listener.error(c.getDisplayName() + " seems to be offline"); | ||
| return new LocalLauncher(listener); | ||
| } | ||
| } else { | ||
| listener.error(workspace + " seems to be offline"); | ||
| return new LocalLauncher(listener); | ||
| } | ||
| } | ||
|
|
||
| @Deprecated | ||
| private static @Nonnull EnvVars envFor(@Nonnull Run<?, ?> build, @Nonnull FilePath workspace, @Nonnull TaskListener listener) throws IOException, InterruptedException { | ||
|
Member
There was a problem hiding this comment. Wait... why did we introduce a new deprecated method?
Member
Author
There was a problem hiding this comment. See above, this is dead code for binary compatibility. |
||
| Computer c = workspace.toComputer(); | ||
| if (c != null) { | ||
| EnvVars e = c.getEnvironment(); | ||
| e.putAll(c.buildEnvironment(listener)); | ||
| e.putAll(build.getEnvironment(listener)); | ||
| return e; | ||
| } else { | ||
| listener.error(workspace + " seems to be offline"); | ||
| return new EnvVars(); | ||
| } | ||
| } | ||
|
|
||
| @Restricted(NoExternalUse.class) | ||
| @Extension public static class CopyStashesAndArtifacts extends FlowCopier.ByRun { | ||
|
|
||
| @Override public void copy(Run<?,?> original, Run<?,?> copy, TaskListener listener) throws IOException, InterruptedException { | ||
| StashAwareArtifactManager saam = stashAwareArtifactManager(original); | ||
| if (saam != null) { | ||
| saam.copyAllArtifactsAndStashes(copy, listener); | ||
| return; | ||
| } | ||
| VirtualFile srcroot = original.getArtifactManager().root(); | ||
| FilePath dstDir = createTmpDir(); | ||
| try { | ||
| Map<String,String> files = new HashMap<>(); | ||
| for (String path : srcroot.list("**/*", null, false)) { | ||
| files.put(path, path); | ||
| InputStream in = srcroot.child(path).open(); | ||
| try { | ||
|
|
||
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.
Welp, I guess that's where I cut the "stable" branch and switch default merge branch to point to it except when the new master is needed.