Allow for larger preset property values, do not override#2124
Allow for larger preset property values, do not override#2124fcrisciani merged 1 commit intomoby:masterfrom
Conversation
drivers/overlay/ostweaks_linux.go
Outdated
| return ioutil.WriteFile(path.Join("/proc/sys", keyPath), []byte(value), 0644) | ||
| } | ||
|
|
||
| func readSystemProperty(key string) ([]byte, error) { |
There was a problem hiding this comment.
I would make this one return a string (you can just cast the []byte)
There was a problem hiding this comment.
still string here makes more sense to me
drivers/overlay/ostweaks_linux.go
Outdated
| logrus.Errorf("error reading the kernel parameter %s = %s, err: %s", k, v, err) | ||
| } | ||
|
|
||
| oldv_i := byte2int(oldv_b) |
There was a problem hiding this comment.
error management does not seem to be correct. If the readSystem fails, you just write an error log but then you try to parse the value anyway.
| @@ -21,10 +22,43 @@ func writeSystemProperty(key, value string) error { | |||
| return ioutil.WriteFile(path.Join("/proc/sys", keyPath), []byte(value), 0644) | |||
There was a problem hiding this comment.
Personally I would have make the following change:
- create a satisfy condition function type.
- enhance the sysctlConf map with string and struct
- cleanup the logic
Example for the whole logic: https://play.golang.com/p/vgsQ455Lvdf
There was a problem hiding this comment.
Think I've got it sorted now. PTAL
drivers/overlay/ostweaks_linux.go
Outdated
|
|
||
| func newValIsHigher(val1, val2 string, check conditionalCheck) bool { | ||
| if check == nil || check(val1, val2) { | ||
| fmt.Println("Yep doing something") |
drivers/overlay/ostweaks_linux.go
Outdated
| fmt.Println("Yep doing something") | ||
| return true | ||
| } | ||
| fmt.Println("Nope") |
There was a problem hiding this comment.
Whoops, this print and the previous one shouldn't have slipped through. Cleaned it up during one of my edits. I'll get those fixed.
drivers/overlay/ostweaks_linux.go
Outdated
| return ioutil.WriteFile(path.Join("/proc/sys", keyPath), []byte(value), 0644) | ||
| } | ||
|
|
||
| func readSystemProperty(key string) ([]byte, error) { |
There was a problem hiding this comment.
still string here makes more sense to me
drivers/overlay/ostweaks_linux.go
Outdated
| logrus.Errorf("error setting the kernel parameter %s = %s, err: %s", k, v.value, err) | ||
| continue | ||
| } | ||
| logrus.Debugf("updating kernel parameter %s = %s (was %s)", k, v.value, oldv) |
drivers/overlay/ostweaks_linux.go
Outdated
| if newValIsHigher(string(oldv), v.value, v.checkFn) { | ||
| // write new prop value to disk | ||
| if err := writeSystemProperty(k, v.value); err != nil { | ||
| logrus.Errorf("error setting the kernel parameter %s = %s, err: %s", k, v.value, err) |
There was a problem hiding this comment.
maybe here you can also mention the old value that is maintained
drivers/overlay/ostweaks_linux.go
Outdated
| for k, v := range osConfig { | ||
| // read the existing property from disk | ||
| oldv, err := readSystemProperty(k) | ||
|
|
There was a problem hiding this comment.
Sorry for the cruft. Thought I'd removed this already. Need to discuss with you.
drivers/overlay/ostweaks_linux.go
Outdated
| oldv, err := readSystemProperty(k) | ||
|
|
||
| if err != nil { | ||
| logrus.Errorf("error reading the kernel parameter %s = %s, err: %s", k, v, err) |
There was a problem hiding this comment.
maybe you can remove the value here, that is the new one, does not have any correlation with the read
There was a problem hiding this comment.
Oh good catch. Didn't consider that.
drivers/overlay/ostweaks_linux.go
Outdated
| continue | ||
| } | ||
|
|
||
| if newValIsHigher(string(oldv), v.value, v.checkFn) { |
There was a problem hiding this comment.
This function name should have a generic name like propertyIsValid, checkIfSuccess, opSuccesful, onPass etc
There was a problem hiding this comment.
Was trying to improve on doSomething; I may have overcorrected. :) Will get that fixed.
Signed-off-by: Jim Carroll <jim.carroll@docker.com>
Codecov Report
@@ Coverage Diff @@
## master #2124 +/- ##
=========================================
Coverage ? 40.34%
=========================================
Files ? 139
Lines ? 22454
Branches ? 0
=========================================
Hits ? 9060
Misses ? 12067
Partials ? 1327
Continue to review full report at Codecov.
|
4 participants
If a sysadmin happens to have set a larger value for any of the properties which are managed by this file, they would be overridden.
The changes proposed in this PR allow a sysadmin to have preexisting property values which are larger than the values set in this file.