fix(pipelines): expectedArtifacts inheritance from dynamic parent templates #3023

xabierlaiseca · 2019-07-04T12:53:05Z

When expected artifacts are defined in a template that is sourced dynamically,
expected artifacts are not added to the execution.

Expected artifacts can be defined in TemplatedPipelineRequest,
TemplateConfiguration and PipelineTemplate objects, so a merge strategy
is required. Given expected artifacts have a unique id, artifacts with
same id in these object will be overridden. The artifacts with higher
priority are the ones in TemplatedPipelineRequest, then TemplateConfiguration
and finally PipelineTemplate.

xabierlaiseca · 2019-07-04T13:00:02Z

@spinnaker/reviewers

...y/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGeneratorSpec.groovy

jervi · 2019-07-04T13:37:48Z

...ont50/src/test/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarterSpec.groovy

+//    result.stages.size() == 1
+//    result.stages[0].type == "wait"
+//    result.stages[0].refId == "wait1"
+//    result.stages[0].name == "Wait for 5 seconds"


Maybe remove these if not relevant anymore

jervi · 2019-07-05T09:19:34Z

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy

+      pipelineConfig = preprocessor.process(pipelineConfig)
+    }
+
+    pipelineConfig.trigger = trigger


Any reason you are moving this back? It breaks what I fixed in #2963.

Apparently it will be required to run resolve artifacts twice. I added a comment in code explaining why

gardleopard · 2019-07-09T13:29:51Z

@spinnaker/google-reviewers PTAL

ezimanyi

I've added a few comments inline to try to understand this better. I'm a bit nervous about this change as there have been a lot of bugs in artifact resolution for pipeline templates recently, and this code is very fragile so I want to make sure that fixing this particular edge case doesn't introduce more general issues. If there's any way to scope the change more specifically to this change rather than always resolving twice, that might be preferable.

I'm less familiar with the V1-template specific stuff, so would probably want someone who has more knowledge of that to take a look. (Perhaps @marchello2000?)

ezimanyi · 2019-07-11T18:57:59Z

...late/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/TemplatedPipelineRequest.java

@@ -27,7 +26,7 @@
  String id;
  String schema;
  String type;
-  List<ExpectedArtifact> expectedArtifacts;
+  List<HashMap<String, Object>> expectedArtifacts;


Why are we moving from a typed object to a generic Map here? Are we expecting that expectedArtifacts contains a list of things that aren't actually expected artifacts?

The only reason to move it to HashMap<String, Object> is that it's easier to merge artifacts from different sources given that the other sources are using HashMap<String, Object> (TemplateConfiguration.PipelineConfiguration and PipelineTemplate.Configuration).

I considered changing these 2 configuration classes to use ExpectedArtifact instead but many tests failed because of the lack of Artifact boundArtifact field in templates and configurations.

Given that orca is not using the expected artifacts in this class, I consider this change involves lower risk than doing a bigger refactoring in order to be able to use ExpectedArtifact in the other cases.

Method and property types should use an interface type, so these should change to Map<String, Object>

@robzienert map type comment addressed

ezimanyi · 2019-07-11T19:02:57Z

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy

