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

Make config reloader file writes atomic #962

Merged
merged 2 commits into from
Mar 22, 2019

Conversation

juliusv
Copy link
Contributor

@juliusv juliusv commented Mar 22, 2019

This addresses an issue found in the Prometheus Operator, which reuses
this reloader sidecar, but which then also has a second sidecar which
may trigger rule-based reloads while the config sidecar is in the middle
of writing out its config (in a non-atomic way):

prometheus-operator/prometheus-operator#2501

I didn't add a test for this because it's hard to catch the original
problem to begin with, but it has happened.

Signed-off-by: Julius Volz julius.volz@gmail.com

This addresses an issue found in the Prometheus Operator, which reuses
this reloader sidecar, but which then also has a second sidecar which
may trigger rule-based reloads while the config sidecar is in the middle
of writing out its config (in a non-atomic way):

prometheus-operator/prometheus-operator#2501

I didn't add a test for this because it's hard to catch the original
problem to begin with, but it has happened.

Signed-off-by: Julius Volz <julius.volz@gmail.com>
Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

lgtm from my side! 👌

@metalmatze
Copy link
Contributor

CI fails with:

pkg/reloader/reloader.go:203:19:	os.Remove	defer os.Remove(tmpFile)

@juliusv I suspect you verified that the normal flow of the reloader works with this change? ☺️

@s-urbaniak
Copy link
Contributor

I guess errcheck is a bit aggressive here, as the os.Remove(tmpFile) in this case is "just" a precaution.

@bwplotka
Copy link
Member

Its definitely not aggressive (: it's just a matter of being explicit when you don't need error by doing: _ = ...

Signed-off-by: Julius Volz <julius.volz@gmail.com>
@juliusv
Copy link
Contributor Author

juliusv commented Mar 22, 2019

I added an explicit error ignore now.

I suspect you verified that the normal flow of the reloader works with this change?

Nope :) Not beyond the existing test suite at least.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thx, LGTM

@@ -199,9 +199,14 @@ func (r *Reloader) apply(ctx context.Context) error {
return errors.Wrap(err, "expand environment variables")
}

if err := ioutil.WriteFile(r.cfgOutputFile, b, 0666); err != nil {
tmpFile := r.cfgOutputFile + ".tmp"
defer os.Remove(tmpFile)
Copy link
Member

Choose a reason for hiding this comment

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

You can satisfy errcheck by

defer func(){ _ := os.Remove(tmpFile) }()

@bwplotka bwplotka merged commit e756fe1 into thanos-io:master Mar 22, 2019
@juliusv juliusv deleted the atomic-config-writes branch March 22, 2019 17:12
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

Successfully merging this pull request may close these issues.

4 participants