Fix race condition when running multiple vendir from the same directory #345

Zebradil · 2023-12-26T23:20:59Z

At the moment, the temporary directory used by vendir is hardcoded to be .vendir-tmp in the current working directory.
Running multiple vendir processes in parallel will cause them to interfere with each other.

This PR is fixing the issue by creating a unique temporary directory for each vendir process.

Fixes #346

100mik

The change makes sense to me!
nit: But maybe we can avoid the other lint-like changes and keep the PR concise?

100mik · 2023-12-27T08:53:58Z

Worth a call out, while consuming this users must ensure that there are no two vendir processes trying to write to the same path.
I am sure the way you are consuming vendir is conscious of that 🚀

Signed-off-by: German Lashevich <[email protected]>

Zebradil · 2023-12-27T15:29:05Z

Hi @100mik, thanks for looking into this PR.

But maybe we can avoid the other lint-like changes and keep the PR concise?

Sure, done.

I am sure the way you are consuming vendir is conscious of that

Yes, destination directories are guaranteed to differ.
Before we were using --chdir flag to jump to a target directory, but that doesn't work well with directory sources that use relative paths. So we made all vendir processes run from the project root and here we are 🙂

100mik

Awesome!
LGTM! But we will be needing a go ahead from one of the vendir approvers on this one
cc: @joaopapereira

praveenrewar

Thank you so much for the PR @Zebradil!
I think the ask is reasonable, do you mind also opening an issue for the same?
Also, would you be able to add a test for this?

pkg/vendir/directory/staging_dir.go

Zebradil · 2023-12-28T11:40:43Z

Also, would you be able to add a test for this?

~~I'll see what I can do.~~ Done. At the time of filing the PR, I wanted first to make sure that the change was desirable and accepted.

UPD:

I think the ask is reasonable, do you mind also opening an issue for the same?

~~Will do 🙂~~ Done: #346

Signed-off-by: German Lashevich <[email protected]>

kumaritanushree · 2024-01-04T12:10:46Z

LGTM

praveenrewar

LGTM! Thank you so much for fixing the issue ❤️

pkg/vendir/directory/staging_dir.go

joaopapereira

I do share the concern that @100mik mentioned, but in the end, I do not think this change will make any difference.
If you fix the panic comment I think we should be in a good place to accept this PR

joaopapereira · 2024-01-05T20:14:11Z

pkg/vendir/directory/staging_dir.go

