Add ability to disable fingerprint recording #85

jonsten · 2020-01-08T09:25:33Z

It seems like the fingerprinting functionality has been broken for a while (e.g. see this comment by Jesse Glick) and causes a lot of problems, especially in large Jenkins instances. This change introduces a property which disables the functionality all together.

Some background, in our case, where we run 10k+ builds a day using the same docker image, the fingerprint file for that image grows very large 50+ MB and around 100k container entries. The large size causes Jenkins to spend more and more time saving the file (serialization, file IO, etc). In the end, when it gets large enough, the builds are aborted without anything being written in the logs (separate issue, but it seems like if more than five minutes is spent in the step start / fingerprint recording phase, then the build is terminated without anything written in the log). At that size it takes around 3-4 seconds to save the file, and if 100 builds are starting containers at the same time, then some of them will spend more than five minutes waiting.

It seems like the fingerprinting functionality has been broken for a while and causes a lot of problems, especially in large Jenkins instances. This change introduces a property which disables the functionality all together.

oleg-nenashev

I agree with such approach in general. It would be great to disable Docker fingerprint recording. It might make sense to offer it as a Global configuration, but system property is acceptable as well IMHO.

I am requesting changes, because the new option needs to be documented in README.

I would also suggest adding...

Javadoc for the new property
New Static Getter method. It will be useful for Dwnstream plugins, because they can optimize their logic when fingerprint recording is disabled
TBD - Web UI configuration options?

src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java

jonsten · 2020-01-08T10:06:25Z

Thank you for the quick feedback!

I opted for the system property as it is the "quick" solution and it doesn't bloat system configuration with yet another checkbox. With that said I agree with you that this might be a rather useful setting. In fact, in a perfect world it should be possible to set from case to case, e.g. same way as it is done for artifacts. Unfortunately I'm not willing to spend the effort to implement all of that. I will see if there is a natural place to add a system config, if not I will go with the system property route.

I am requesting changes, because the new option needs to be documented in README.

Definitely I'll fix!

Javadoc for the new property

I'll fix!

New Static Getter method. It will be useful for Dwnstream plugins, because they can optimize their logic when fingerprint recording is disabled

Shouldn't they be able to use the field directly or that is a no-go way to solve it?

oleg-nenashev · 2020-01-08T10:11:48Z

Shouldn't they be able to use the field directly or that is a no-go way to solve it?

It is also possible, but it will complicate further changes should we decide to change/improve behavior later (e.g. adding a global configuration). Static methods are more convenient from the extensibility PoV

jonsten · 2020-01-08T10:14:34Z

Shouldn't they be able to use the field directly or that is a no-go way to solve it?

It is also possible, but it will complicate further changes should we decide to change/improve behavior later (e.g. adding a global configuration). Static methods are more convenient from the extensibility PoV

Got ya, I'll add a static method!

As for global config, I see no natural home for such an option, so, let's start with the property for now. Adding information in the README and in code should be enough for people to find it if they start to run into performance problems.

Base of review feedback.

oleg-nenashev · 2020-01-08T12:13:22Z

src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java

