Add the ability for driverdog to copy modules #119
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
yeazelm
force-pushed
the
copying-driverdog
branch
from
4d6663e
to
1ea7f6f
Compare
|
^ Fix clippy for Enum naming. |
larvacea
reviewed
Sep 3, 2024
bcressey
reviewed
Sep 3, 2024
yeazelm
force-pushed
the
copying-driverdog
branch
from
1ea7f6f
to
4555c98
Compare
|
Updated the commit to reflect the two different configuration types: |
The expected behavior for link-modules and load-modules is to load all the modules from all the driver configuration files unless a specific modules set is specified. In the case that a modules set is specified, then driverdog should only link or load modules from the specified target configuration. The link logic worked this way but the load logic would always run the else clause, therefore always loading all modules even if only one was specified. Signed-off-by: Matthew Yeazel <[email protected]>
cbgbt
reviewed
Sep 7, 2024
|
Overall I think the config driven by the enums seems quite clean and easy to follow. Nice work! |
yeazelm
force-pushed
the
copying-driverdog
branch
from
4555c98
to
ad700fb
Compare
|
^ updated based upon @cbgbt comments |
cbgbt
approved these changes
Sep 11, 2024
arnaldo2792
approved these changes
Sep 11, 2024
There are cases where modules could be included that are already linked but could be loaded by driverdog. This adds the ability to specify the command line argument copy-modules and improves the configuration format to allow configurations to either link or copy modules. Signed-off-by: Matthew Yeazel <[email protected]>
yeazelm
force-pushed
the
copying-driverdog
branch
from
ad700fb
to
3fcf714
Compare
|
^ Fix the comments from @arnaldo2792 |
cbgbt
approved these changes
Sep 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue number:
Related to # bottlerocket-os/bottlerocket#4172
Description of changes:
This PR fixes a bug in driverdog (see the first commit) and adds the ability for driverdog to read a configuration file that doesn't link. With that new logic it also supports a link option to only copy the drivers to the right place since some modules may already be linked, but still need to be loaded by driverdog. See bottlerocket-os/bottlerocket#4172 for more background.
Testing done:
Added unit tests to ensure the new loading of the file works for linking vs copying files. I also built an
aws-k8s-1.30-nvidiaAMI and ensured things still loaded correctly. More testing will come under the PR that combines all the work together to ensure the two drivers can be chosen at runtime.I also built on top of #118 for including both drivers and provided the following config file to make sure it works:
Which resulted in:
Note, to make this work we would need
nvidia-modeset.koandnvidia-peermem.koin the config file for it to fully work as intended.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.