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.

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

tools: add diff-kernel-config to identify kernel config changes #2368

Merged
merged 2 commits into from Sep 19, 2022

Conversation

markusboehme
Copy link
Member

Issue number: n/a

Description of changes:

tools: add diff-kernel-config to identify kernel config changes

With multiple kernel versions being supported for two architectures in
cloud and metal flavors it can be tedious and error-prone to get the
full picture on the effects changes have on the resulting kernel
configurations. The new diff-kernel-config tool helps automate this
task.

diff-kernel-config compares kernel configurations before and after a
series of commits to the Bottlerocket repository. For this, it runs
several full kernel builds that cover all possible combinations of
kernel version, architecture, and cloud/metal flavor that could be
shipped in any of the official variants. Combinations not used in any
variant are skipped.

diff-kernel-config produces kernel config diffs broken down by
combination in its output directory, alongside a comprehensive report of
all changes and a summary that can aid the code review process.

Testing done:

$ time ./tools/diff-kernel-config -a fork/fix/disable-usb-net-drivers -b fork/fix/disable-usb-net-drivers^ -k 5.10 -k 5.15 -o configs
[...]

config-aarch64-5.10-aws-k8s-1.23-diff:   42 removed,   0 added,   1 changed
config-aarch64-5.15-aws-dev-diff:         0 removed,   0 added,   0 changed
config-aarch64-5.15-metal-dev-diff:       0 removed,   0 added,   0 changed
config-x86_64-5.10-aws-k8s-1.23-diff:    35 removed,   0 added,   1 changed
config-x86_64-5.10-metal-k8s-1.23-diff:  35 removed,   0 added,   1 changed
config-x86_64-5.15-aws-dev-diff:          0 removed,   0 added,   0 changed
config-x86_64-5.15-metal-dev-diff:        0 removed,   0 added,   0 changed

A full report has been placed in 'configs/diff-report'

real    64m21.385s
user    3m31.946s
sys     1m30.428s

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Copy link
Contributor

@foersleo foersleo left a comment

Choose a reason for hiding this comment

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

This is awesome. Currently giving it a spin for some of my hardware enablement patches.

tools/diff-kernel-config Outdated Show resolved Hide resolved
Copy link
Contributor

@foersleo foersleo left a comment

Choose a reason for hiding this comment

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

Reiterating that this is super helpful, especially with bigger config changes to check what else gets pulled in when switching kernel options.

I have used it a few times the past couple of days to straighten out my work on the hardware enablement.

Copy link
Contributor

@arnaldo2792 arnaldo2792 left a comment

Choose a reason for hiding this comment

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

How can we let users about the existence of this script? Should we mention it in the main README or probably in the QUICKSTART-LOCAL doc?

@markusboehme
Copy link
Member Author

How can we let users about the existence of this script? Should we mention it in the main README or probably in the QUICKSTART-LOCAL doc?

Users wouldn't be interested in this unless they spin their own variant and modify the kernel packages. I wouldn't expect it in either the top-level README (script is too narrow and specific) or QUICKSTART-LOCAL (script is not related to running local VMs). It might find a mention in a dedicated README in the kernel packages, but perhaps it's also fine to not mention it at all for now and do that when a number of contributions without config diff stats come in. What do you think?

Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

This is great!

tools/diff-kernel-config Show resolved Hide resolved
Comment on lines +212 to +233
rpm2cpio "${latest_archive_rpms[0]}" \
| cpio --quiet --extract --to-stdout \
| tar --xz --extract --to-stdout kernel-devel/scripts/diffconfig >"${diffconfig}"
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot to love in this script but this may be my favorite part.

Comment on lines +239 to +260
# Generate combined report of changes
head -v -n 999999 "${output_dir}"/*-diff >"${output_dir}"/diff-report
echo "A full report has been placed in '${output_dir}/diff-report'"
Copy link
Contributor

Choose a reason for hiding this comment

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

If the report itself is the only reason to keep the directory around, it might be a nicer interface to use a temporary directory for the output, and just copy the report out as a permanent, gitignore'd file at the end.

That would trade off the need to keep track of output directories that don't yet exist with the need to occasionally prune older report files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, I haven't really made up my mind yet on this matter. I've found use for both the combined report and the individual files. Let's see how folks will use it in the next PRs and decide one way or the other? I agree on simplifying the interface should it turn out the combined report is all that's getting used.

With multiple kernel versions being supported for two architectures in
cloud and metal flavors it can be tedious and error-prone to get the
full picture on the effects changes have on the resulting kernel
configurations. The new diff-kernel-config tool helps automate this
task.

diff-kernel-config compares kernel configurations before and after a
series of commits to the Bottlerocket repository. For this, it runs
several full kernel builds that cover all possible combinations of
kernel version, architecture, and cloud/metal flavor that could be
shipped in any of the official variants. Combinations not used in any
variant are skipped.

diff-kernel-config produces kernel config diffs broken down by
combination in its output directory, alongside a comprehensive report of
all changes and a summary that can aid the code review process.

Signed-off-by: Markus Boehme <markubo@amazon.com>
The diff-kernel-config tool may not be easily discoverable in the tools/
directory of the repository. Create minimal readme files in the kernel
packages that point out its existence. The tool's usage information will
answer any questions about how to produce the desired config change
report.

Signed-off-by: Markus Boehme <markubo@amazon.com>
@markusboehme
Copy link
Member Author

The force push fixed leaking a temporary file and now comes with readme files in the kernel packages that point towards this script.

@markusboehme markusboehme merged commit 5fc04d6 into bottlerocket-os:develop Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants