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

re-synchronize default linux kernel config with buildroot #9394

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

dalehamel
Copy link
Contributor

@dalehamel dalehamel commented Oct 5, 2020

The current linux configuration appears to have been manually edited, and includes configuration options that do not exist on the current Linux kernel version used by minikube, or else configs were added that only applied to 5.4, which were never reverted when the 5.4 kernel itself was reverted (i think because #8187 was reverted?)

In particular, I think the PRs that caused this to diverge are:

#8582, which adds IKHEADERS, which only applies to newer (5.4) kernel version (cc @alban

#8187 which seems to be unecessary, since the config for CONFIG_PCI and CONFIG_TMPFS seem to be added by default (cc @afbjorklund), and it looks like CONFIG_VIRTIO_FS is only available in 5.4 or later

It looks like we tried upgrading to 5.4 and reverted it ,but never reverted the linux config?

Without a consistent, machine-generated kernel config, done via make linux-menuconfig and make linux-update-defconfig, it will be pretty hairy to make any kernel changes.

EDIT: I just realized, maybe this should be run as a CI check? Verify that "make linux-update-defconfig" results in 0 changes? Would prevent manual edits of the file, and enforce that it is machine generated as buildroot expects.

I also just noticed that #9329 keeps the old LTS kernel, so i think this patch is still relevant, and shouldn't conflict with the kernel config in that PR.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 5, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @dalehamel. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 5, 2020
@dalehamel
Copy link
Contributor Author

Note that i haven't actually changed anything here, i've just run the scripts you are supposed to run with buildroot, to do a no-op change. The changes here are the result of buildroot's automation.

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@dalehamel dalehamel mentioned this pull request Oct 5, 2020
@dalehamel
Copy link
Contributor Author

dalehamel commented Oct 6, 2020

This should wait for #9329 and then be regenerated, as the diff will make more sense.

EDIT: i reread the PR description, and noticed that they are not bumping linux there, so this is still relevant

@dalehamel dalehamel changed the title Properly generate linux config re-synchronize linux config with buildroot Oct 6, 2020
@dalehamel dalehamel changed the title re-synchronize linux config with buildroot re-synchronize default linux kernel config with buildroot Oct 6, 2020
@afbjorklund
Copy link
Collaborator

It looks like we tried upgrading to 5.4 and reverted it ,but never reverted the linux config?

That is pretty much the case. And we haven't made a final decision, if we should stick with 4.19 or not.
Mostly because we never got to the bottom of what the problem with the new buildroot and kernel was ?

If time is running out, I'm all for cleaning up the previous kernel config and redoing it properly later on.
The minikube kernel config has grown quite organically, and doesn't always describe the changes made.

I made some efforts to compare with a more vanilla kernel config, in another project.

The boot2docker project had some nice division of kernel config into separate files.

@dalehamel
Copy link
Contributor Author

I don't have a problem with the current, minimal config, it's just missing a few options, and I think that it would be good to keep it consistent with what buildroot expects.

Since buildroot offers this feature to trim down the kernel config to "what you need", and that is what we're storing in here, it makes sense to assert this I think (eg, a CI check that the configured kernel has 0 changes as I suggested above).

We can re-sync it with 5.4, or try a separate 5.4 config (and build both 4.19 and 5.4 for a while, maybe?)

@afbjorklund
Copy link
Collaborator

afbjorklund commented Oct 22, 2020

Let's stick with 4.19 for now, and fix the config and do a minor upgrade...

I think this needs to be rebased/regenerated, before we can merge it ?

But I also get the same results from running make linux-menuconfig

@afbjorklund
Copy link
Collaborator

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund, dalehamel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2020
@afbjorklund afbjorklund merged commit fa27e41 into kubernetes:master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants