From 63f655f11c9be6e79a5f9979dbfa3c2984f3ad2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20H=C3=A4user?= Date: Tue, 3 Apr 2018 15:16:47 +0200 Subject: [PATCH 1/3] Synchronise on project I am not sure this makes any sense. I am just trying to solve this issue: https://issues.jenkins-ci.org/browse/JENKINS-41880 My thoughts: This is likely caused by changing the finger prints, the finger prints are scoped to the project (right?). If this is the case synchronising on the project could make sense. Also `addFromFacet` should not be a performance killer and rather quick. The alternative would be for me to synchronise the complete access to the method. WDYT? --- .../fingerprint/DockerFingerprintAction.java | 12 +-- .../fingerprint/DockerFingerprints.java | 82 ++++++++++--------- 2 files changed, 48 insertions(+), 46 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java index a23189bc..cf710579 100644 --- a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java +++ b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java @@ -96,20 +96,20 @@ public Set getImageIDs() { public @CheckForNull String getFingerprintHash(@CheckForNull String imageId) { return (imageId != null) ? DockerFingerprints.getFingerprintHash(imageId) : null; } - + @Restricted(NoExternalUse.class) public @CheckForNull Fingerprint getFingerprint(@CheckForNull String imageId) { if (imageId == null) { return null; } - + try { return DockerFingerprints.of(imageId); } catch (IOException ex) { return null; // nothing to do in web UI - return null as well } } - + public List getDockerFacets(String imageId) { List res = new LinkedList(); final Fingerprint fp = getFingerprint(imageId); @@ -125,9 +125,9 @@ public List getDockerFacets(String imageId) { /** * Adds an action with a reference to fingerprint if required. It's - * recommended to call the method from { + * recommended to call the method from {@link hudson.BulkChange} + * transaction to avoid saving the {@link Run} multiple times. * - * @BulkChange} transaction to avoid saving the {@link Run} multiple times. * @param fp Fingerprint * @param imageId ID of the docker image * @param run Run to be updated @@ -140,7 +140,7 @@ static void addToRun(Fingerprint fp, String imageId, Run run) throws IOException action = new DockerFingerprintAction(); run.addAction(action); } - + if (action.imageIDs.add(imageId)) { run.save(); } // else no need to save updates diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java index 047b4e40..c7a1aea8 100644 --- a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java +++ b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java @@ -26,8 +26,12 @@ import hudson.BulkChange; import hudson.model.Fingerprint; import hudson.model.Run; +import jenkins.model.FingerprintFacet; import jenkins.model.Jenkins; +import org.apache.commons.lang.StringUtils; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; import java.io.IOException; import java.util.Collection; import java.util.Collections; @@ -35,10 +39,6 @@ import java.util.List; import java.util.logging.Level; import java.util.logging.Logger; -import javax.annotation.CheckForNull; -import javax.annotation.Nonnull; -import jenkins.model.FingerprintFacet; -import org.apache.commons.lang.StringUtils; /** * Entry point into fingerprint related functionalities in Docker. @@ -254,55 +254,57 @@ public static void addRunFacet(@Nonnull ContainerRecord record, @Nonnull Run run) throws IOException { - long timestamp = System.currentTimeMillis(); - if (ancestorImageId != null) { - Fingerprint f = forImage(run, ancestorImageId); + synchronized (run.getParent()) { + long timestamp = System.currentTimeMillis(); + if (ancestorImageId != null) { + Fingerprint f = forImage(run, ancestorImageId); + Collection facets = f.getFacets(); + DockerDescendantFingerprintFacet descendantFacet = null; + for (FingerprintFacet facet : facets) { + if (facet instanceof DockerDescendantFingerprintFacet) { + descendantFacet = (DockerDescendantFingerprintFacet) facet; + break; + } + } + BulkChange bc = new BulkChange(f); + try { + if (descendantFacet == null) { + descendantFacet = new DockerDescendantFingerprintFacet(f, timestamp, ancestorImageId); + facets.add(descendantFacet); + } + descendantFacet.addDescendantImageId(descendantImageId); + descendantFacet.addFor(run); + DockerFingerprintAction.addToRun(f, ancestorImageId, run); + bc.commit(); + } finally { + bc.abort(); + } + } + Fingerprint f = forImage(run, descendantImageId); Collection facets = f.getFacets(); - DockerDescendantFingerprintFacet descendantFacet = null; + DockerAncestorFingerprintFacet ancestorFacet = null; for (FingerprintFacet facet : facets) { - if (facet instanceof DockerDescendantFingerprintFacet) { - descendantFacet = (DockerDescendantFingerprintFacet) facet; + if (facet instanceof DockerAncestorFingerprintFacet) { + ancestorFacet = (DockerAncestorFingerprintFacet) facet; break; } } BulkChange bc = new BulkChange(f); try { - if (descendantFacet == null) { - descendantFacet = new DockerDescendantFingerprintFacet(f, timestamp, ancestorImageId); - facets.add(descendantFacet); + if (ancestorFacet == null) { + ancestorFacet = new DockerAncestorFingerprintFacet(f, timestamp, descendantImageId); + facets.add(ancestorFacet); + } + if (ancestorImageId != null) { + ancestorFacet.addAncestorImageId(ancestorImageId); } - descendantFacet.addDescendantImageId(descendantImageId); - descendantFacet.addFor(run); - DockerFingerprintAction.addToRun(f, ancestorImageId, run); + ancestorFacet.addFor(run); + DockerFingerprintAction.addToRun(f, descendantImageId, run); bc.commit(); } finally { bc.abort(); } } - Fingerprint f = forImage(run, descendantImageId); - Collection facets = f.getFacets(); - DockerAncestorFingerprintFacet ancestorFacet = null; - for (FingerprintFacet facet : facets) { - if (facet instanceof DockerAncestorFingerprintFacet) { - ancestorFacet = (DockerAncestorFingerprintFacet) facet; - break; - } - } - BulkChange bc = new BulkChange(f); - try { - if (ancestorFacet == null) { - ancestorFacet = new DockerAncestorFingerprintFacet(f, timestamp, descendantImageId); - facets.add(ancestorFacet); - } - if (ancestorImageId != null) { - ancestorFacet.addAncestorImageId(ancestorImageId); - } - ancestorFacet.addFor(run); - DockerFingerprintAction.addToRun(f, descendantImageId, run); - bc.commit(); - } finally { - bc.abort(); - } } } From a6087704250d6a100403e3bf27e215d89bede12a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20H=C3=A4user?= Date: Thu, 5 Apr 2018 08:31:44 +0200 Subject: [PATCH 2/3] Revert whitespace changes --- .../fingerprint/DockerFingerprintAction.java | 12 +-- .../fingerprint/DockerFingerprints.java | 82 +++++++++---------- 2 files changed, 46 insertions(+), 48 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java index cf710579..a23189bc 100644 --- a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java +++ b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprintAction.java @@ -96,20 +96,20 @@ public Set getImageIDs() { public @CheckForNull String getFingerprintHash(@CheckForNull String imageId) { return (imageId != null) ? DockerFingerprints.getFingerprintHash(imageId) : null; } - + @Restricted(NoExternalUse.class) public @CheckForNull Fingerprint getFingerprint(@CheckForNull String imageId) { if (imageId == null) { return null; } - + try { return DockerFingerprints.of(imageId); } catch (IOException ex) { return null; // nothing to do in web UI - return null as well } } - + public List getDockerFacets(String imageId) { List res = new LinkedList(); final Fingerprint fp = getFingerprint(imageId); @@ -125,9 +125,9 @@ public List getDockerFacets(String imageId) { /** * Adds an action with a reference to fingerprint if required. It's - * recommended to call the method from {@link hudson.BulkChange} - * transaction to avoid saving the {@link Run} multiple times. + * recommended to call the method from { * + * @BulkChange} transaction to avoid saving the {@link Run} multiple times. * @param fp Fingerprint * @param imageId ID of the docker image * @param run Run to be updated @@ -140,7 +140,7 @@ static void addToRun(Fingerprint fp, String imageId, Run run) throws IOException action = new DockerFingerprintAction(); run.addAction(action); } - + if (action.imageIDs.add(imageId)) { run.save(); } // else no need to save updates diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java index c7a1aea8..047b4e40 100644 --- a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java +++ b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java @@ -26,12 +26,8 @@ import hudson.BulkChange; import hudson.model.Fingerprint; import hudson.model.Run; -import jenkins.model.FingerprintFacet; import jenkins.model.Jenkins; -import org.apache.commons.lang.StringUtils; -import javax.annotation.CheckForNull; -import javax.annotation.Nonnull; import java.io.IOException; import java.util.Collection; import java.util.Collections; @@ -39,6 +35,10 @@ import java.util.List; import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; +import jenkins.model.FingerprintFacet; +import org.apache.commons.lang.StringUtils; /** * Entry point into fingerprint related functionalities in Docker. @@ -254,57 +254,55 @@ public static void addRunFacet(@Nonnull ContainerRecord record, @Nonnull Run run) throws IOException { - synchronized (run.getParent()) { - long timestamp = System.currentTimeMillis(); - if (ancestorImageId != null) { - Fingerprint f = forImage(run, ancestorImageId); - Collection facets = f.getFacets(); - DockerDescendantFingerprintFacet descendantFacet = null; - for (FingerprintFacet facet : facets) { - if (facet instanceof DockerDescendantFingerprintFacet) { - descendantFacet = (DockerDescendantFingerprintFacet) facet; - break; - } - } - BulkChange bc = new BulkChange(f); - try { - if (descendantFacet == null) { - descendantFacet = new DockerDescendantFingerprintFacet(f, timestamp, ancestorImageId); - facets.add(descendantFacet); - } - descendantFacet.addDescendantImageId(descendantImageId); - descendantFacet.addFor(run); - DockerFingerprintAction.addToRun(f, ancestorImageId, run); - bc.commit(); - } finally { - bc.abort(); - } - } - Fingerprint f = forImage(run, descendantImageId); + long timestamp = System.currentTimeMillis(); + if (ancestorImageId != null) { + Fingerprint f = forImage(run, ancestorImageId); Collection facets = f.getFacets(); - DockerAncestorFingerprintFacet ancestorFacet = null; + DockerDescendantFingerprintFacet descendantFacet = null; for (FingerprintFacet facet : facets) { - if (facet instanceof DockerAncestorFingerprintFacet) { - ancestorFacet = (DockerAncestorFingerprintFacet) facet; + if (facet instanceof DockerDescendantFingerprintFacet) { + descendantFacet = (DockerDescendantFingerprintFacet) facet; break; } } BulkChange bc = new BulkChange(f); try { - if (ancestorFacet == null) { - ancestorFacet = new DockerAncestorFingerprintFacet(f, timestamp, descendantImageId); - facets.add(ancestorFacet); - } - if (ancestorImageId != null) { - ancestorFacet.addAncestorImageId(ancestorImageId); + if (descendantFacet == null) { + descendantFacet = new DockerDescendantFingerprintFacet(f, timestamp, ancestorImageId); + facets.add(descendantFacet); } - ancestorFacet.addFor(run); - DockerFingerprintAction.addToRun(f, descendantImageId, run); + descendantFacet.addDescendantImageId(descendantImageId); + descendantFacet.addFor(run); + DockerFingerprintAction.addToRun(f, ancestorImageId, run); bc.commit(); } finally { bc.abort(); } } + Fingerprint f = forImage(run, descendantImageId); + Collection facets = f.getFacets(); + DockerAncestorFingerprintFacet ancestorFacet = null; + for (FingerprintFacet facet : facets) { + if (facet instanceof DockerAncestorFingerprintFacet) { + ancestorFacet = (DockerAncestorFingerprintFacet) facet; + break; + } + } + BulkChange bc = new BulkChange(f); + try { + if (ancestorFacet == null) { + ancestorFacet = new DockerAncestorFingerprintFacet(f, timestamp, descendantImageId); + facets.add(ancestorFacet); + } + if (ancestorImageId != null) { + ancestorFacet.addAncestorImageId(ancestorImageId); + } + ancestorFacet.addFor(run); + DockerFingerprintAction.addToRun(f, descendantImageId, run); + bc.commit(); + } finally { + bc.abort(); + } } } From cca5639d6705d49302c5e333d4d55bb4db5625b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20H=C3=A4user?= Date: Thu, 5 Apr 2018 08:29:56 +0200 Subject: [PATCH 3/3] Synchronize on fingerprint --- .../docker/commons/fingerprint/DockerFingerprints.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java index 047b4e40..95c62f42 100644 --- a/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java +++ b/src/main/java/org/jenkinsci/plugins/docker/commons/fingerprint/DockerFingerprints.java @@ -257,6 +257,7 @@ public static void addFromFacet(@CheckForNull String ancestorImageId, @Nonnull S long timestamp = System.currentTimeMillis(); if (ancestorImageId != null) { Fingerprint f = forImage(run, ancestorImageId); + synchronized (f) { Collection facets = f.getFacets(); DockerDescendantFingerprintFacet descendantFacet = null; for (FingerprintFacet facet : facets) { @@ -278,8 +279,10 @@ public static void addFromFacet(@CheckForNull String ancestorImageId, @Nonnull S } finally { bc.abort(); } + } } Fingerprint f = forImage(run, descendantImageId); + synchronized (f) { Collection facets = f.getFacets(); DockerAncestorFingerprintFacet ancestorFacet = null; for (FingerprintFacet facet : facets) { @@ -303,6 +306,7 @@ public static void addFromFacet(@CheckForNull String ancestorImageId, @Nonnull S } finally { bc.abort(); } + } } }