[JENKINS-49635] VirtualFile support

[jglick]

Downstream of jenkinsci/jenkins#3302 & jenkinsci/plugin-pom#98.

• reusable test facility
• StashManager integration
• stronger tests

@reviewbybees

[svanoort]

🐛 Hard nope for me right here. We do not accept gratuitous core bumps especially to such a bleeding edge core versions because it prevents anybody from being able to consume new features and bugfixes to the pipeline plugins.

[abayer]

I agree. At a minimum this needs to hit LTS first.

[svanoort]

Prefer 2 LTS cycles because otherwise it's hard to ship fixes to many of the users (especially important for urgent issues) that tend to run an LTS behind but yeah, 1 LTS is the bare minimum for me too -- should be reserved for critical fixes and super high-value features generally though.

[svanoort]

Unmerge-able due to dependency on an unreleased Jenkins core. Please find a way to accomplish this that does NOT stand in the way of plugin development (or force massive amounts of backporting for all new work).

[svanoort]

Note: that's only a time-sensitive merge-block, not a permanent one. I'm assuming that this is a speculative WIP PR so that won't be an issue and by the time it's release-ready the dependency will have been in an LTS or two.

[jglick]

Well, no. See JEP-300.

Anyway that is parenthetical—obviously no one was proposing a PR like this be merged today. Please reserve PR reviews to substantive matters of the code change itself to avoid visual clutter. It is helpful to create (I think conventionally orange?) on-hold labels as a marker for PRs which are not eligible for immediate merging to their target branch for logistical reasons, at least until Essentials is live.

[oleg-nenashev]

The code mostly looks good to me. Environment management looks quite suspicious, but it's a wider issue in Jenkins.

[oleg-nenashev]

Not sure about the current implementation. It builds computer environment variables, but ignores possible overrides by the Run's logic. I hope it does not impact artifact managers, but I can imagine withEnvVars() closure overriding some variables for the external artifact manager. E.g. just tool() installation for the artifact management binary.

Maybe an edge case

[jglick]

Yes this could be improved I think—will check. Anyway this method is only called when using the deprecated overloads. As of jenkinsci/workflow-basic-steps-plugin#60 the actual EnvVars is loaded from the step context, for example taking into account withEnv blocks.

Added on-hold label to remind viewers of current unmergeability.

[carlossg]

I would split this, at least another for StashAwareArtifactManager, otherwise we only get the first failure

[vivek]

As of jenkinsci/jep#117 plus recent edits here, Javadoc and JEP both already link to recommended implementation strategies, and all relevant APIs are marked beta. Anyone who deliberately bypasses beta restrictions in a new plugin, does not consult with the authors of the APIs in question, and offers a poor-quality implementation is beyond our help. There is no reason to believe they would choose to implement any other intermediate API we offer anyway.

I think we can move forward with above consideration. @svanoort this sounds good to you? Lets get moving on this PR :)

[svanoort]

@vivek I'm just waiting on some docs from @jglick as promised somewhere way above and then yeah, I'll do the dance to set up the master/stable branches, merge this to master, and set the merge target as stable

[vivek]

Thanks @svanoort!

[carlossg]

We will only ever care about storing external artifacts in cloud blob stores (AWS S3 and similar implementations in Azure, Google Cloud, etc) that are supported via the JClouds library.

That is not accurate, implementors may choose their own libraries. JClouds makes it easier but other libraries may be more optimized, so it's up to future implementors to choose, but that's not the API decision

If community members decide to implement their own external storage solutions on top of the new API here, we are not responsible for any harm they do to Jenkins or the Pipeline ecosystem

correct, we can't predict the future

I raised a concern that this set of APIs is so tightly coupled to JClouds APIs that it cannot be used for artifact repositories such as Artifactory and Nexus

I don't think it is coupled to JClouds but to blobstores. I'd rather not over engineer a solution for artifact repositories until there is any interest from those parties, and they might as well ignore this as using mvn deploy or such would make it unnecessary

I have proposed a compromise, which is to define one additional level of ExtensionPoint above the JCloudsArtifactManager that plugin authors can extend. This would extend StashAwareArtifactManager and provide a minimal set of logic for failure detection and handling - probably hooks to wrap an HTTP call with RobustHttpClient, maybe a top-level timeout and retry for operations.

I don't agree that retry logic should live in pipeline. This approach would assume that other cloud libraries don't already have some retry logic or that use a specific http client, which may not hold true. If we see other implementations copy that logic then we can move it up or sideways to a common utilities plugin

when community members see the implementation and try to roll their own storage implementation

again I don't think we should over engineer it, the first implementation complies with requirements, there is no timeframe for new implementations and this is considered Beta to bring other interested parties

The entire point of marking newly introduced APIs with the beta annotation is so that we can ship working features without needing this kind of discussion of hypotheticals, do development in master, and refactor later if and when our initial assumptions are proven wrong.

agreed

[jglick]

(503 from repo.azure.jenkins.io)

[svanoort]

@jglick This is still not what we had discussed.

[svanoort]

Still did not apply the requested docs change, just reworded some unrelated things.

[svanoort]

@vivek Still blocked here due to not making the requested and trivial change. I noted this would be ready to merge:

once the "you should extend this other thing here, extending this is dangerous" expectation is clearly spelled out in Javadocs and documentation - both in this PR (warning: "implement StashAwareArtifactManager at your own risk if you use network storage") and in the downstream implementation.

Instead a few trivial rewordings were done.

I've set a very low bar here for merging what I consider to be a risky change and yet still someone felt they did not need to meet it?

[vivek]

@svanoort PTAL, I think its good to go.

[svanoort]

It's getting closer but I still want it to be very explicit about the dangerous bits and what they are. This one is risky enough that we can't afford to be cryptic about intent.

Something like this would be preferred:

"For off-master artifact storage, you should NOT extent this directly but instead use the $JCloudImpl by extending its BlobStore. This is dangerous to directly extend if using remote storage unless you write a very robust handling of network failures including at least a base timeout and retries."

Saying a standard implementation exists does not make it clear that you must be basing off that. Thanks!