Save information about child jobs in parent job #13

Jimilian · 2017-06-27T15:37:14Z

In some cases it would be nice to know what exactly was triggered by pipeline job. I.e. this information can be used by Build Failure Analyzer Plugin to show failures in child jobs on the top trigger job.

oleg-nenashev · 2017-06-27T15:53:40Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+import java.util.List;
+
+
+public class BuildInfoAction extends InvisibleAction {


I see no problem with making it visible, but maybe it's outside the scope

I can put this comment to the code ;)

oleg-nenashev · 2017-06-27T15:54:24Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+    }
+
+    public List<BuildInfo> getBuildInfoList() {
+        return buildInfoList;


Wrap with unmodifyableList()

oleg-nenashev · 2017-06-27T15:55:13Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+    }
+
+    public void addBuildInfo(String projectName, int buildNumber) {
+        buildInfoList.add(new BuildInfo(projectName, buildNumber));


It seems there may be concurrency issues with this field. I would recommend CopyOnWrite list or synchronize

CopyOnWrite looks like overkill, synchronize needs some manual work, so, I picked up ConcurrentLinkedQueue (current implementation of this action doesn't require random access).

oleg-nenashev · 2017-06-27T15:57:35Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+        private final String projectName;
+        private final int buildNumber;
+
+        public BuildInfo(String projectName, int buildNumber) {


This approach may start leaking job existence fact if the API user has no Item.Discover permission. Not sure if it's acceptable

How can I prevent this?
Btw, is it possible that user has access to list of actions of some build, but doesn't have access to the log of this build? (right now plugin publishes names/build numbers of triggered jobs to the log).

For now I made this class private and only list of runs can be retrieved from action. I hope that it would be safer.

* Make BuildInfoAction thread-safe * Create method to provide list of child runs * Add tests for BuildInfoAction

Jimilian · 2017-06-28T13:37:24Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java

@@ -29,6 +30,15 @@ public void onStarted(Run<?, ?> run, TaskListener listener) {
                    TaskListener taskListener = stepContext.get(TaskListener.class);
                    // encodeTo(Run) calls getDisplayName, which does not include the project name.
                    taskListener.getLogger().println("Starting building: " + ModelHyperlinkNote.encodeTo("/" + run.getUrl(), run.getFullDisplayName()));
+                    FlowNode parentNode = stepContext.get(FlowNode.class);
+                    if (parentNode != null) {


@oleg-nenashev, I suppose that this code is not thread-safe as well. Is it correct?
I think to put synchronized(this) for addAction/getAction

Jimilian · 2017-07-03T12:07:12Z

@oleg-nenashev, @jglick, can you take a look, please?

Jimilian · 2017-07-20T21:20:24Z

@jglick, will you have time to review this change? Should I rebase it first?

oleg-nenashev

LGTM

oleg-nenashev · 2017-07-20T21:26:31Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+    public List<Run<?, ?>> getChildBuilds() {
+        List<Run<?, ?>> builds = new ArrayList<>();
+
+        for (BuildInfo buildInfo : buildInfos) {


Minor optimization: Return Collections.emptyList() if the size is null before creating a new ArrayList. Maybe also makes sense to pre-initialize builds size to buildInfos.size

Not sure if ConcurrentLinkedQueue.size() is fast enough though. Maybe it will hammer the performance

ConcurrentLinkedQueue.isEmpty() comes for free, but in current implementation it's not possible to have empty collection of buildInfos longer than several ms (+ StopTheWorld pause :) ). Anyway, I added this check anyway - who knows how this Action will be used in future.
Pre-initialize - good idea, can decrease memory footprint a little bit.

jglick

A few technical mistakes which could be easily corrected. More broadly, the intent of this PR is unclear, since it adds no user-visible functionality. If it is intended as an API for other plugins to consume, there would need to be at least one downstream PR demonstrating the integrated functionality, which could be used to evaluate the suitability of the API.

jglick · 2017-07-31T13:57:00Z

src/test/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoActionTest.java

+            Thread.sleep(100);
+        }
+
+        assertEquals(501, buildInfoAction.getChildBuilds().size());


This test does not seem to be proving anything very interesting. I would just delete it. (PowerMock is a pain in the butt to maintain for Jenkins plugins anyway.)

Right now I used PowerMock only for Jenkins singleton, for the rest of stuff I used Mockito. What do you prefer to use for mocking Jenkins.getInstance method?

jglick · 2017-07-31T13:57:46Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+    BuildInfoAction() {
+    }
+
+    BuildInfoAction(String projectName, int buildNumber) {


Useless constructor, better deleted.

jglick · 2017-07-31T13:58:30Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+        for (BuildInfo buildInfo : buildInfos) {
+            if (buildInfo != null) {
+                Job job = Jenkins.getActiveInstance().getItemByFullName(buildInfo.getProjectName(), Job.class);
+                if (buildInfo.getBuildNumber() != 0) {


Huh? Why would the build number be zero?

jglick · 2017-07-31T13:58:47Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+            if (buildInfo != null) {
+                Job job = Jenkins.getActiveInstance().getItemByFullName(buildInfo.getProjectName(), Job.class);
+                if (buildInfo.getBuildNumber() != 0) {
+                    builds.add((job != null) ? job.getBuildByNumber(buildInfo.getBuildNumber()) : null);


If the job is missing, do not add null.

And getBuildByNumber may well add null; do not add that.

jglick · 2017-07-31T13:59:45Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+
+        for (BuildInfo buildInfo : buildInfos) {
+            if (buildInfo != null) {
+                Job job = Jenkins.getActiveInstance().getItemByFullName(buildInfo.getProjectName(), Job.class);


Unclear whether this should be impersonating SYSTEM. Who is calling it? Perhaps better to return the raw data and let the caller decide how to handle missing information.

jglick · 2017-07-31T14:01:54Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java

@@ -29,6 +30,11 @@ public void onStarted(Run<?, ?> run, TaskListener listener) {
                    TaskListener taskListener = stepContext.get(TaskListener.class);
                    // encodeTo(Run) calls getDisplayName, which does not include the project name.
                    taskListener.getLogger().println("Starting building: " + ModelHyperlinkNote.encodeTo("/" + run.getUrl(), run.getFullDisplayName()));
+                    FlowNode parentNode = stepContext.get(FlowNode.class);
+                    if (parentNode != null) {


No need to check for null if defined via @StepContextParameter (which is actually deprecated but fixing that requires a larger change).

jglick · 2017-07-31T14:02:35Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerListener.java

+        if (buildInfoAction == null) {
+            synchronized (this) {
+                buildInfoAction = parentNode.getAction(BuildInfoAction.class);
+                if (buildInfoAction == null) {


Double-checked locking, unsafe.

jglick · 2017-07-31T14:03:11Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildInfoAction.java

+
+    private static class BuildInfo {
+        private final String projectName;
+        private final int buildNumber;


Easier just to use String via Run.externalizableId.

It does not look easier for me, in this case I would need one String concatenation + String.split for every call for nothing. I now that it's tiny stuff, but even from code perspective it doesn't simplify much.

svanoort · 2017-07-31T15:09:31Z

src/test/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStepTest.java

@@ -430,6 +450,23 @@ public MultiBranchProjectFactory newInstance() {
        }
    }

+    private void assertBuildInfoAction(WorkflowRun workflowRun, String projectName, int buildNumber) {
+        FlowGraphWalker walker = new FlowGraphWalker(workflowRun.getExecution());


🐜 Deprecated, use DepthFirstScanner instead

It needs workflow-api:2.17, so, I updated it in pom.xml as well, but now it fails in verification: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fpipeline-build-step-plugin/detail/PR-13/9/tests/
Also, I think @jglick would not be happy to introduce dependency for higher version (especially in case of test code needs it -> not production).
Also I didn't find tag @Deprecated in constructor of FlowGraphWalker class in new API.

@svanoort, @jglick, @oleg-nenashev So, what should I do?

svanoort · 2017-07-31T15:11:13Z

I'll more or less echo Jesse's comments here -- would need to see an example of how this API is used in practice.

Jimilian · 2017-08-17T07:45:58Z

@svanoort, @jglick I wanted to use it Build Failure Analyzer Plugin (@rsandell). I created proof of concept some time ago and it works perfect, but to create good PR I need to submit this change first. So, I wanted to use API presented in this PR in this way: https://github.com/Jimilian/build-failure-analyzer-plugin/blob/pipeline_jobs_poc/src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/dbf/BuildFlowDBF.java

jglick · 2023-12-08T21:11:17Z

#127 includes the same information, but additionally indicates which step was responsible for a given downstream build.

Save information about child jobs in parent job

46b0391

oleg-nenashev reviewed Jun 27, 2017

View reviewed changes

Several improvements in BuildInfoAction

755b480

* Make BuildInfoAction thread-safe * Create method to provide list of child runs * Add tests for BuildInfoAction

Jimilian commented Jun 28, 2017

View reviewed changes

Implement double-checked locking for retriving the BuildInfoAction

05a8806

Jimilian closed this Jun 29, 2017

Jimilian reopened this Jun 29, 2017

Jimilian closed this Jun 29, 2017

Jimilian reopened this Jun 29, 2017

Jimilian closed this Jun 30, 2017

Jimilian reopened this Jun 30, 2017

jglick self-requested a review July 3, 2017 13:56

oleg-nenashev approved these changes Jul 20, 2017

View reviewed changes

Jimilian added 2 commits July 26, 2017 18:43

Merge remote-tracking branch 'upstream/master' into child_jobs

66aa284

Minor performance optimizations

42aec90

jglick requested changes Jul 31, 2017

View reviewed changes

svanoort reviewed Jul 31, 2017

View reviewed changes

Fix the issues mentioned in code review

9b0afd5

Jimilian closed this Aug 6, 2017

Jimilian reopened this Aug 6, 2017

jglick mentioned this pull request Jun 3, 2019

[JENKINS-49073] Propagate the downstream result, not just FAILURE #24

Merged

jglick closed this Dec 8, 2023

jglick mentioned this pull request Dec 14, 2023

Store DownstreamBuildAction on Run instead of FlowNode for consistent persistence even on completed builds #128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save information about child jobs in parent job #13

Save information about child jobs in parent job #13

Jimilian commented Jun 27, 2017

oleg-nenashev Jun 27, 2017

Jimilian Jun 28, 2017

oleg-nenashev Jun 27, 2017

Jimilian Jun 28, 2017

oleg-nenashev Jun 27, 2017

Jimilian Jun 28, 2017

oleg-nenashev Jun 27, 2017

Jimilian Jun 28, 2017

Jimilian Jun 28, 2017

Jimilian commented Jul 3, 2017

Jimilian commented Jul 20, 2017

oleg-nenashev left a comment

oleg-nenashev Jul 20, 2017

oleg-nenashev Jul 20, 2017

Jimilian Jul 26, 2017

jglick left a comment

jglick Jul 31, 2017

Jimilian Aug 7, 2017

jglick Jul 31, 2017

jglick Jul 31, 2017

jglick Jul 31, 2017

jglick Jul 31, 2017

jglick Jul 31, 2017

jglick Jul 31, 2017

jglick Jul 31, 2017

Jimilian Aug 7, 2017

svanoort Jul 31, 2017

Jimilian Aug 7, 2017 •

edited

Loading

svanoort commented Jul 31, 2017

Jimilian commented Aug 17, 2017

jglick commented Dec 8, 2023

		import java.util.List;


		public class BuildInfoAction extends InvisibleAction {

Save information about child jobs in parent job #13

Save information about child jobs in parent job #13

Conversation

Jimilian commented Jun 27, 2017

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

Jimilian commented Jul 3, 2017

Jimilian commented Jul 20, 2017

oleg-nenashev left a 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

jglick left a 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

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

Jimilian Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

svanoort commented Jul 31, 2017

Jimilian commented Aug 17, 2017

jglick commented Dec 8, 2023

Jimilian Aug 7, 2017 •

edited

Loading