search

carvel-dev / vendir

Easy way to vendor portions of git repos, github releases, helm charts, docker image contents, etc. declaratively
https://carvel.dev/vendir
Apache License 2.0
281 stars 50 forks source link

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

Closed Zebradil closed 8 months ago

Zebradil commented 9 months ago

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 commented 9 months ago

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 🚀

Zebradil commented 9 months ago

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 🙂

Zebradil commented 9 months ago

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

kumaritanushree commented 9 months ago

LGTM

Zebradil commented 9 months ago

@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.

joaopapereira commented 8 months ago

@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.

Zebradil commented 8 months ago

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

UPD: I just went for it.