Conversation
fixes Internal(Vm(SetMemoryAttributes(Error(22)))) issue. libkrun 1.17 includes the issue but currently it's in rawhide only. Signed-off-by: Douglas Schilling Landgraf <dougsland@redhat.com>
Signed-off-by: Douglas Schilling Landgraf <dougsland@redhat.com>
dougsland
requested review from
Yarboa,
aesteve-rh,
kleinffm,
nsednev,
pengshanyu and
rhatdan
as code owners
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the libkrun installation script to pull packages from Fedora rawhide on Fedora-based systems and adds some debug output, plus a minor README heading change. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @dougsland, 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 primarily refines the installation process for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The README heading was renamed to
# Topicstestbut the table-of-contents link still points to#topics, which will break the anchor; either revert the heading or adjust the link target to match. - The
install_libkrunfunction now always installslibkrunfrom rawhide (--releasever=rawhide --enablerepo=rawhide), which may not exist or be appropriate on non-Fedora systems; consider gating the rawhide-specific flags behind the same Fedora check or a separate configuration knob. - The new
exec_cmd "cat /usr/lib/qm/rootfs/etc/yum.repos.d/*"anddnf repolistcalls add a lot of noisy output to the test logs; if these are only for debugging, consider guarding them with a verbosity/debug flag or removing them before merging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The README heading was renamed to `# Topicstest` but the table-of-contents link still points to `#topics`, which will break the anchor; either revert the heading or adjust the link target to match.
- The `install_libkrun` function now always installs `libkrun` from rawhide (`--releasever=rawhide --enablerepo=rawhide`), which may not exist or be appropriate on non-Fedora systems; consider gating the rawhide-specific flags behind the same Fedora check or a separate configuration knob.
- The new `exec_cmd "cat /usr/lib/qm/rootfs/etc/yum.repos.d/*"` and `dnf repolist` calls add a lot of noisy output to the test logs; if these are only for debugging, consider guarding them with a verbosity/debug flag or removing them before merging.
## Individual Comments
### Comment 1
<location> `README.md:1` </location>
<code_context>
-# Topics
+# Topicstest
- [Topics](#topics)
</code_context>
<issue_to_address>
**issue (bug_risk):** Heading text likely contains a typo and now mismatches the table-of-contents anchor link.
The new heading `# Topicstest` looks accidental and now conflicts with the TOC link `#topics`, so the anchor will break. Please update the heading (e.g. back to `# Topics` or the intended text) and align the TOC link text/anchor with it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request updates the libkrun installation logic, particularly for Fedora rawhide environments within the check_libkrun.sh test script, and makes a minor adjustment to the README.md heading. The changes in check_libkrun.sh introduce conditional installation of fedora-repos-rawhide and install libkrun, crun-krun, and libkrunfw using rawhide repositories. However, the removal of libkrun and crun-krun from the main setup script might be an oversight if these are core dependencies, and the unconditional call to enable_repo in check_libkrun.sh might lead to incorrect repository enablement on non-CentOS systems.
| fi | ||
|
|
||
| cmd_dnf_install="dnf -y install --releasever=${VERSION_ID} --setopt=reposdir=${ROOTFS}/etc/yum.repos.d --installroot ${ROOTFS} libkrun crun-krun bluechi-agent ${EXTRA_FLAG}" | ||
| cmd_dnf_install="dnf -y install --releasever=${VERSION_ID} --setopt=reposdir=${ROOTFS}/etc/yum.repos.d --installroot ${ROOTFS} bluechi-agent ${EXTRA_FLAG}" |
There was a problem hiding this comment.
The libkrun and crun-krun packages have been removed from this dnf install command in the main setup script. If these are core dependencies for QM, they should be installed by the primary setup script. While check_libkrun.sh now installs them, that is a test script, not the main installer. This could lead to missing dependencies for a full QM setup if libkrun is expected to be present by default.
| cmd_dnf_install="dnf -y install --releasever=${VERSION_ID} --setopt=reposdir=${ROOTFS}/etc/yum.repos.d --installroot ${ROOTFS} bluechi-agent ${EXTRA_FLAG}" | |
| cmd_dnf_install="dnf -y install --releasever=${VERSION_ID} --setopt=reposdir=${ROOTFS}/etc/yum.repos.d --installroot ${ROOTFS} libkrun crun-krun bluechi-agent ${EXTRA_FLAG}" |
| #if grep -qi "^ID=centos" /etc/os-release; then | ||
| # enable_repo | ||
| #fi |
There was a problem hiding this comment.
The conditional block for enabling the CentOS-specific libkrun COPR repository was commented out, but the enable_repo function is still called unconditionally at line 35. The enable_repo function specifically targets centos-stream-9-$(arch). This means that if this script is run on a Fedora system, it will attempt to enable a CentOS COPR repo, which is likely incorrect and could lead to errors or unexpected behavior. The enable_repo call should remain conditional based on the OS ID.
| #if grep -qi "^ID=centos" /etc/os-release; then | |
| # enable_repo | |
| #fi | |
| if grep -qi "^ID=centos" /etc/os-release; then | |
| enable_repo | |
| fi |
| @@ -1,4 +1,4 @@ | |||
| # Topics | |||
| # Topicstest | |||
There was a problem hiding this comment.
The change from # Topics to # Topicstest appears to be a typo. Please correct this to the intended heading.
| # Topicstest | |
| # Topics |
| # Adding rawhide as 1.17 fixes Internal(Vm(SetMemoryAttributes(Error(22)))) | ||
| # See-Also: https://github.com/containers/qm/pull/959 | ||
| if grep -qi "^ID=fedora" /etc/os-release; then | ||
| # Need the remove as soon the land in stable channel |
There was a problem hiding this comment.
The comment # Need the remove as soon the land in stable channel indicates a temporary solution. It would be beneficial to track this as a proper TODO or issue in the project's issue tracker to ensure it's addressed when libkrun 1.17 lands in a stable channel.
1 participant
Summary by Sourcery
Update libkrun installation to use Fedora rawhide repositories when appropriate and make a minor README heading adjustment.
Enhancements:
Documentation: