-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
etcdserver: always check if the data dir is writable before starting #4090
Conversation
@@ -57,6 +59,16 @@ func ReadDir(dirpath string) ([]string, error) { | |||
return names, nil | |||
} | |||
|
|||
// MkdirAllIfNotExist is simliar to os.MkdirAll, but will not return ErrExist | |||
// if the dir exsits before. | |||
func MkdirAllIfNotExist(dir string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unwieldy function name-- at first glance I expected it to fail if the directory exists. Possible alternatives (not necessarily good): MkdirAllClobber
, MkdirAllAlways
, TouchDirAll
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to mkdirAllAlways
. If you still do not feel good about the naming, it probably means that helper func itself is unnecessary . We can just handle the error in main path.
PTAL |
if mkerr := fileutil.MkdirAllIfNotExist(cfg.DataDir); mkerr != nil { | ||
return nil, fmt.Errorf("cannot create data directory: %v", mkerr) | ||
} | ||
if wderr := fileutil.IsDirWriteable(cfg.DataDir); wderr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IsDirWriteable
check probably belongs in the function MkdirAllIfNotExist
since every call will probably be followed by the writeable test otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a hard time to think of a good name for it.
Basically there are 4 cases:
- the dir does not exist; a dir is made; the dir must be writable
- the dir does not exist; a dir fails to be made; the dir must be not writable
- the dir exists, check the dir and it is writable
- the dir exists, check the dir and it is not writable
I cannot come up with a good func name to summary these functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's like the directory equivalent of touch
(ignoring atime/mtime stuff), isn't it? Create if not present, otherwise return true if writeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel touch is used to change time... But after ignoring that, sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it does the IsDirWriteable
test it'll change the mtime of the directory because of that ".touch" file business, so it all works out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right...
Agreed on the auto-creation, I'd rather have the program auto-create a directory than have it ask me to create it myself. No-create would catch misconfiguration (which would manifest elsewhere anyway), of course, but it doesn't seem to be worth the inconvenience. |
@heyitsanthony All fixed. Please take a look again. |
lgtm |
etcdserver: always check if the data dir is writable before starting
Fix #3713
#3713 also mentions another issue that etcd should not auto create data-dir if not exist. I do not think that is a good suggestion. I feel it is just OK to create a non-existing data dir for user.
/cc @heyitsanthony