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
Allow for larger preset property values, do not override #2124
Conversation
drivers/overlay/ostweaks_linux.go
Outdated
@@ -21,10 +22,43 @@ func writeSystemProperty(key, value string) error { | |||
return ioutil.WriteFile(path.Join("/proc/sys", keyPath), []byte(value), 0644) | |||
} | |||
|
|||
func readSystemProperty(key string) ([]byte, 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.
I would make this one return a string (you can just cast the []byte)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
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.
remove this
drivers/overlay/ostweaks_linux.go
Outdated
fmt.Println("Yep doing something") | ||
return true | ||
} | ||
fmt.Println("Nope") |
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.
remove this
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.
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
@@ -21,10 +22,43 @@ func writeSystemProperty(key, value string) error { | |||
return ioutil.WriteFile(path.Join("/proc/sys", keyPath), []byte(value), 0644) | |||
} | |||
|
|||
func readSystemProperty(key string) ([]byte, 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.
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) |
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.
updating --> updated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe here you can also mention the old value that is maintained
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.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these extra lines
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function name should have a generic name like propertyIsValid
, checkIfSuccess
, opSuccesful
, onPass
etc
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.
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.
|
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.
LGTM
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.