screwdriver-cd / screwdriver

An open source build platform designed for continuous delivery.
http://screwdriver.cd
Other
1.01k stars 170 forks source link

Remote-join is broken when downstream is entered multiple times #2980

Open jithine opened 8 months ago

jithine commented 8 months ago

What happened:

Remote join job is not triggered if the downstream pipeline is part of more than 1 remote trigger scenario

Pipeline demonstrating issue: https://cd.screwdriver.cd/pipelines/12907/events/784127 In this pipeline the last remote-join job third is not triggered. Only second job works

Screenshot 2024-01-11 at 3 19 20 PM

In this instance the downstream pipelines are entered twice as part of remote-join workflow.

What you expected to happen:

  1. Remote-Join job third should have been triggered.
  2. downstream job should have displayed proper trigger during second remote-trigger. In current scenario, the 2nd remote-trigger happens in the event where first remote-trigger happened, but it's upstream is not displayed in workflow graph
  3. Remote join should have worked whether ~ is used or not. In this instance I wasn't able to get any remote-trigger to work without ~ in downstream job Eg event https://cd.screwdriver.cd/pipelines/12908/events/784107 How to reproduce it:

https://github.com/jithine/remote-join-upstream/blob/main/screwdriver.yaml https://github.com/jithine/remote-join-downstream-l2-1/blob/main/screwdriver.yaml https://github.com/jithine/remote-join-downstream-l2-2/blob/main/screwdriver.yaml https://github.com/jithine/remote-join-downstream-l2-3/blob/main/screwdriver.yaml

jithine commented 8 months ago

FYI @kumada626 @y-oksaku

y-oksaku commented 8 months ago

Problem 1

Several builds in the downstream were not triggered by the upstream build. (eg. second and third in this event) Consequently, the join list of the third job in the upstream was not satisfied.

This bug is caused by a 'return' within the for-loop here. https://github.com/screwdriver-cd/screwdriver/blob/3170afbb20eee4d6b6936e9332a279811054e165/plugins/builds/index.js#L1087-L1096

Problem 3

This problem is similar to https://github.com/screwdriver-cd/screwdriver/pull/2964 .

Only ~sd@ is triggered in eventFactory.createBuilds. So sd@ is not triggered when create downstream event at here. https://github.com/screwdriver-cd/screwdriver/blob/3170afbb20eee4d6b6936e9332a279811054e165/plugins/builds/index.js#L1119-L1135

I think it would work if we add trigger processing in the same way.

jithine commented 8 months ago

Thanks for checking @y-oksaku Problem 1 is our immediate priority please.

jithine commented 8 months ago

@y-oksaku we've ended up rolling back the change to unblock internal users. Once you have a fix, pls go ahead and undo these reverts.

y-oksaku commented 8 months ago

@jithine OK, but please note that after revert, Y2 and bar in the following are not triggered.

upstream(123)

jobs:
  hub:
    requires: [ ~commit, ~pr ]
  test:
    requires: []
  test2:
    requires: []
  Y2:
    requires: [ ~sd@12926:foo, test, test2 ]

downstream(456)

jobs:
  foo:
    requires: [ ~sd@123:hub ]
  bar:
    requires: [ sd@123:hub ]
jithine commented 8 months ago

@y-oksaku that's unfortunate, looks like we would end up breaking some customer's workflow. Once you have the fix ready for the workflow in this issue, let's immediately roll forward and bring back all the reverted changes.

y-oksaku commented 7 months ago

Problem 2

I agree that it would be better to display the 2nd upstream remote trigger on the downstream. However, this display is shown in the UI based on the event.startFrom data, so we would need to either modify this column or add a new one. This column is also used in the trigger process, so it would be beneficial to address this along with other trigger issues.

jithine commented 7 months ago

@jithine OK, but please note that after revert, Y2 and bar in the following are not triggered.

upstream(123)

jobs:
  hub:
    requires: [ ~commit, ~pr ]
  test:
    requires: []
  test2:
    requires: []
  Y2:
    requires: [ ~sd@12926:foo, test, test2 ]

downstream(456)

jobs:
  foo:
    requires: [ ~sd@123:hub ]
  bar:
    requires: [ sd@123:hub ]

@y-oksaku can you describe what is not working in this workflow ? My analysis here

  1. bar is not working because ~ is missing, we should address this scenario for sure since all trigger conditions have been satisfied.
  2. Y2 should have triggered if either ~sd@12926:foo runs or if test & test2 runs.
jithine commented 7 months ago

Problem 2

I agree that it would be better to display the 2nd upstream remote trigger on the downstream. However, this display is shown in the UI based on the event.startFrom data, so we would need to either modify this column or add a new one. This column is also used in the trigger process, so it would be beneficial to address this along with other trigger issues.

@y-oksaku in this case, I think rather than attempting to display green on 2nd remote trigger, we should try to continue the graph by adding a graph edge from node component to node ~sd@12907@compent

y-oksaku commented 7 months ago

@jithine

@y-oksaku can you describe what is not working in this workflow ? My analysis here

  1. bar is not working because ~ is missing, we should address this scenario for sure since all trigger conditions have been satisfied.

Yes. ~ is missing, so bar job is not triggered when the build hub completed. This bug is not caused by PRs you reverted, so it is still occurring.

  1. Y2 should have triggered if either ~sd@12926:foo runs or if test & test2 runs.

Y2 is not triggered when downstream job foo completed. (Sorry, there is a mistake: [ ~sd@12926:foo, test, test2 ][ ~sd@456:foo, test1, test2 ]) This problem fixed in https://github.com/screwdriver-cd/screwdriver/pull/2964 but now it was reverted since this fix caused the main Problem 1.

jithine commented 7 months ago

@y-oksaku my test pipeline worked with latest API. So I think the main issue has been fixed. Should I separate out the problem 2 into a separate issue and close this ?