Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

timestamp gitlab ci job outputs #13605

Merged
merged 4 commits into from
Mar 15, 2023
Merged

timestamp gitlab ci job outputs #13605

merged 4 commits into from
Mar 15, 2023

Conversation

altaua
Copy link
Contributor

@altaua altaua commented Mar 14, 2023

@altaua altaua added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 14, 2023
@altaua altaua requested a review from a team as a code owner March 14, 2023 19:36
@altaua altaua self-assigned this Mar 14, 2023
@mateo-moon
Copy link
Contributor

I don't like an optional timestamp functionality. This adds complexity. I'd rather find the way to fix the issues with some pipelines failing with this script.

@altaua
Copy link
Contributor Author

altaua commented Mar 14, 2023 •
edited
Loading

I don't like an optional timestamp functionality. This adds complexity. I'd rather find the way to fix the issues with some pipelines failing with this script.

Aye, I'd like them to be enabled everywhere as well; I'm planning to investigate the failures once I've confirmed everything else works so far at least.

@altaua altaua force-pushed the mira/gitlab-timestamp branch 2 times, most recently from 3ee5fc4 to 9516fa1 Compare March 14, 2023 21:11
@altaua
Copy link
Contributor Author

altaua commented Mar 14, 2023

Turns out the linux-stable-int test was just a hardcoded timeout inside the test being hit because the naive timestamping script we'd been using slows output-heavy tests down a lot.

This was further compounded by the old script being bugged and discarding all remaining buffered output when the job script finishes.

@altaua altaua force-pushed the mira/gitlab-timestamp branch 3 times, most recently from 2f606ea to 2cb5f65 Compare March 15, 2023 11:36
Mira Ressel added 2 commits March 15, 2023 12:52
timestamp gitlab ci job outputs
7a2bcf3 
Based on previous work by @alvicsam in #13047.
inline timestamp script
b32a61b 
Some of our jobs don't check out the substrate repo.
@altaua altaua force-pushed the mira/gitlab-timestamp branch from 2cb5f65 to 1499706 Compare March 15, 2023 12:40
include .timestamp in pipelines overriding the default before_script
7a9b587 
Still not including it in the zombienet jobs, they have their own
timestamping anyway.
@altaua altaua force-pushed the mira/gitlab-timestamp branch from 1499706 to 7a9b587 Compare March 15, 2023 12:42
@altaua altaua changed the title [DRAFT] timestamp gitlab ci job outputs timestamp gitlab ci job outputs Mar 15, 2023
@altaua altaua added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 15, 2023
mateo-moon
mateo-moon approved these changes Mar 15, 2023
View reviewed changes
scripts/ci/gitlab/timestamp.yml Outdated
exec > >(s_datestamp) 2>&1

TIMESTAMP_PID=$!
trap cleanup EXIT
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I don't understand what is it for) Could you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's to fix a bug in the original script: The main job script might finish before the s_datestamp process substitution has had a chance to process all output; if we don't wait for it, bash will just exit immediately, killing s_datestamp and thus discarding the last chunk of output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Incidentally, that's what made us not see the timeout error message in test-linux-stable-int for #13047)

@paritytech-ci paritytech-ci requested a review from a team March 15, 2023 12:48
@mateo-moon
Copy link
Contributor

It might be useful to add this script to other projects. Do you know is there any shared library for scripts in the gitlab?

@altaua
Copy link
Contributor Author

altaua commented Mar 15, 2023

It might be useful to add this script to other projects. Do you know is there any shared library for scripts in the gitlab?

Other repos could include the .timestamp spec with an include:project directive; we just need to discuss if we're fine with including it from substrate or would like to start a dedicated pipeline library repo oslt (or do we perchance have one and I just haven't discovered it yet?).

@rcny
Copy link
Contributor

rcny commented Mar 15, 2023

It might be useful to add this script to other projects. Do you know is there any shared library for scripts in the gitlab?

Other repos could include the .timestamp spec with an include:project directive; we just need to discuss if we're fine with including it from substrate or would like to start a dedicated pipeline library repo oslt (or do we perchance have one and I just haven't discovered it yet?).

I've created this project. Feel free to add the timestamp snippet there and reference it here.

@mateo-moon
Copy link
Contributor

It might be useful to add this script to other projects. Do you know is there any shared library for scripts in the gitlab?

Other repos could include the .timestamp spec with an include:project directive; we just need to discuss if we're fine with including it from substrate or would like to start a dedicated pipeline library repo oslt (or do we perchance have one and I just haven't discovered it yet?).

The dedicated library sounds just right. This would help us to maintain shared code properly and use DRY approach.

@altaua altaua force-pushed the mira/gitlab-timestamp branch from 2171f72 to 8299e22 Compare March 15, 2023 15:28
@altaua
Copy link
Contributor Author

altaua commented Mar 15, 2023

.timestamp will have to remain disabled for macos jobs; /bin/bash on ci-mac2 is far too old, and annoyingly gitlab can't be configured to use the homebrew bash version instead.

For reference, the missing bash features are

  • the printf %(..)T format specifier -- this could be worked around by invoking date instead on macos, that'd just add some overhead (observed up to 100% for output-heavy jobs)
  • writing process substitution pids to $? -- working around this would be very annoying, and if we don't, the jobs would be at risk of not printing parts of their output.

@altaua
Copy link
Contributor Author

altaua commented Mar 15, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit ba87188 into master Mar 15, 2023
@paritytech-processbot paritytech-processbot bot deleted the mira/gitlab-timestamp branch March 15, 2023 16:37
@kiltbot kiltbot mentioned this pull request Apr 3, 2023
Closed
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
@ukint-vs
timestamp gitlab ci job outputs (paritytech#13605)
cfc3475 
* timestamp gitlab ci job outputs

Based on previous work by @alvicsam in paritytech#13047.

* inline timestamp script

Some of our jobs don't check out the substrate repo.

* include .timestamp in pipelines overriding the default before_script

Still not including it in the zombienet jobs, they have their own
timestamping anyway.

* move timestamp.yml to shared pipeline repo

https://gitlab.parity.io/parity/infrastructure/ci_cd/shared
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@nathanwhit
timestamp gitlab ci job outputs (paritytech#13605)
24b24c5 
* timestamp gitlab ci job outputs

Based on previous work by @alvicsam in paritytech#13047.

* inline timestamp script

Some of our jobs don't check out the substrate repo.

* include .timestamp in pipelines overriding the default before_script

Still not including it in the zombienet jobs, they have their own
timestamping anyway.

* move timestamp.yml to shared pipeline repo

https://gitlab.parity.io/parity/infrastructure/ci_cd/shared
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mateo-moon mateo-moon mateo-moon approved these changes

@alvicsam alvicsam alvicsam approved these changes

@rcny rcny rcny approved these changes

Assignees

@altaua altaua

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@altaua @mateo-moon @rcny @alvicsam