Simplify config rewrite file handling and make it really atomic by ushachar · Pull Request #7824 · redis/redis

ushachar · 2020-09-20T22:17:16Z

No description provided.

yossigo · 2020-09-22T09:31:53Z

src/config.c

@ushachar Please consider doing snprintf() into the buffer and check return code to confirm it was big enough, could be simpler.

Agreed. Fixed.

yossigo · 2020-09-22T09:33:04Z

src/config.c

I think a short write should also be handled here.

Yep... was already in progress on it. Done.

yossigo · 2020-09-22T09:34:53Z

src/config.c

I think it might make sense to fchmod() earlier, so once rename() is successful we're all done. In this order we may end up with the wrong permissions.

Good idea. Done.

oranagra · 2020-09-22T09:50:55Z

summing up feedback i relayed privately few days ago:

if you do keep the length check, you can change strncpy to memcpy if you want.
if rename fails, maybe we want to delete the temp file.
most importantly: I really think your commit comment needs to be improved. describe what was wrong with the previous code and what are you doing differently (for easier reading).

ushachar · 2020-09-22T21:43:36Z

@oranagra / @yossigo updated the PR.

oranagra · 2020-09-23T05:45:02Z

src/config.c

this is the first and only use of u_int (personally i don't think i ever encountered it).
i'm afraid of some platform compatibility issues, would feel better to use anything else

ok - switched to explicit 'unsigned int'

oranagra

@yossigo please review/approve

src/config.c

Make sure we handle short writes correctly, sync to disk after writing and use rename to make sure the replacement is actually atomic. In any case of failure old configuration will remain in place. Also, add some additional logging to make it easier to diagnose rewrite problems.

yossigo · 2020-09-25T09:56:09Z

Merged, thanks @ushachar!

ushachar · 2020-09-25T13:44:42Z

@oranagra / @yossigo Is there a label to make sure something is mentioned in the release notes? Folks running without write perms on the config directory need to be aware of this change....

oranagra · 2020-09-25T14:13:27Z

I've added the release notes label, home we'll catch it when the day comes

Make sure we handle short writes correctly, sync to disk after writing and use rename to make sure the replacement is actually atomic. In any case of failure old configuration will remain in place. Also, add some additional logging to make it easier to diagnose rewrite problems. (cherry picked from commit c30bd02)

Make sure we handle short writes correctly, sync to disk after writing and use rename to make sure the replacement is actually atomic. In any case of failure old configuration will remain in place. Also, add some additional logging to make it easier to diagnose rewrite problems.

Make sure we handle short writes correctly, sync to disk after writing and use rename to make sure the replacement is actually atomic. In any case of failure old configuration will remain in place. Also, add some additional logging to make it easier to diagnose rewrite problems. (cherry picked from commit c30bd02)

remicollet · 2020-11-13T10:44:20Z

Looks like this break config rewrite, because of permission problem, as most of the time saving /etc/redis.conf imply write access to /etc, which is usually not allowed.

ushachar · 2020-11-14T22:20:45Z

Looks like this break config rewrite, because of permission problem, as most of the time saving /etc/redis.conf imply write access to /etc, which is usually not allowed.

@remicollet - Yep - See comment in #7824 (comment).
There's no other way to make the rewrite truly atomic. If you want to keep the conf file under /etc, I suggest creating a subdirectory for redis (which would also be useful for other related config files like certificates) and only granting the Redis user write access to that.

yossigo · 2020-11-16T07:05:00Z

@remicollet Do you see a problem with the subdirectory approach? This seems to be what other distros are doing as well and as @ushachar mentioned it has other advantages.

remicollet · 2020-11-16T07:10:53Z

@yossigo indeed subdirectory is a solution, but cannot be used, as it will break existing configuration (upgrade path)

remicollet · 2020-11-16T10:05:05Z

See PR #8058

yossigo · 2020-11-20T16:08:08Z

@remicollet Thank you for proposing a fix in your PR.

However, I'm afraid in a way it goes against the motivation of the original fix - which was to ensure rewrites are atomic. If we take this approach rewrites are again not guaranteed to be atomic, because that would depend on what filesystem permissions are used and whether or not the fallback is triggered. For this reason we can't accept this fix.

As an outstanding part mentioned in #10737, we could just make the cluster config file and ACL file saving done with a more safe and atomic pattern (write to temp file, fsync, rename, fsync dir). The cluster config file uses an in-place overwrite and truncation (which was also used by the main config file before #7824). The ACL file is using the temp file and rename approach, but was missing an fsync. Co-authored-by: 朱天 <[email protected]>

As an outstanding part mentioned in redis#10737, we could just make the cluster config file and ACL file saving done with a more safe and atomic pattern (write to temp file, fsync, rename, fsync dir). The cluster config file uses an in-place overwrite and truncation (which was also used by the main config file before redis#7824). The ACL file is using the temp file and rename approach, but was missing an fsync. Co-authored-by: 朱天 <[email protected]>

yossigo requested changes Sep 22, 2020

View reviewed changes

ushachar force-pushed the make_config_rewrite_atomic branch from 6980e88 to 323e18e Compare September 22, 2020 21:40

oranagra reviewed Sep 23, 2020

View reviewed changes

ushachar force-pushed the make_config_rewrite_atomic branch from 323e18e to 2c63b6c Compare September 23, 2020 10:58

oranagra previously approved these changes Sep 23, 2020

View reviewed changes

oranagra reviewed Sep 24, 2020

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

oranagra requested a review from yossigo September 24, 2020 09:24

ushachar dismissed oranagra’s stale review via ded9459 September 24, 2020 23:27

ushachar force-pushed the make_config_rewrite_atomic branch from 2c63b6c to ded9459 Compare September 24, 2020 23:27

oranagra approved these changes Sep 25, 2020

View reviewed changes

yossigo approved these changes Sep 25, 2020

View reviewed changes

yossigo merged commit c30bd02 into redis:unstable Sep 25, 2020

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Sep 25, 2020

oranagra mentioned this pull request Oct 26, 2020

release 6.0.9 #7965

Merged

remicollet mentioned this pull request Nov 13, 2020

[BUG] atomic config rewrite breaks the feature #8051

Closed

oranagra mentioned this pull request Dec 13, 2020

Redis sentinel 6.0.9 in docker: Could not create tmp config file (Permission denied) #8172

Closed

This was referenced Jan 11, 2021

Release 6.0.10 #8309

Merged

Redis 6.2 RC1. #8187

Merged

oranagra mentioned this pull request Jul 4, 2022

Make cluster config file saving atomic and fsync acl #10924

Merged

Conversation

ushachar commented Sep 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oranagra commented Sep 22, 2020

Uh oh!

ushachar commented Sep 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yossigo commented Sep 25, 2020

Uh oh!

ushachar commented Sep 25, 2020

Uh oh!

oranagra commented Sep 25, 2020

Uh oh!

remicollet commented Nov 13, 2020

Uh oh!

ushachar commented Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yossigo commented Nov 16, 2020

Uh oh!

remicollet commented Nov 16, 2020

Uh oh!

remicollet commented Nov 16, 2020

Uh oh!

yossigo commented Nov 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ushachar commented Nov 14, 2020 •

edited

Loading