@@ -221,6 +232,9 @@ private DockerFingerprints() {} // no instantiation
     * Adds a new {@link ContainerRecord} for the specified image, creating necessary intermediate objects as it goes.
     */
    public static void addRunFacet(@Nonnull ContainerRecord record, @Nonnull Run<?,?> run) throws IOException {
+        if (DISABLE) {
+            return;
+        }


Would it also make sense to add logging on the FINE or INFO levels?

I'm not the expert here, after all, I'm the one making a change to disable all of this, so I have a hard time finding a point where it would be useful. But if you can see a reason for adding it, I'll happily add it :) Besides, at level FINE/INFO it would require the user to actively set the log level in order to get it to the system logs etc (have it on by default would be way to chatty IMHO).

oleg-nenashev

All major comments are addressed, 👍 for merging

Besides, at level FINE/INFO it would require the user to actively set the log level in order to get it to the system logs etc (have it on by default would be way to chatty IMHO).

That's why it is FINE. We do not want it to be printed by default, unless a deep dive needed for diagnostics.

jglick

Should no longer be necessary as of jenkinsci/docker-workflow-plugin#180 in 1.19 plus jenkinsci/pipeline-model-definition-plugin#350 in 1.4.0.

jonsten · 2020-01-08T19:31:34Z

Should no longer be necessary as of jenkinsci/docker-workflow-plugin#180 in 1.19 plus jenkinsci/pipeline-model-definition-plugin#350 in 1.4.0.

Ummm okay, I've might have missed something, so please explain how I prevent fingerprint recording without this change when i run a scripted pipeline, e.g.:

def image = docker.image('some_image')
image.inside {
    ...
}

With the latest release of docker common and docker workflow plugin this code will record fingerprints.

Edit; Aha, I think I see it now, looking at jenkinsci/docker-workflow-plugin#180 it looks like inside command wasn't addressed.

jglick · 2020-01-08T20:08:59Z

Yes https://github.com/jenkinsci/docker-workflow-plugin/blob/74a2370901f41e8b5b541d768b440e2ab1cd1b18/src/main/java/org/jenkinsci/plugins/docker/workflow/WithContainerStep.java#L209 was forgotten, as were a couple lines in DockerDSL/help.jelly.

jonsten · 2020-01-08T20:27:59Z

Okay, then it sounds like I could scrap this change and instead remove the call to addRunFacet in WithContainerStep. I'll see what I can do tomorrow.

oleg-nenashev · 2020-01-08T21:57:04Z

@jglick @jonsten I think this is still relevant, because there are other plugins relying on Fingerprints. E.g. DockerHub Notifications IIRC. Pipeline is not the only API where it is used. One of the follow-ups for the Docker Traceability project in 2015 was to improve fingerprints performance, but it has never happened as you may guess :( https://jenkins.io/projects/gsoc/2020/project-ideas/external-fingerprint-storage-for-jenkins/ in GSoC may somewhat address it

jglick · 2020-01-08T23:33:26Z

I do not think other plugins produce Docker fingerprints. Anyway the request here was about docker-workflow.

jonsten · 2020-01-09T06:41:34Z

Based on what @jglick bought up I've created jenkinsci/docker-workflow-plugin#198 over at docker-workflow. I've also added the log entries you suggested @oleg-nenashev. I leave it to you guys to decide which one to merge (or both(, or none)).

Add ability to disable fingerprint recording

8a88427

It seems like the fingerprinting functionality has been broken for a while and causes a lot of problems, especially in large Jenkins instances. This change introduces a property which disables the functionality all together.

oleg-nenashev requested changes Jan 8, 2020

View reviewed changes

src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java Show resolved Hide resolved

Add documentation and helper method for fingerprint disabling

c6b86d3

Base of review feedback.

jonsten requested a review from oleg-nenashev January 8, 2020 10:50

oleg-nenashev reviewed Jan 8, 2020

View reviewed changes

oleg-nenashev approved these changes Jan 8, 2020

View reviewed changes

oleg-nenashev requested a review from jglick January 8, 2020 14:40

jglick requested changes Jan 8, 2020

View reviewed changes

jonsten mentioned this pull request Jan 9, 2020

Remove automatic fingerprinting from Image.inside() step. jenkinsci/docker-workflow-plugin#198

Merged

Print log message when fingerprinting is disabled

753d413

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to disable fingerprint recording #85

Add ability to disable fingerprint recording #85

jonsten commented Jan 8, 2020

oleg-nenashev left a comment

jonsten commented Jan 8, 2020

oleg-nenashev commented Jan 8, 2020

jonsten commented Jan 8, 2020

oleg-nenashev Jan 8, 2020

jonsten Jan 8, 2020

oleg-nenashev left a comment

jglick left a comment •

edited

Loading

jonsten commented Jan 8, 2020 •

edited

Loading

jglick commented Jan 8, 2020

jonsten commented Jan 8, 2020

oleg-nenashev commented Jan 8, 2020

jglick commented Jan 8, 2020

jonsten commented Jan 9, 2020

Add ability to disable fingerprint recording #85

Are you sure you want to change the base?

Add ability to disable fingerprint recording #85

Conversation

jonsten commented Jan 8, 2020

oleg-nenashev left a comment

Choose a reason for hiding this comment

jonsten commented Jan 8, 2020

oleg-nenashev commented Jan 8, 2020

jonsten commented Jan 8, 2020

oleg-nenashev Jan 8, 2020

Choose a reason for hiding this comment

jonsten Jan 8, 2020

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

jglick left a comment • edited Loading

Choose a reason for hiding this comment

jonsten commented Jan 8, 2020 • edited Loading

jglick commented Jan 8, 2020

jonsten commented Jan 8, 2020

oleg-nenashev commented Jan 8, 2020

jglick commented Jan 8, 2020

jonsten commented Jan 9, 2020

jglick left a comment •

edited

Loading

jonsten commented Jan 8, 2020 •

edited

Loading