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

Inheritance methods of Metadata is not intuitive #3125

Open
yakanechi opened this issue May 29, 2024 · 7 comments
Open

Inheritance methods of Metadata is not intuitive #3125

yakanechi opened this issue May 29, 2024 · 7 comments

Comments

@yakanechi
Copy link
Contributor

What happened:
If there are multiple trigger sources for the current metadata, the value of the job that completed first takes precedence.
In the following example, the value is set in job1 and then reset in job3, but job2 is completed before job3, so the value of x is 1 in job4.
It seems strange that values set later are not used.

What you expected to happen:
Consider adding a timestamp-like feature for each metadata, with the newly added ones taking precedence.

How to reproduce it:

jobs:
  Job1:
    requires: [ ~commit ]
    steps:
      - meta set x "1"

  Job2:
    requires: [ ~Job1 ]
    steps:
      - echo "nothing"

  Job3:
    requires: [ ~Job1 ]
    steps:
      - sleep 60
      - meta set x "2"

  Job4:
    requires: [ Job2, Job3 ]
    steps:
      - echo $(meta get x)

@VonnyJap
Copy link
Member

@sagar1312 @tkyi - do you guys have any thoughts on how to solve this issue? Basically this looks like a bug, x should be seen as 2 in Job4, but users are seeing the old value 1 returned.

@yakanechi yakanechi changed the title Add features such as timestamps to Metadata Inheritance methods of Metadata is not intuitive May 31, 2024
@tkyi
Copy link
Member

tkyi commented Jul 11, 2024

Yeah that sounds like a bug. Should be the more recently finished build that takes precedence.

@VonnyJap
Copy link
Member

VonnyJap commented Nov 6, 2024

@yakanechi - I was able to reproduce this issue.
https://cd.screwdriver.cd/pipelines/15511/events/823582
https://github.com/VonnyJap/screwdriver-config/blob/main/test-meta/screwdriver.yaml

I believe this can be fixed with this implementation below:
Sort by build endtime is performed prior to the merge of all parent builds metadata.

Will your side be able to make a launcher change on this?

@yakanechi
Copy link
Contributor Author

@VonnyJap
Yes, we plan to fix this in the near future.

@kumada626
Copy link
Contributor

Only sort parent builds by build endtime not solve below case.

jobs:
  Job1:
    requires: [ ~commit ]
    steps:
      - meta set x "1"

  Job2:
    requires: [ ~Job1 ]
    steps:
      - meta set x "2"

  Job3:
    requires: [ ~Job1 ]
    steps:
      - sleep 60
      - echo "nothing"

  Job4:
    requires: [ Job2, Job3 ]
    steps:
      - echo $(meta get x)

In that case, x should be seen as 2 in Job4, but users are seeing the old value 1 returned.
Because x is set in Job1 and is carried over to Job3, when the meta of Job3 is merged, x is also overwritten with the value of Job3.
I think that something like a timestamp per metadata key might be needed to solve this problem.

@VonnyJap
Copy link
Member

VonnyJap commented Nov 18, 2024

@kumada626 - I got your point now.
So yeah, we need to implement a more robust solution around metadata, including whether or not metadata is dirty (being changed) or clean (no change) after passing through a certain build. During the merging, this information along with timestamp is needed to decide the latest value of a certain metadata key. This is going to involve a larger design change, which we need to partner to do together.

As such, I think the PR raised here can be put on hold until we make the larger design change.
screwdriver-cd/launcher#481

Please let me know what your thoughts are.

@kumada626
Copy link
Contributor

@VonnyJap I agree that large design changes are needed.
However, at least at this point in time, the order of builds to merge the metadata will be unique, so that PR change can be merged before the design change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants