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.

Sign up for GitHub

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

Jump to bottom

Add basic kernel tuning functionality #47

Merged
merged 1 commit into from
Mar 27, 2019
Merged

Add basic kernel tuning functionality #47

merged 1 commit into from
Mar 27, 2019

Conversation

ashcrow
Copy link
Member

@ashcrow ashcrow commented Mar 26, 2019 •
edited
Loading

Checks /etc/pivot/kernel-args for argument changes. This early version only supports bare arguments that are in the whitelist.

See conversation at openshift/machine-config-operator#576

Requires: coreos/rpm-ostree#1796

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 26, 2019
@ashcrow ashcrow requested a review from abhinavdahiya March 26, 2019 17:00
@ashcrow ashcrow mentioned this pull request Mar 26, 2019
Merged
@ashcrow
Copy link
Member Author

ashcrow commented Mar 26, 2019

Almost done. Trying to find the right invocation for rpm-ostree kargs for bare arguments.

runcom
runcom reviewed Mar 26, 2019
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
runcom
runcom reviewed Mar 26, 2019
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
cgwalters
cgwalters reviewed Mar 26, 2019
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
@ashcrow ashcrow force-pushed the system-tuning branch 2 times, most recently from 54b3da9 to 4b1222c Compare March 26, 2019 18:58
@ashcrow ashcrow mentioned this pull request Mar 26, 2019
Closed
cgwalters
cgwalters reviewed Mar 27, 2019
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
@cgwalters cgwalters mentioned this pull request Mar 27, 2019
Closed
@cgwalters
Copy link
Member

Re-linking the MCO issue for implementing day-2 changes: openshift/machine-config-operator#388

Note that the implementation of that could write the file and call pivot.service, but it seems easier to have the MCD directly talk to rpm-ostree.

@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

Updated per requests

ashcrow
ashcrow commented Mar 27, 2019
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

Note: Keeping as WIP until the updated rpm-ostree is available. /cc @jlebon

jlebon
jlebon reviewed Mar 27, 2019
View reviewed changes
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Show resolved Hide resolved
@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

Updated per @jlebon's review

@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

Still need to check /proc/cmdline ... will work on that now.

@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

/proc/cmdline work added. It's unfortunately a bit verbose

@ashcrow
Add basic kernel tuning functionality
9383200 
Checks /etc/pivot/kernel-args for argument changes. This early
version only supports bare arguments that are in the whitelist.

See conversation at openshift/machine-config-operator#576

Signed-off-by: Steve Milner <[email protected]>
@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

squashed

@ashcrow ashcrow changed the title WIP: Add basic kernel tuning functionality Add basic kernel tuning functionality Mar 27, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2019
@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

Removed WIP as it should be in a good state, though I'm going to do some manual testing shortly.

@ashcrow ashcrow changed the title Add basic kernel tuning functionality WIP: Add basic kernel tuning functionality Mar 27, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2019
@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019 •
edited
Loading

Adding it back .,.. /proc/cmdline and rpm-ostree kargs don't match when checking. They do, but after reboot which is fine for this use case.

@ashcrow ashcrow changed the title WIP: Add basic kernel tuning functionality Add basic kernel tuning functionality Mar 27, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2019
@ashcrow
Copy link
Member Author

ashcrow commented Mar 27, 2019

Ready for last set of review/verification.

cgwalters
cgwalters reviewed Mar 27, 2019
View reviewed changes
cmd/root.go
}

checkable := string(content)
if strings.Contains(checkable, arg) {
Copy link
Member

Choose a reason for hiding this comment

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

I think right now this is OK, but in the future we're clearly going to need to actually parse the cmdline properly because this will e.g. succeed if some other user-editable string in the cmdline contains nosmt like rd.lvm.lv=janosmt because their name is "Janos Mountain" or something.

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019
@cgwalters cgwalters merged commit d95d123 into openshift:master Mar 27, 2019
runcom
runcom reviewed May 14, 2019
View reviewed changes
cmd/root.go
// Read and parse the file
file, err := os.Open(tuningFilePath)
// Clean up
defer file.Close()
Copy link
Member

@runcom runcom May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

if open fails for any reason (like a not found of the tuningFilePath), this will panic, this statement should go after the error check below

Copy link
Member

Choose a reason for hiding this comment

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

cc @ashcrow @jlebon

Copy link
Member

Choose a reason for hiding this comment

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

#52

cgwalters added a commit to cgwalters/pivot that referenced this pull request May 14, 2019
@cgwalters
root: Fix defer call to be after error check
ab431f6 
Spotted by runcom:
openshift#47 (comment)
@cgwalters cgwalters mentioned this pull request May 14, 2019
Merged
jlebon pushed a commit that referenced this pull request May 14, 2019
@cgwalters @jlebon
root: Fix defer call to be after error check
8688a45 
Spotted by runcom:
#47 (comment)
@cgwalters cgwalters mentioned this pull request Jul 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlebon jlebon jlebon left review comments

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@runcom runcom runcom left review comments

@cgwalters cgwalters cgwalters left review comments

@yuqi-zhang yuqi-zhang Awaiting requested review from yuqi-zhang

Assignees

@cgwalters cgwalters

Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ashcrow @cgwalters @runcom @abhinavdahiya @jlebon @openshift-ci-robot