Skip to content

Conversation

@emosbaugh
Copy link
Member

@emosbaugh emosbaugh commented Feb 18, 2025

Description, Motivation and Context

Path is wrong for some systems.

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

@emosbaugh emosbaugh added type::bug Something isn't working bug::normal labels Feb 18, 2025
@emosbaugh emosbaugh requested a review from a team as a code owner February 18, 2025 19:25
@emosbaugh emosbaugh requested a review from jdewinne February 18, 2025 19:28
squizzi
squizzi previously approved these changes Feb 18, 2025
Copy link
Member

@squizzi squizzi left a comment

Choose a reason for hiding this comment

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

lgtm, just a nit on logging the directories checked.


kernelPath := filepath.Join("/lib/modules", kernelRelease)
if _, err := os.Stat(kernelPath); os.IsNotExist(err) {
klog.V(2).Infof("modules are not loadable because kernel path %q does not exist, assuming we are in a container", kernelPath)
Copy link
Member

@squizzi squizzi Feb 18, 2025

Choose a reason for hiding this comment

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

nit: It might be worth logging in this instance that /lib/modules does not exist, checking /usr/lib/modules before the assuming we are in container log fires, because that makes it sound like we only ever checked /usr/lib/modules.

Or maybe update the kernelPath in Infof to be:

klog.V(2).Infof("modules are not loadable because kernel path %q or %q do not exist, assuming we are in a container", kernelPathLib, kernelPathUsr)

Copy link
Member Author

Choose a reason for hiding this comment

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

updated to print the expected path

@emosbaugh emosbaugh enabled auto-merge (squash) February 18, 2025 21:06
@laverya laverya changed the title fix(host-preflights): buildtin kernel modules file from wrong path fix(host-preflights): buildin kernel modules file from wrong path Feb 18, 2025
@laverya laverya changed the title fix(host-preflights): buildin kernel modules file from wrong path fix(host-preflights): builtin kernel modules file from wrong path Feb 18, 2025
@emosbaugh emosbaugh merged commit 51c3a0c into main Feb 18, 2025
22 checks passed
@emosbaugh emosbaugh deleted the emosbaugh/20250218/builtin-kernel-modules-debian branch February 18, 2025 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug::normal type::bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants