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][CI] Don't run "Pulsar CI checks completed" too early and fix disk space issue in saving docker image #17584

Merged
merged 4 commits into from
Sep 12, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Sep 12, 2022

Fixes #17583

Motivation

Pulsar CI is broken since "Pulsar CI checks completed" runs too early.
There's another problem that disk space runs out in "Build Pulsar docker image" with error message

Error response from daemon: write /var/lib/docker/tmp/docker-export-3539721826/b4b6ffd2e938fa5512aa646b2c22044e3e0b14e74d3dc8374a9b3723a7235c53/layer.tar: no space left on device

Modifications

  • remove "if: always()" since it caused the job to run also when something failed
  • release disk space in "Build Pulsar docker image" build job before exporting the docker image

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

- "if: always()" caused the job to run also when something failed
@lhotari lhotari added this to the 2.12.0 milestone Sep 12, 2022
@lhotari lhotari self-assigned this Sep 12, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 12, 2022
@@ -844,7 +844,6 @@ jobs:
'system-tests',
'macos-build'
]
if: always()
Copy link
Member

Choose a reason for hiding this comment

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

I ever wrote another script for running always but check the result of needs to determinate which cleanups can be trigger.

See also https://github.com/apache/incubator-kvrocks/blob/2402dfec05c582235dfb195bb1d4210584e59848/.github/workflows/kvrocks.yaml#L220-L233.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, nice example. I didn't know about that ${{ needs.build-and-test.result }} trick.

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari changed the title [fix][CI] Don't run "Pulsar CI checks completed" too early [fix][CI] Don't run "Pulsar CI checks completed" too early and fix disk space issue in saving docker image Sep 12, 2022
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

Thanks @lhotari
I noticed this issue during the weekend too

@lhotari
Copy link
Member Author

lhotari commented Sep 12, 2022

Available disk space before building docker image:

/dev/root         85161M 41518M    43627M  49% /

Available disk space after building docker image:

/dev/root         85161M 83949M     1197M  99% /

I wonder when the build started consuming so much disk space? @tisonkun could it be related to the Trino changes?

@tisonkun
Copy link
Member

@lhotari I don't find the possibility by #17062. It does some rename and location change works, while no docker image layer change. You may go through the assembly and dockerfile changes for double check.

@nicoloboschi
Copy link
Contributor

@lhotari something is still wrong.
Unit test Broker 2 timed out: https://github.com/apache/pulsar/runs/8299450001?check_suite_focus=true
Checks completed job has been skipped: https://github.com/apache/pulsar/runs/8300402413?check_suite_focus=true

But I'm still able to merge your pull

@lhotari lhotari merged commit 6751039 into apache:master Sep 12, 2022
@tisonkun
Copy link
Member

Hi @lhotari @nicoloboschi according to docker image changes, this patch may be relevant #17129.

@lhotari
Copy link
Member Author

lhotari commented Sep 12, 2022

@lhotari something is still wrong. Unit test Broker 2 timed out: https://github.com/apache/pulsar/runs/8299450001?check_suite_focus=true Checks completed job has been skipped: https://github.com/apache/pulsar/runs/8300402413?check_suite_focus=true

But I'm still able to merge your pull

there's a follow up in #17586 to address this problem. This solution is inspired by @tisonkun 's solution in kvrocks (#17584 (comment)).

tisonkun pushed a commit to tisonkun/pulsar that referenced this pull request Sep 14, 2022
…sk space issue in saving docker image (apache#17584)

* [fix][CI] Don't run "Pulsar CI checks completed" too early

- "if: always()" caused the job to run also when something failed

* Clean up disk space in "Build Pulsar docker image"

* Show disk space before and after cleaning

* Fix deleting files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] "Pulsar CI checks completed" runs too early
4 participants