Split image name into snapshot and release registries, with a separate path parameter #645

mccheah · 2020-02-06T20:06:43Z

In the docker image generator, we were originally publishing all built images to the same registry. It is however useful to publish snapshot versions of one's Spark images to one registry, and release versions to another registry. As such, the schema for the configuration is now as follows:

sparkDocker {
  baseImage <full path to base image, including the registry and tag>
  imagePath <The path component only of the image name>
  snapshotRegistry <The registry to use when pushing snapshot versions>
  releaseRegistry <The registry to use when pushing release versions>
  tags <set of tags to push>
}

We follow the SLS versioning specification to determine whether or not an image is being built on a release version or a snapshot version.

…e path parameter.

iamdanfox · 2020-02-06T20:11:34Z

...enerator/src/main/java/org/apache/spark/deploy/kubernetes/docker/gradle/DockerBuildTask.java

@@ -28,7 +28,7 @@

    private File dockerFile;
    private File dockerBuildDirectory;
-    private Property<String> imageName;
+    private Provider<String> imageName;


re-assigning a provider like this will give you quite weird behaviour if anyone uses the getImageName accessor and tries to .map it or access it later:

1. someone sets up a .map 2. someone else calls setImageName(..) which _replaces_ the internal provider field 3. the new value never makes it to person (1)

I'd recommend creating a private final Property here instead.

The problem is that the imageName is derived from the other fields in the extension (both registries + image path), and thus is not a property that can be set directly in the extension. We can set the three properties in each place we need the image name (this task, the tag-all task, and push task). But that would require deriving the image name from those fields each time. And Property#flatMap returns a Provider, not a Property. Thoughts?

Property has a setter which takes a provider, so it should be fine to set up all the derivation once (which should produce a provider) and then set this task's property to be hooked up to that imageName provider.

Ah, missed that - makes sense that these can remain as Property fields backed by the deriving Provider then. (Seems weird to keep having to go back and forth between Property and Provider and back to Property again.)

iamdanfox · 2020-02-06T20:12:14Z

...enerator/src/main/java/org/apache/spark/deploy/kubernetes/docker/gradle/DockerBuildTask.java

@@ -53,7 +53,7 @@ public final void setDockerBuildDirectory(File dockerBuildDirectory) {
        this.dockerBuildDirectory = dockerBuildDirectory;
    }

-    public final void setImageName(Property<String> imageName) {
+    public final void setImageName(Provider<String> imageName) {
        this.imageName = imageName;


when the imageName is a Property, you can use .set instead of assignment. This allows anyone who is observing the original value to notice when a new value has been 'set'.

But ImageName will not be initialized to anything (null) on the no-arg constructor of this task. I think it's fine to use setter for now - again looking to minimize changes from the original.

iamdanfox · 2020-02-06T20:12:50Z

...e-generator/src/main/java/org/apache/spark/deploy/kubernetes/docker/gradle/LazyExecTask.java

@@ -19,28 +19,28 @@
 import java.util.List;
 import java.util.stream.Collectors;
 import org.gradle.api.DefaultTask;
-import org.gradle.api.provider.Property;
+import org.gradle.api.provider.Provider;
 import org.gradle.api.tasks.Input;
 import org.gradle.api.tasks.TaskAction;

 public class LazyExecTask extends DefaultTask {


I think bog-standard exec tasks have a CommandLineArgumentProvider abstraction which might obviate this class?

I'm not too keen on trying to use this PR to do refactors and cleanups as well - only the minimal necessary to complete the task at hand.

We can file this as a TODO

iamdanfox · 2020-02-06T20:15:08Z

...erator/src/main/java/org/apache/spark/deploy/kubernetes/docker/gradle/SparkDockerPlugin.java

+                                                        project,
+                                                        imagePath,
+                                                        snapshotRegistry,
+                                                        releaseRegistry))));


I think it might be useful to put this logic as a read-only provider on the extension, because won't people need to grab the derived value you create and template it into their configuration.yml?

That could make sense, although we're having concerns about using a Provider in these fields at all, right? See #645 (comment). But this is what I meant about needing a Provider and not a Property because of the derivation matter.

mccheah · 2020-02-06T22:42:08Z

@iamdanfox I switched to using a Property instead of a Provider, that's automatically computed read-only by the extension. I didn't address some of the other things you have mentioned, since I think they belong in separate refactor PRs rather than in a PR that is introducing new functionality.

iamdanfox · 2020-02-07T11:32:18Z

...tor/src/main/java/org/apache/spark/deploy/kubernetes/docker/gradle/SparkDockerExtension.java

+                                                        project,
+                                                        resolvedImagePath,
+                                                        resolvedSnapshotRegistry,
+                                                        resolvedReleaseRegistry)))));


I think all the flatmaps and casting is a little more complicated than necessary. Why not just construct a fresh provider which just happens to call .get() on all the others?

project.provider(() -> { return ImageResolver.resolveImageName(p, imagePath.get(), snapshotRegistry.get() ... ); }

iamdanfox · 2020-02-07T11:50:28Z

...-generator/src/main/java/org/apache/spark/deploy/kubernetes/docker/gradle/ImageResolver.java

+        }
+        imageNameBuilder.append(imagePath);
+        return imageNameBuilder.toString();
+    }


Do you think it's worth doing some regex validation on the imagePath, just so that people don't leave values like docker.palantir/foo/bar in there and get some bizarre concatenation of everything?

I'm ok with not having validation here, again, keeping parity to how things were before - we weren't validating the scheme of the URI for the full image field.

…ase-registries

mccheah · 2020-02-10T20:49:23Z

@iamdanfox any further comments here?

iamdanfox

Looks great, thanks @mccheah!

mccheah added 2 commits February 6, 2020 12:01

Split image name into snapshot and release registries, with a separat…

a87cd6c

…e path parameter.

Remove unnecessary sls-packaging dependency

8fbaa4c

iamdanfox reviewed Feb 6, 2020

View reviewed changes

Address comments, revert unnecessary changes.

7fb1f9d

iamdanfox reviewed Feb 7, 2020

View reviewed changes

mccheah added 2 commits February 7, 2020 11:15

Merge remote-tracking branch 'palantir/master' into snapshot-and-rele…

d269295

…ase-registries

Use project.provider

9e65eb0

iamdanfox approved these changes Feb 10, 2020

View reviewed changes

mccheah merged commit b5c101e into master Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split image name into snapshot and release registries, with a separate path parameter #645

Split image name into snapshot and release registries, with a separate path parameter #645

mccheah commented Feb 6, 2020 •

edited

Loading

iamdanfox Feb 6, 2020

mccheah Feb 6, 2020

iamdanfox Feb 6, 2020 •

edited

Loading

mccheah Feb 6, 2020

iamdanfox Feb 6, 2020

mccheah Feb 6, 2020

iamdanfox Feb 6, 2020

mccheah Feb 6, 2020

mccheah Feb 6, 2020

iamdanfox Feb 6, 2020

mccheah Feb 6, 2020

mccheah commented Feb 6, 2020

iamdanfox Feb 7, 2020

iamdanfox Feb 7, 2020

mccheah Feb 7, 2020

mccheah commented Feb 10, 2020

iamdanfox left a comment

Split image name into snapshot and release registries, with a separate path parameter #645

Split image name into snapshot and release registries, with a separate path parameter #645

Conversation

mccheah commented Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamdanfox Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccheah commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccheah commented Feb 10, 2020

iamdanfox left a comment

Choose a reason for hiding this comment

mccheah commented Feb 6, 2020 •

edited

Loading

iamdanfox Feb 6, 2020 •

edited

Loading