@@ -100,14 +100,18 @@ class DependentPipelineStarter implements ApplicationContextAware {

    def trigger = pipelineConfig.trigger
    //keep the trigger as the preprocessor removes it.
-    def expectedArtifacts = pipelineConfig.expectedArtifacts


It looks like this is reverting the fix that was made in #2430. Have you tested that you haven't re-introduced the bug that was fixed there?

ezimanyi · 2019-07-11T19:44:06Z

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy


    if (parentPipelineStageId != null) {
      pipelineConfig.receivedArtifacts = artifactResolver?.getArtifacts(parentPipeline.stageById(parentPipelineStageId))
    } else {
      pipelineConfig.receivedArtifacts = artifactResolver?.getAllArtifacts(parentPipeline)
    }

+    // For pipelines with dynamic source, it is required to perform the following steps:
+    //   1. Resolve source template artifact so the dynamic source expression can be evaluated.


I'm not sure that I fully understand why we need to resolve artifacts twice here, and have some concerns about now always resolving artifacts twice. I think partly it's that I don't fully understand what is a "pipeline with dynamic source" is. Can you please give an example (or point to a test case) for what such a pipeline looks like and how it would get resolved?

with pipeline with dynamic source I meant to have an expression to fetch source pipeline from artifacts such as

"source": "{% for artifact in trigger.artifacts %}{% if artifact.name == 'pipeline-template' && artifact.type == 'http/file' %}{{artifact.reference}}{% endif %}{% endfor %}"

here a test with dynamic parent:

orca/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarterSpec.groovy

Line 467 in 2e07fcc

def "should trigger v1 templated pipelines with dynamic source using prior artifact"() {

On why we need to resolve artifacts twice, the first time we need it in order to load parent pipeline, once we have loaded the parent pipeline, we need to resolve artifacts again as new artifacts could have been found in parent pipeline.

xabierlaiseca · 2019-07-12T09:17:57Z

@ezimanyi I hope my comments give you some clarification on this. Let me know if more explanation is needed.

ezimanyi · 2019-07-13T00:17:00Z

@xabierlaiseca : Thanks for the explanation, and the pointer to the test. Looking at that example, it seems like the only effect of the first resolveExpectedArtifacts is just to copy the contents of pipeline.recievedArtifacts to pipeline.trigger.artifacts.

It seems like it might be less complex and less risky of a change to leave a single pass of artifact resolution (happening after running the pre-processors as you have here) and have the template refer to {% for artifact in receivedArtifacts %}. If I understand how this works (which it's possible I don't fully yet) I think that should work.

Alternately I could also imagine just directly setting trigger.artifacts where we set pipelineConfig.receivedArtifacts a few lines above if for some reason we need to read these from the trigger. That would avoid actually resolving twice (which has caused issues in the past).

ezimanyi · 2019-07-13T00:20:35Z

The context behind my extreme caution around this is that there were a lot of very subtle bugs in artifact resolution for templated pipelines in the numerous comments on spinnaker/spinnaker#2654. I spent a bunch of time fixing those, but the code is still more complex and not as well tested than I'd like so I want to be careful about introducing increased complexity.

I have less direct experience with how the V1 template processing works (which is why my comments are more around the artifact resolution in DependentPipelineStarter). Would definitely appreciate a look from @robzienert or @marchello2000 who would be in a better position to comment on that part of it.

...in/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGenerator.java

marchello2000 · 2019-07-16T23:24:33Z

...in/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGenerator.java

+    return Optional.ofNullable(eas).orElse(Collections.emptyList());
+  }
+
+  private List<HashMap<String, Object>> mergeExpectedArtifact(


just confirming this is the intent (which, looking at that usages, I think it is) if there are duplicates, the artifacts in the eas2 will "win", right?

yes, that is the intent, this way an spinnaker configuration will be able to override an expected artifact in a template

marchello2000 · 2019-07-16T23:36:53Z

Sorry, just saw this.
I agree with @ezimanyi comments. I don't have context on prior subtleties of failures other than they existed and were painful... @robzienert maybe you can look at this too?

marchello2000 · 2019-07-16T23:38:20Z

@xabierlaiseca : Thanks for the explanation, and the pointer to the test. Looking at that example, it seems like the only effect of the first resolveExpectedArtifacts is just to copy the contents of pipeline.recievedArtifacts to pipeline.trigger.artifacts.

It seems like it might be less complex and less risky of a change to leave a single pass of artifact resolution (happening after running the pre-processors as you have here) and have the template refer to {% for artifact in receivedArtifacts %}. If I understand how this works (which it's possible I don't fully yet) I think that should work.

Alternately I could also imagine just directly setting trigger.artifacts where we set pipelineConfig.receivedArtifacts a few lines above if for some reason we need to read these from the trigger. That would avoid actually resolving twice (which has caused issues in the past).

I agree, seems like this should work, @xabierlaiseca what do you think?

xabierlaiseca · 2019-07-19T11:37:19Z

@ezimanyi @marchello2000 I tried {% for artifact in receivedArtifacts %} but receivedArtifacts could not be resolved when rendering the template.

I'll try the alternative way that was mentioned

xabierlaiseca · 2019-07-22T11:25:19Z

@ezimanyi @marchello2000 can you please have a look to this commit 6f9408f , I believe this is what you suggested instead of resolving artifacts twice but I would like to get some feedback about it from you

marchello2000 · 2019-07-22T16:32:03Z

Thanks, @xabierlaiseca, for reworking this. Unfortunately, I don't understand the subtleties of artifact resolution so I will let @ezimanyi review this

ezimanyi · 2019-07-23T18:08:45Z

@xabierlaiseca : Yes, the latest commit (6f9408f) is indeed what I was suggesting to avoid resolving artifacts twice. If that works for your use case then I think that would be more robust than always resolving twice.

I think the only remaining question from my perspective is still that it appears this reverts #2430 (which stored and re-set the expected artifacts). Was that a deliberate change, or will this re-introduce the issue there?

PerGon · 2019-07-25T06:37:59Z

@ezimanyi regarding #2430 , that fix is covered in line https://github.com/spinnaker/orca/pull/3023/files#diff-4f3d52b85acf51b2ed51955fdbbb78e3R165
so that is covered

…plates When expected artifacts are defined in a template that is sourced dynamically, expected artifacts are not added to the execution. Expected artifacts can be defined in `TemplatedPipelineRequest`, `TemplateConfiguration` and `PipelineTemplate` objects, so a merge strategy is required. Given expected artifacts have a unique `id`, artifacts with same `id` in these object will be overridden. The artifacts with higher priority are the ones in `TemplatedPipelineRequest`, then `TemplateConfiguration` and finally `PipelineTemplate`.

xabierlaiseca · 2019-07-26T10:10:40Z

@ezimanyi @marchello2000 given @PerGon's comment, I believe the PR is ready to be merged, I've just squashed my commits. Could you please re-review and merge it you consider is now ready?

ezimanyi · 2019-07-26T18:23:37Z

OK, this final version looks good to me! Thanks for addressing all of the comments!

jervi reviewed Jul 4, 2019

View reviewed changes

...y/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGeneratorSpec.groovy Show resolved Hide resolved

jervi reviewed Jul 4, 2019

View reviewed changes

...y/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGeneratorSpec.groovy Show resolved Hide resolved

jervi reviewed Jul 4, 2019

View reviewed changes

jervi reviewed Jul 5, 2019

View reviewed changes

xabierlaiseca force-pushed the master branch from b697b56 to d35551a Compare July 9, 2019 10:46

xabierlaiseca force-pushed the master branch from d35551a to 21c224a Compare July 10, 2019 09:18

ezimanyi reviewed Jul 11, 2019

View reviewed changes

marchello2000 reviewed Jul 16, 2019

View reviewed changes

...in/java/com/netflix/spinnaker/orca/pipelinetemplate/v1schema/V1SchemaExecutionGenerator.java Show resolved Hide resolved

marchello2000 reviewed Jul 16, 2019

View reviewed changes

xabierlaiseca force-pushed the master branch 3 times, most recently from 4d4b0eb to 6f9408f Compare July 22, 2019 11:23

xabierlaiseca force-pushed the master branch from a746d98 to 2ffc344 Compare July 22, 2019 14:28

xabierlaiseca force-pushed the master branch 3 times, most recently from 2733ccb to 0fee5d9 Compare July 26, 2019 10:06

xabierlaiseca force-pushed the master branch from 0fee5d9 to dc4ec8f Compare July 26, 2019 10:09

ezimanyi approved these changes Jul 26, 2019

View reviewed changes

ezimanyi merged commit 511bff2 into spinnaker:master Jul 26, 2019

spinnakerbot added the target-release/1.16 label Jul 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pipelines): expectedArtifacts inheritance from dynamic parent templates #3023

fix(pipelines): expectedArtifacts inheritance from dynamic parent templates #3023

xabierlaiseca commented Jul 4, 2019

xabierlaiseca commented Jul 4, 2019

jervi Jul 4, 2019

xabierlaiseca Jul 4, 2019

jervi Jul 5, 2019

xabierlaiseca Jul 5, 2019

gardleopard commented Jul 9, 2019

ezimanyi left a comment

ezimanyi Jul 11, 2019

xabierlaiseca Jul 12, 2019

robzienert Jul 16, 2019

xabierlaiseca Jul 22, 2019

ezimanyi Jul 11, 2019

ezimanyi Jul 11, 2019

xabierlaiseca Jul 12, 2019 •

edited

Loading

xabierlaiseca commented Jul 12, 2019

ezimanyi commented Jul 13, 2019

ezimanyi commented Jul 13, 2019

marchello2000 Jul 16, 2019

xabierlaiseca Jul 19, 2019 •

edited

Loading

marchello2000 commented Jul 16, 2019

marchello2000 commented Jul 16, 2019

xabierlaiseca commented Jul 19, 2019

xabierlaiseca commented Jul 22, 2019 •

edited

Loading

marchello2000 commented Jul 22, 2019

ezimanyi commented Jul 23, 2019

PerGon commented Jul 25, 2019

xabierlaiseca commented Jul 26, 2019

ezimanyi commented Jul 26, 2019

fix(pipelines): expectedArtifacts inheritance from dynamic parent templates #3023

fix(pipelines): expectedArtifacts inheritance from dynamic parent templates #3023

Conversation

xabierlaiseca commented Jul 4, 2019

xabierlaiseca commented Jul 4, 2019

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

gardleopard commented Jul 9, 2019

ezimanyi 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

xabierlaiseca Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

xabierlaiseca commented Jul 12, 2019

ezimanyi commented Jul 13, 2019

ezimanyi commented Jul 13, 2019

Choose a reason for hiding this comment

xabierlaiseca Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

marchello2000 commented Jul 16, 2019

marchello2000 commented Jul 16, 2019

xabierlaiseca commented Jul 19, 2019

xabierlaiseca commented Jul 22, 2019 • edited Loading

marchello2000 commented Jul 22, 2019

ezimanyi commented Jul 23, 2019

PerGon commented Jul 25, 2019

xabierlaiseca commented Jul 26, 2019

ezimanyi commented Jul 26, 2019

xabierlaiseca Jul 12, 2019 •

edited

Loading

xabierlaiseca Jul 19, 2019 •

edited

Loading

xabierlaiseca commented Jul 22, 2019 •

edited

Loading