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

helmfile sync, multiple charts in same namespace results in a race condition #149

Closed
Stono opened this issue May 17, 2018 · 7 comments
Closed

Comments

@Stono
Copy link
Contributor

Stono commented May 17, 2018

Hey,
If you have a helmfile, which:

  • references multiple charts all being deployed into the same namespace
  • and that namespace does not exist
  • you do not have concurrency=1

Then you can run into a race condition where helm will attempt to create that namespace multiple times:

exec: helm upgrade --install --reset-values data-platform-grafana /app/charts/grafana --namespace data-platform
exec: helm upgrade --install --reset-values data-platform-alertmanager /app/charts/alertmanager --namespace data-platform
exec: helm upgrade --install --reset-values data-platform-prometheus /app/charts/prometheus --namespace data-platform
Release "data-platform-grafana" does not exist. Installing it now.
Release "data-platform-alertmanager" does not exist. Installing it now.
Error: release data-platform-alertmanager failed: namespaces "data-platform" already exists

None of these charts contain a namespace.yaml, this is purely due to helm creating the namespace as part of the helm install.

Suggestion
Perhaps helmfile could be smart enough to enforce a concurrency of 1, per namespace, when the namespace doesn't already exist?

@mumoshu
Copy link
Collaborator

mumoshu commented May 18, 2018

@Stono Thank you for the detailed report. Good catch! This should be fixed.

IMHO, this is an upstream issue, because it is helm who is doing a kind of optimistic-locking. It could detect the race by looking to the error cause.

If it can't be fixed upstream early, I'd rather like helmfile to retry only once on already exists error. Because it is simpler and just make it tolerant to the race.

WDYT?

@sstarcher
Copy link
Contributor

I agree I really don't want to limit to a concurrency of 1 in a namespace. We can either retry or we can create the NS ahead of time if it does not exist, but an issue should be opened upstream.

@Stono
Copy link
Contributor Author

Stono commented May 18, 2018

I shall open it upstream :-)

@amnk
Copy link
Contributor

amnk commented Jun 11, 2018

I'm facing similar (but not the same) issue: when specifying same secrets file for several charts, it results in race while cleaning up files:

err: remove secrets.yaml.dec: no such file or directory
err: remove secrets.yaml.dec: no such file or directory

@mumoshu
Copy link
Collaborator

mumoshu commented Jun 11, 2018

@amnk Ah, good catch! It could happen. Would you mind raising another issue for that?
In contrast to this upstream issue, yours seem to be in helmfile, because it's helmfile who creates and cleans the files.

@amnk
Copy link
Contributor

amnk commented Jun 12, 2018

@mumoshu done. And thanks for prompt replies!

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 22, 2018

The upstream issue is already fixed by @Stono's awesome PR, and it has been already released as part of helm v2.10! So, closing this as fixed.

Thanks everyone for your supports 🎉

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