-	rootDir := ".vendir-tmp"
+	rootDir, err := os.MkdirTemp(".", ".vendir-tmp-")
+	if err != nil {
+		panic(fmt.Errorf("Creating tmp dir: %s", err))


Instead of panicking, let us return an error change the function signature since it is only used in 1 place in the code.

It calmed down 🙂

joaopapereira · 2024-01-05T20:17:08Z

test/e2e/concurrent_processes_test.go

+					RunOpts{
+						Dir:         tmpRoot,
+						StdinReader: strings.NewReader(fmt.Sprintf(yaml, n)),
+						AllowError:  true,


nit: Since we expect this to not fail, we could set this to false.

True, addressed.

Zebradil · 2024-01-05T22:56:13Z

@joaopapereira

I do share the concern that @100mik mentioned, but in the end, I do not think this change will make any difference.

Did you mean this conversation?

I also think it is not ideal to create-delete-create the dir. I would remove the initial cleanup step, but I'm unsure whether it is completely safe. With the current code, where we have guarantees that the staging dir is always newly created, I don't see any risks.

Signed-off-by: German Lashevich <[email protected]>

joaopapereira · 2024-01-09T17:46:57Z

@joaopapereira

I do share the concern that @100mik mentioned, but in the end, I do not think this change will make any difference.

Did you mean this conversation?

I also think it is not ideal to create-delete-create the dir. I would remove the initial cleanup step, but I'm unsure whether it is completely safe. With the current code, where we have guarantees that the staging dir is always newly created, I don't see any risks.

Sorry, I meant this comment:

Worth a call out, while consuming this users must ensure that there are no two vendir processes trying to write to the same path.

joaopapereira

Since we are creating a new tmp folder do we need to call cleanUpAll in the Prepare function? This is going to delete the folder that we just created.

Zebradil · 2024-01-10T09:01:59Z

@joaopapereira let's remove the cleanup part then? Do you anticipate any risks in this?

UPD: I just went for it.

Signed-off-by: German Lashevich <[email protected]>

joaopapereira

LGTM

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [carvel.dev/vendir](https://togithub.com/carvel-dev/vendir) | `v0.38.0` -> `v0.39.0` | [![age](https://developer.mend.io/api/mc/badges/age/go/carvel.dev%2fvendir/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/carvel.dev%2fvendir/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/carvel.dev%2fvendir/v0.38.0/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/carvel.dev%2fvendir/v0.38.0/v0.39.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>carvel-dev/vendir (carvel.dev/vendir)</summary> ### [`v0.39.0`](https://togithub.com/carvel-dev/vendir/releases/tag/v0.39.0) [Compare Source](https://togithub.com/carvel-dev/vendir/compare/v0.38.0...v0.39.0) <details> <summary><h2>Installation and signature verification</h2></summary> ##### Installation ##### By downloading binary from the release For instance, if you are using Linux on an AMD64 architecture: ```shell ### Download the binary curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/vendir-linux-amd64 ### Move the binary in to your PATH mv vendir-linux-amd64 /usr/local/bin/vendir ### Make the binary executable chmod +x /usr/local/bin/vendir ``` ##### Via Homebrew (macOS or Linux) ```shell $ brew tap carvel-dev/carvel $ brew install vendir $ vendir version ``` ##### Verify checksums file signature Install cosign on your system https://docs.sigstore.dev/system_config/installation/ The checksums file provided within the artifacts attached to this release is signed using [Cosign](https://docs.sigstore.dev/cosign/overview/) with GitHub OIDC. To validate the signature of this file, run the following commands: ```shell ### Download the checksums file, certificate and signature curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/checksums.txt curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/checksums.txt.pem curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/checksums.txt.sig ### Verify the checksums file cosign verify-blob checksums.txt \ --certificate checksums.txt.pem \ --signature checksums.txt.sig \ --certificate-identity-regexp=https://github.com/carvel-dev \ --certificate-oidc-issuer=https://token.actions.githubusercontent.com ``` ##### Verify binary integrity To verify the integrity of the downloaded binary, you can utilize the checksums file after having validated its signature. ```shell ### Verify the binary using the checksums file sha256sum -c checksums.txt --ignore-missing ``` </details> ### ✨ What's new * fix grammar in README by @vtrent[https://github.com/carvel-dev/vendir/pull/324](https://togithub.com/carvel-dev/vendir/pull/324)ll/324 * Added changes to sign artifacts by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/339](https://togithub.com/carvel-dev/vendir/pull/339)ll/339 * Simplify gitignore and make sure all binaries are accounted for by @100m[https://github.com/carvel-dev/vendir/pull/351](https://togithub.com/carvel-dev/vendir/pull/351)ll/351 * PrivateKey with or without extra char as newline will be accepted by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/349](https://togithub.com/carvel-dev/vendir/pull/349)ll/349 * Fix race condition when running multiple vendir from the same directory by @Zebrad[https://github.com/carvel-dev/vendir/pull/345](https://togithub.com/carvel-dev/vendir/pull/345)ll/345 * Refactor lazy sync code by @Zebrad[https://github.com/carvel-dev/vendir/pull/340](https://togithub.com/carvel-dev/vendir/pull/340)ll/340 * Fix: updated setup cosign step in release process by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/352](https://togithub.com/carvel-dev/vendir/pull/352)ll/352 * updated release to have installation and verification steps included in release notes by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/354](https://togithub.com/carvel-dev/vendir/pull/354)ll/354 #### New Contributors * @vtrenton made their first contributi[https://github.com/carvel-dev/vendir/pull/324](https://togithub.com/carvel-dev/vendir/pull/324)ll/324 * @100mik made their first contributi[https://github.com/carvel-dev/vendir/pull/351](https://togithub.com/carvel-dev/vendir/pull/351)ll/351 **Full Changelog**: carvel-dev/vendir@v0.38.0...v0.39.0 ### 📂 Files Checksum 012531a2f1a2de8bc89f1623edfc40a7ac5aee421fe609085278fb9e287f1cdf ./vendir-linux-arm64 20b71cc25dc3fea31edf9667c92a05167f713935f854882159736443c2f7a0e6 ./vendir-windows-amd64.exe 90ae82718c1072831f3097bdb031d5a897cc9f2f8334e2e1d7f35e35d0abd84f ./vendir-darwin-amd64 91ecf04ad5cdfa0f8839dc1430da7a4da665f7cb88c64c0c72202f6db261e651 ./vendir-darwin-arm64 feb2836153508adfb6fd33c127e466c9ce26577678e93a252be2fec445f4501f ./vendir-linux-amd64 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/mykso/myks).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [carvel-dev/vendir](https://togithub.com/carvel-dev/vendir) | minor | `v0.38.0` -> `v0.39.0` | --- ### Release Notes <details> <summary>carvel-dev/vendir (carvel-dev/vendir)</summary> ### [`v0.39.0`](https://togithub.com/carvel-dev/vendir/releases/tag/v0.39.0) [Compare Source](https://togithub.com/carvel-dev/vendir/compare/v0.38.0...v0.39.0) <details> <summary><h2>Installation and signature verification</h2></summary> ##### Installation ##### By downloading binary from the release For instance, if you are using Linux on an AMD64 architecture: ```shell ### Download the binary curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/vendir-linux-amd64 ### Move the binary in to your PATH mv vendir-linux-amd64 /usr/local/bin/vendir ### Make the binary executable chmod +x /usr/local/bin/vendir ``` ##### Via Homebrew (macOS or Linux) ```shell $ brew tap carvel-dev/carvel $ brew install vendir $ vendir version ``` ##### Verify checksums file signature Install cosign on your system https://docs.sigstore.dev/system_config/installation/ The checksums file provided within the artifacts attached to this release is signed using [Cosign](https://docs.sigstore.dev/cosign/overview/) with GitHub OIDC. To validate the signature of this file, run the following commands: ```shell ### Download the checksums file, certificate and signature curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/checksums.txt curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/checksums.txt.pem curl -LO https://github.com/carvel-dev/vendir/releases/download/v0.39.0/checksums.txt.sig ### Verify the checksums file cosign verify-blob checksums.txt \ --certificate checksums.txt.pem \ --signature checksums.txt.sig \ --certificate-identity-regexp=https://github.com/carvel-dev \ --certificate-oidc-issuer=https://token.actions.githubusercontent.com ``` ##### Verify binary integrity To verify the integrity of the downloaded binary, you can utilize the checksums file after having validated its signature. ```shell ### Verify the binary using the checksums file sha256sum -c checksums.txt --ignore-missing ``` </details> ### ✨ What's new * fix grammar in README by @vtrent[https://github.com/carvel-dev/vendir/pull/324](https://togithub.com/carvel-dev/vendir/pull/324)ll/324 * Added changes to sign artifacts by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/339](https://togithub.com/carvel-dev/vendir/pull/339)ll/339 * Simplify gitignore and make sure all binaries are accounted for by @100m[https://github.com/carvel-dev/vendir/pull/351](https://togithub.com/carvel-dev/vendir/pull/351)ll/351 * PrivateKey with or without extra char as newline will be accepted by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/349](https://togithub.com/carvel-dev/vendir/pull/349)ll/349 * Fix race condition when running multiple vendir from the same directory by @Zebrad[https://github.com/carvel-dev/vendir/pull/345](https://togithub.com/carvel-dev/vendir/pull/345)ll/345 * Refactor lazy sync code by @Zebrad[https://github.com/carvel-dev/vendir/pull/340](https://togithub.com/carvel-dev/vendir/pull/340)ll/340 * Fix: updated setup cosign step in release process by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/352](https://togithub.com/carvel-dev/vendir/pull/352)ll/352 * updated release to have installation and verification steps included in release notes by @kumaritanushr[https://github.com/carvel-dev/vendir/pull/354](https://togithub.com/carvel-dev/vendir/pull/354)ll/354 #### New Contributors * @vtrenton made their first contributi[https://github.com/carvel-dev/vendir/pull/324](https://togithub.com/carvel-dev/vendir/pull/324)ll/324 * @100mik made their first contributi[https://github.com/carvel-dev/vendir/pull/351](https://togithub.com/carvel-dev/vendir/pull/351)ll/351 **Full Changelog**: carvel-dev/vendir@v0.38.0...v0.39.0 ### 📂 Files Checksum 012531a2f1a2de8bc89f1623edfc40a7ac5aee421fe609085278fb9e287f1cdf ./vendir-linux-arm64 20b71cc25dc3fea31edf9667c92a05167f713935f854882159736443c2f7a0e6 ./vendir-windows-amd64.exe 90ae82718c1072831f3097bdb031d5a897cc9f2f8334e2e1d7f35e35d0abd84f ./vendir-darwin-amd64 91ecf04ad5cdfa0f8839dc1430da7a4da665f7cb88c64c0c72202f6db261e651 ./vendir-darwin-arm64 feb2836153508adfb6fd33c127e466c9ce26577678e93a252be2fec445f4501f ./vendir-linux-amd64 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/mykso/myks).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Zebradil force-pushed the unique-tmp-dir branch from 5eb3a00 to daac963 Compare December 26, 2023 23:21

Zebradil mentioned this pull request Dec 26, 2023

fix(sync): allow local paths in vendir config mykso/myks#191

Merged

100mik reviewed Dec 27, 2023

View reviewed changes

100mik requested review from joaopapereira and kumaritanushree December 27, 2023 08:54

Fix race condition when running multiple vendir from the same directory

ad7060c

Signed-off-by: German Lashevich <[email protected]>

Zebradil force-pushed the unique-tmp-dir branch from daac963 to ad7060c Compare December 27, 2023 15:24

100mik approved these changes Dec 27, 2023

View reviewed changes

praveenrewar reviewed Dec 27, 2023

View reviewed changes

pkg/vendir/directory/staging_dir.go Show resolved Hide resolved

Add e2e test for concurrent vendir processes

84b2be4

Signed-off-by: German Lashevich <[email protected]>

Zebradil requested a review from praveenrewar January 5, 2024 08:49

praveenrewar approved these changes Jan 5, 2024

View reviewed changes

pkg/vendir/directory/staging_dir.go Show resolved Hide resolved

joaopapereira requested changes Jan 5, 2024

View reviewed changes

Zebradil added 2 commits January 5, 2024 23:56

Keep calm and return error

782af1d

Signed-off-by: German Lashevich <[email protected]>

Streamline error handling in the test

927b4fe

Signed-off-by: German Lashevich <[email protected]>

Zebradil force-pushed the unique-tmp-dir branch from 129eb30 to 927b4fe Compare January 5, 2024 22:56

Zebradil requested a review from joaopapereira January 5, 2024 22:58

joaopapereira reviewed Jan 9, 2024

View reviewed changes

Remove unnecessary cleanUpAll() call from StagingDir.Prepare()

f9ac5a0

Signed-off-by: German Lashevich <[email protected]>

Zebradil requested a review from joaopapereira January 10, 2024 09:41

joaopapereira approved these changes Jan 10, 2024

View reviewed changes

joaopapereira merged commit c1e3217 into carvel-dev:develop Jan 10, 2024
4 checks passed

Zebradil deleted the unique-tmp-dir branch January 10, 2024 20:20

Zebradil mentioned this pull request May 3, 2024

REQUEST: New membership for Zebradil carvel-dev/carvel#763

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition when running multiple vendir from the same directory #345

Fix race condition when running multiple vendir from the same directory #345

Zebradil commented Dec 26, 2023 •

edited

Loading

100mik left a comment

100mik commented Dec 27, 2023

Zebradil commented Dec 27, 2023

100mik left a comment

praveenrewar left a comment

Zebradil commented Dec 28, 2023 •

edited

Loading

kumaritanushree commented Jan 4, 2024

praveenrewar left a comment

joaopapereira left a comment

joaopapereira Jan 5, 2024

Zebradil Jan 5, 2024

joaopapereira Jan 5, 2024

Zebradil Jan 5, 2024

Zebradil commented Jan 5, 2024

joaopapereira commented Jan 9, 2024

joaopapereira left a comment

Zebradil commented Jan 10, 2024 •

edited

Loading

joaopapereira left a comment

Fix race condition when running multiple vendir from the same directory #345

Fix race condition when running multiple vendir from the same directory #345

Conversation

Zebradil commented Dec 26, 2023 • edited Loading

100mik left a comment

Choose a reason for hiding this comment

100mik commented Dec 27, 2023

Zebradil commented Dec 27, 2023

100mik left a comment

Choose a reason for hiding this comment

praveenrewar left a comment

Choose a reason for hiding this comment

Zebradil commented Dec 28, 2023 • edited Loading

kumaritanushree commented Jan 4, 2024

praveenrewar left a comment

Choose a reason for hiding this comment

joaopapereira left a comment

Choose a reason for hiding this comment

joaopapereira Jan 5, 2024

Choose a reason for hiding this comment

Zebradil Jan 5, 2024

Choose a reason for hiding this comment

joaopapereira Jan 5, 2024

Choose a reason for hiding this comment

Zebradil Jan 5, 2024

Choose a reason for hiding this comment

Zebradil commented Jan 5, 2024

joaopapereira commented Jan 9, 2024

joaopapereira left a comment

Choose a reason for hiding this comment

Zebradil commented Jan 10, 2024 • edited Loading

joaopapereira left a comment

Choose a reason for hiding this comment

Zebradil commented Dec 26, 2023 •

edited

Loading

Zebradil commented Dec 28, 2023 •

edited

Loading

Zebradil commented Jan 10, 2024 •

edited

Loading