Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bake): Lookup artifact details from all upstream stages #3011

Merged
merged 2 commits into from
Jul 2, 2019

Conversation

marchello2000
Copy link
Contributor

@marchello2000 marchello2000 commented Jun 26, 2019

Imagine following setup:

Pipeline A
 -> Jenkins job (produces foo.*.deb)
 -> Run Pipeline B
    -> Jenkins job (produces bar.*.deb)
    -> Bake package foo

In this case, the package lookup for bake stage will fail to lookup artifact
details for foo.deb to pass to the bakery. Thus the bakery is free to pick the
latest artifact matching the name which can be the wrong artifact.

This change allows package look up to traverse up parent pipeline stages, similar
to FindImage tasks

parentPipelineStages.sort(
new Comparator<Stage>() {
@Override
public int compare(Stage s1, Stage s2) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried doing Comparator.comparingNullLast(Comparator.comparing(stage::getEndTime)) but it was throwing NPE, if you know what I was doing wrong, would love to simplify this to use an off the shelf comparator

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have any context on why this time comparison is required ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a great mystery... hence my comment about this being a "port to java". I wonder if it's there for unit tests?
Or, more likely, this is a relic of the past that could be removed? @ajordens / @duftler do either of you happen to know the history here? (#874)

@spinnakerbot
Copy link
Contributor

Please delete the pull request instructions from the body of your pull request message.

The instructions start with the line:

We prefer small, well tested pull requests.

You can reopen your pull request when this has been addressed.

@emjburns emjburns reopened this Jun 26, 2019
allPriorStages.addAll(getAncestors(parent.get(), execution));
} else if ((execution.getType() == PIPELINE)
&& (execution.getTrigger() instanceof PipelineTrigger)) {
PipelineTrigger parentTrigger = (PipelineTrigger) execution.getTrigger();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes only one level back, correct? So your package must be created either in the existing pipeline or its parent, but not its grandparent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test case for this case if not present already : parent -> child -> grandchild

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will go back multiple levels, getAncestors() recurses:

      if (parentPipelineStage.isPresent()) {
        allPriorStages.addAll(getAncestors(parentPipelineStage.get(), parentPipelineExecution));
      } else {

This could be problematic from a resource consumption standpoint, especially in cases I've seen within the community where users have massive chains of dependent pipelines.

I'd like to see parent pipeline traversal limited to fulfilling a search condition. So instead of "get all stages across all parents, then filter for $condition", "traverse ancestors until $condition".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @asher is correct - this should recurse until root execution is found.

@asher : can you elaborate on this comment (we can chat in person as well - no rush). I agree it would be more efficient to only recurse until the condition is satisfied. But other than the efficiency are there any other reasons for it that I am not understanding?

Because the stages are returned in order (e.g. closest ancestor first) so the search downstream will find the closest match (assuming they traverse in order which currently all callers do).

To be clear: I like your suggestion and I think it will actually simplify some findImage code - just want to make sure I understand it completely since I am not super familiar with various pipeline designs you've seen from the community

allPriorStages.addAll(getAncestors(parent.get(), execution));
} else if ((execution.getType() == PIPELINE)
&& (execution.getTrigger() instanceof PipelineTrigger)) {
PipelineTrigger parentTrigger = (PipelineTrigger) execution.getTrigger();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will go back multiple levels, getAncestors() recurses:

      if (parentPipelineStage.isPresent()) {
        allPriorStages.addAll(getAncestors(parentPipelineStage.get(), parentPipelineExecution));
      } else {

This could be problematic from a resource consumption standpoint, especially in cases I've seen within the community where users have massive chains of dependent pipelines.

I'd like to see parent pipeline traversal limited to fulfilling a search condition. So instead of "get all stages across all parents, then filter for $condition", "traverse ancestors until $condition".

@marchello2000 marchello2000 requested a review from asher July 1, 2019 16:58
@marchello2000
Copy link
Contributor Author

@asher : made the function take a predicate and terminate as soon as match is found - a little more code but the consumption side nicer (not to mention it's more efficient, thanks!)
Also added some "grandparent" tests

Imagine following setup:
```
Pipeline A
 -> Jenkins job (produces foo.*.deb)
 -> Run Pipeline B
    -> Jenkins job (produces bar.*.deb)
    -> Bake package foo
```

In this case, the package lookup for bake stage will fail to lookup artifact
details for `foo.deb` to pass to the bakery. Thus the bakery is free to pick the
latest artifact matching the name which can be the wrong artifact.

This change allows package look up to traverse up parent pipeline stages, similar
to `FindImage` tasks
Copy link
Contributor

@asher asher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for implementing the predicate based approach!

@marchello2000 marchello2000 merged commit e4a8d3f into spinnaker:master Jul 2, 2019
@marchello2000 marchello2000 deleted the mark/spin-4858 branch July 2, 2019 05:17
jervi added a commit to jervi/orca that referenced this pull request Oct 1, 2019
A previous refactor (spinnaker#3011) reversed the order of stages returned from parent executions, which in turn caused a bug in the lookup of image id's.
If you have several image producing stages in a parent pipeline, for instance a "Find image by tag" stage followed by a bake stage, Spinnaker would now use the output of the "Find image by tag" stage instead of the bake stage. This PR changes it back to the former behaviour, so that the image closest to the current executing stage will be used.
jervi added a commit to jervi/orca that referenced this pull request Oct 1, 2019
A previous refactor (spinnaker#3011) reversed the order of stages returned from parent executions, which in turn caused a bug in the lookup of image id's.
If you have several image producing stages in a parent pipeline, for instance a "Find image by tag" stage followed by a bake stage, Spinnaker would now use the output of the "Find image by tag" stage instead of the bake stage. This PR changes it back to the former behaviour, so that the image closest to the current executing stage will be used.
marchello2000 pushed a commit that referenced this pull request Oct 2, 2019
A previous refactor (#3011) reversed the order of stages returned from parent executions, which in turn caused a bug in the lookup of image id's.
If you have several image producing stages in a parent pipeline, for instance a "Find image by tag" stage followed by a bake stage, Spinnaker would now use the output of the "Find image by tag" stage instead of the bake stage. This PR changes it back to the former behaviour, so that the image closest to the current executing stage will be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants