QA fixups and selinux changes for chrony PMDA - #2660
Merged
Conversation
- fix typo in common.bpftrace diagnostic - rework logic in 1721 to track biolatency.bt being replaced by multiple versions of this file
The xfs metric names order seems to have some dependency on directory order for the "fake" stats files ... take control and make the order of metrics to be fetched deterministic. Also added per-tarball _triage_wait_point using $seq.wait.$iter.
If filename is given, look for it in $here, just like the default $seq.wait.
Specifically one-liners to answer the question "which installed packages are using the most disk space?".
Same problem and same fix as was previously triaged for qa/1290.
I now believe this line:
Warning: invalid file descriptor -1 in syscall fstat()
comes from valgrind on x86_64 RHEL 9.8 and (some versions) of x86_64
Ubuntu 26.04 ... the non-valgrind "dual" tests don't show the warning,
only the valgind variants do.
Same fix as previously applied for qa/987.
… PMDA
1. on RHEL 9.8 the construct
ifdef(`kernel_io_uring_use', ...)
fails because the interface is defined but it expands to use
io_uring_t which is not defined ... this involves a fair amount of
digging starting with
$ sudo semodule -X 200 -i pcp.pp
Failed to resolve typeattributeset statement at /var/lib/selinux/targeted/tmp/modules/200/pcp/cil:118
Failed to resolve AST
semodule: Failed!
replace the ifdef() with optional_policy() [like we use elswhere
for other interface uses] seems to fix this.
2. there were a stack of extra AVCs from qa/1686 and the chrony PMDA
... it took several iterations before I stopped seeing new AVCs
so I suspect there are multiple code paths in this PMDA and their
execution while running qa/1686 is not deterministic, so repeated
runs see potentially different AVCs
Also tidied up the GNUmakefile to remove duplicate definitions Warnings
that are caused by other people's policies and have nothing to do with
PCP.
Conflicts:
src/selinux/GNUmakefile
Resolved diffs.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe pull request updates QA scripts for deterministic metric checks, Valgrind output filtering, bpftrace setup, wait-file handling, and callback accounting. It also revises VM package guidance and updates SELinux build recipes and PCP policy permissions. QA validation updates
SELinux policy updates
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qa/common.check (1)
3849-3852: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve
$seq.waitrelative to$herein the 0-argument case.The changes updated the 2-argument branch to explicitly prepend
$here/to the wait file, keeping it consistent with the 1-argument branch and the function documentation. However, the 0-argument fallback case was missed and still resolves$seq.waitin the current working directory, which might lead to unexpected behavior if the script changes directories.🛠️ Proposed fix
else - __file=$seq.wait + __file="$here/$seq.wait" __msg="" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/common.check` around lines 3849 - 3852, Update the 0-argument fallback in the argument-handling branch to resolve the wait file as relative to $here, matching the existing 1-argument and 2-argument cases; change the $seq.wait assignment while preserving the empty $seq.msg initialization.
🧹 Nitpick comments (1)
qa/README.vm (1)
289-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProvide the concrete package sorting commands.
The comment
# sort packages on installed size, largest to smallestwas added to several distribution sections, but the actual commands were omitted. Consider filling these in for completeness:
qa/README.vm#L289-L289: add the command for FreeBSD (e.g.,pkg query '%as %n' | sort -nr).qa/README.vm#L337-L337: add the command for NetBSD (e.g.,pkg_info -as | sort -nr).qa/README.vm#L366-L366: add the command for Gentoo (e.g.,qsize -as | sort -nr).qa/README.vm#L444-L444: add the command for Arch Linux (e.g.,expac -H M '%m\t%n' | sort -hrorpacman -Qi).qa/README.vm#L479-L479: add the command for OpenIndiana.qa/README.vm#L509-L509: add the command for OpenBSD.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/README.vm` at line 289, Complete each “sort packages on installed size, largest to smallest” section in qa/README.vm by adding the appropriate concrete sorting command: FreeBSD at lines 289-289, NetBSD at 337-337, Gentoo at 366-366, Arch Linux at 444-444, OpenIndiana at 479-479, and OpenBSD at 509-509. Use each platform’s package-management tooling and ensure output is sorted by installed size in descending order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/selinux/GNUmakefile`:
- Line 49: Update the SELinux policy compilation recipe in GNUmakefile to
capture the output of the $(MAKE) invocation in a temporary file, preserve and
explicitly propagate its exit status, then filter the captured output to
suppress only the specified duplicate-definition warnings. Ensure successful
builds remain successful even when all output is filtered, and failed
compilations still fail the recipe; clean up the temporary file in all cases.
In `@src/selinux/pcp.te`:
- Around line 116-118: Guard both kernel_io_uring_use calls with an ifdef before
their existing optional_policy blocks: update src/selinux/pcp.te lines 116-118
for pcp_pmcd_t and lines 234-236 for pcp_pmproxy_t. Preserve optional_policy
inside each ifdef so builds without the macro remain compatible while the
dependency stays optional at load time.
---
Outside diff comments:
In `@qa/common.check`:
- Around line 3849-3852: Update the 0-argument fallback in the argument-handling
branch to resolve the wait file as relative to $here, matching the existing
1-argument and 2-argument cases; change the $seq.wait assignment while
preserving the empty $seq.msg initialization.
---
Nitpick comments:
In `@qa/README.vm`:
- Line 289: Complete each “sort packages on installed size, largest to smallest”
section in qa/README.vm by adding the appropriate concrete sorting command:
FreeBSD at lines 289-289, NetBSD at 337-337, Gentoo at 366-366, Arch Linux at
444-444, OpenIndiana at 479-479, and OpenBSD at 509-509. Use each platform’s
package-management tooling and ensure output is sorted by installed size in
descending order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c3e83684-2987-4da1-9bbf-a028f7d05a1f
⛔ Files ignored due to path filters (2)
qa/987.outis excluded by!**/*.outqa/988.outis excluded by!**/*.out
📒 Files selected for processing (13)
qa/1290qa/1307qa/1438qa/1471qa/1721qa/987qa/988qa/README.vmqa/checkqa/common.bpftraceqa/common.checksrc/selinux/GNUmakefilesrc/selinux/pcp.te
| @test -d tmp || mkdir tmp # workaround an el7 selinux makefile issue | ||
| # lots of duplicate definition from system container.if - filter out | ||
| $(MAKE) -f /usr/share/selinux/devel/Makefile $(IAM).pp 2>&1 | grep -v container.if | ||
| $(MAKE) -f /usr/share/selinux/devel/Makefile $(IAM).pp 2>&1 | grep -v -E '/(container|zabbix|nbdkit|passt|smartmon|insights_core)\.if:[0-9]*: Warning: duplicate definition' |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve the make exit status when filtering warnings.
Piping $(MAKE) directly into grep without pipefail masks the exit status of make. If the SELinux policy compilation fails, grep -v will still exit with 0 (since it outputs the build errors which don't match the regex), causing this Makefile recipe to falsely report success. This can lead to stale .pp files being silently packaged without developers noticing the failure.
Additionally, if make succeeds and outputs only warnings which are completely filtered out, grep -v will exit with 1, causing the build to falsely fail.
Store the output in a temporary file and explicitly preserve the original make exit status:
🛠 Proposed fix
- $(MAKE) -f /usr/share/selinux/devel/Makefile $(IAM).pp 2>&1 | grep -v -E '/(container|zabbix|nbdkit|passt|smartmon|insights_core)\.if:[0-9]*: Warning: duplicate definition'
+ @$(MAKE) -f /usr/share/selinux/devel/Makefile $(IAM).pp > tmp/build.log 2>&1; \
+ RET=$$?; \
+ grep -v -E '/(container|zabbix|nbdkit|passt|smartmon|insights_core)\.if:[0-9]*: Warning: duplicate definition' tmp/build.log || true; \
+ exit $$RET📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $(MAKE) -f /usr/share/selinux/devel/Makefile $(IAM).pp 2>&1 | grep -v -E '/(container|zabbix|nbdkit|passt|smartmon|insights_core)\.if:[0-9]*: Warning: duplicate definition' | |
| @$(MAKE) -f /usr/share/selinux/devel/Makefile $(IAM).pp > tmp/build.log 2>&1; \ | |
| RET=$$?; \ | |
| grep -v -E '/(container|zabbix|nbdkit|passt|smartmon|insights_core)\.if:[0-9]*: Warning: duplicate definition' tmp/build.log || true; \ | |
| exit $$RET |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/selinux/GNUmakefile` at line 49, Update the SELinux policy compilation
recipe in GNUmakefile to capture the output of the $(MAKE) invocation in a
temporary file, preserve and explicitly propagate its exit status, then filter
the captured output to suppress only the specified duplicate-definition
warnings. Ensure successful builds remain successful even when all output is
filtered, and failed compilations still fail the recipe; clean up the temporary
file in all cases.
As pointed out by @coderrabbitai in the context of PR performancecopilot#2660 it is a bad idea to use the construct $(MAKE) ... | grep in a makefile rule because the result (as far as make is concerned) moves from the exit status of $(MAKE) to the exit status of grep. Simply dropping the | grep part of the rule is the right fix. We are already catching and culling these triaged (not-a-PCP-issue) selinux build warnings in scripts/cull-build-warnings. Same change in src/selinux/import/GNUmakefile
As suggested by @coderrabbitai in the context of PR performancecopilot#2660, I had missed setting the "$here/" prefix for the wait file on the no arguments code path ... fortunately this is usage of _triage_wait_point() rare (maybe never?) and not very useful in practice.
@coderabbitai suggestion to avoid build failure on RHEL 7 (and older) where kernel_io_uring_use() is not defined.
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
As per title.