-
Notifications
You must be signed in to change notification settings - Fork 21
manifest-rhel-10.1: add azure-vm-utils for Azure udev rules #112
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaradhak 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 |
Summary of ChangesHello @aaradhak, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines Azure VM disk and device configuration for RHEL 10.1 by integrating the official Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds the azure-vm-utils package to the RHEL 10.1 manifest to use upstream Azure udev rules, and removes the old custom udev rules file. While adding the package is a good improvement for RHEL 10.1, removing the udev rules file from the repository will negatively affect other OS versions that are built from this configuration and still rely on those rules. I have provided a critical comment with a suggested alternative to address this issue.
|
rhcos-devel pipeline build with the azure-vm-utils pkg - https://jenkins-rhcos--devel-pipeline.apps.int.preprod-stable-spoke1-dc-iad2.itup.redhat.com/blue/organizations/jenkins/build/detail/build/44/pipeline/ |
a4da2bd to
8a420dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file would still be needed for rhel-9.6 IIUC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right, even after the code freeze in rhel-9.6, I believe this would be needed. I will add it back.
This was the original expectation, but the team actually got this into RHEL 9 as well (RHEL 9.7+): https://issues.redhat.com/browse/RHEL-88789 I think my new recommendation here is that we:
|
Add azure-vm-utils package to RHEL 10.1 manifest to replace custom Azure udev rules with upstream rules from the azure-vm-utils package. This provides proper Azure VM disk/device configuration including support for NVMe devices via azure-nvme-id. The azure-vm-utils package is only available in RHEL 10.1 as of now which is why it is added only to manifest-rhel-10.1.yaml. This replaces the custom overlay udev rules previously maintained in overlay.d/25rhcos-azure-udev/. Fixes: https://issues.redhat.com/browse/COS-2074 Related: openshift/os#1776
9ecfd72 to
0aa95d8
Compare
|
@aaradhak: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Add azure-vm-utils package to RHEL 10.1 manifest to replace custom Azure udev rules with upstream rules from the
azure-vm-utilspackage. This provides proper Azure VM disk/device configuration including support for NVMe devices via azure-nvme-id.The
azure-vm-utilspackage is only available in RHEL 10.1 as of now which is why it is added only to manifest-rhel-10.1.yaml. This replaces the custom overlay udev rules previously maintained inoverlay.d/25rhcos-azure-udev/.Fixes: https://issues.redhat.com/browse/COS-2074
Related: openshift/os#1776