Revert "DS Merge 07/24/2026" (#335) - #338
Conversation
This reverts commit f099946, the upstream merge from 07/24/2026. Reverting to bisect a podReadyLatency_P99 regression (+64%) detected in the udn-density-pods workload on OCP 5.0 nightlies starting around 2026-07-15. See PERFSCALE-5389 for details.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThe PR updates DRA resource mapping and CNI configuration handling, removes obsolete DRA client paths, changes metrics behavior, moves builds to OpenShift 4.22 and Go 1.25, updates workflow action references, and adjusts deployment and E2E manifests. ChangesRuntime and DRA changes
Build and workflow delivery
Deployment and test manifest updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redhat-chai-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
deployments/multus-daemonset.yml (1)
65-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore Multus pod list/watch permissions in the split rule
deployments/multus-daemonset.yml#L65-L72anddeployments/multus-daemonset-crio.yml#L65-L72combinepodsandpods/statusas verbs are written as- getand- update, which does not providelist/watch. Split the rules so the workload retains the Multus-required pod list/watch access while keepingpods/statuslimited togetandupdate.🤖 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 `@deployments/multus-daemonset.yml` around lines 65 - 72, In deployments/multus-daemonset.yml lines 65-72 and deployments/multus-daemonset-crio.yml lines 65-72, split the pods and pods/status RBAC entries into separate rules: grant pods get, list, update, and watch, while keeping pods/status restricted to get and update.Source: Path instructions
pkg/k8sclient/k8sclient.go (1)
295-302: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
errMsgpassed as a format string can corrupt the emitted event message.
errMsgis later used asmessageFmtwith no args; any%in the wrapped error text (e.g. fromerr,net.Name, ornet.Namespace) will be misinterpreted as a format verb, producing garbled Kubernetes event text (e.g.%!s(MISSING)).🐛 Proposed fix
- client.Eventf(pod, v1.EventTypeWarning, "NoNetworkFound", errMsg) + client.Eventf(pod, v1.EventTypeWarning, "NoNetworkFound", "%s", errMsg)🤖 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 `@pkg/k8sclient/k8sclient.go` around lines 295 - 302, Update the Eventf call in the GetNetAttachDef error path to pass a constant format string and supply errMsg as its argument, rather than using errMsg directly as the format string. Keep the existing errMsg construction and returned error unchanged.pkg/types/conf.go (1)
85-109: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve raw plugin JSON when creating delegate bytes.
ConvertNetworkConfigListToNetConfList()drops the libcniPluginConfig.Bytes; laterjson.Marshal(netConfList)only writes fields defined ontypes.NetConfListandtypes.PluginConf. Top-level keys likerawConfListBytes/InjectCNIVersionInConfListor arbitrary extra plugin JSON are lost fromdelegateConf.Bytes, so construct/drop rawbytesafter marshaling but before the optional deviceID/cni-args mutation.🤖 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 `@pkg/types/conf.go` around lines 85 - 109, Update LoadDelegateNetConfFromConfList to preserve the original raw plugin JSON when constructing delegateConf.Bytes. After marshaling netConfList, merge or reconstruct the raw conf-list and plugin bytes from confList so top-level and arbitrary plugin fields are retained, then perform any existing deviceID or CNI-args mutation afterward.pkg/types/types.go (1)
101-126: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid persisting the duplicate
CNINetworkConfigListin cached delegates.
LoadDelegateNetConfFromConfListpopulates bothdelegateConf.ConfListanddelegateConf.CNINetworkConfigList, andsaveDelegatesmarshals the fullDelegateNetConfslice to the CNI cache. Since the cache is loaded back onCmdDel, this adds a redundant fulllibcni.NetworkConfigList, including its ownBytesand plugin bytes, to every cached add/del path for conflist delegates. Remove or omit this field from the persisted delegate representation to avoid multiplying cache size.🤖 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 `@pkg/types/types.go` around lines 101 - 126, Exclude DelegateNetConf.CNINetworkConfigList from JSON persistence while retaining it for runtime use by LoadDelegateNetConfFromConfList and related flows. Update the field’s serialization metadata so saveDelegates no longer writes the duplicate network configuration and cache loading remains compatible.
🧹 Nitpick comments (3)
docs/how-to-use.md (1)
677-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the new code fences with the repository’s markdownlint configuration.
These four fenced examples trigger MD040/MD046. Either convert them to the configured indented style or update the markdownlint configuration and add language identifiers where fenced blocks remain.
Also applies to: 699-699, 728-728, 746-746
🤖 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 `@docs/how-to-use.md` at line 677, Update the four fenced examples in docs/how-to-use.md, including the blocks near lines 677, 699, 728, and 746, to comply with the repository’s markdownlint configuration by converting them to the configured indented style or retaining fences with explicit language identifiers and the necessary lint configuration.Source: Linters/SAST tools
pkg/server/server.go (1)
513-520: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sets.NewStringis deprecated in current apimachinery; prefer the genericSet[string].Deprecated: use generic Set instead.
♻️ Suggested refactor
- ignoreKeys := sets.NewString() + ignoreKeys := sets.New[string]() if ignoreReadinessIndicator { ignoreKeys.Insert("readinessindicatorfile") }🤖 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 `@pkg/server/server.go` around lines 513 - 520, Replace the deprecated sets.NewString usage in the override-copy logic with the generic sets.Set[string] type and its supported constructor/initialization. Preserve the existing ignoreKeys.Insert and ignoreKeys.Has behavior, including conditional exclusion of readinessindicatorfile.pkg/kubeletclient/kubeletclient.go (1)
160-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional:
strings.Cutreads more clearly thanSplit+length-check for a single separator.♻️ Optional refactor
- res := strings.Split(cdiDevice.Name, "=") - if len(res) == 2 { - deviceIDs = append(deviceIDs, res[1]) - } else { - logging.Errorf("GetPodResourceMap: Invalid CDI format") - } + if _, id, ok := strings.Cut(cdiDevice.Name, "="); ok { + deviceIDs = append(deviceIDs, id) + } else { + logging.Errorf("GetPodResourceMap: Invalid CDI format") + }🤖 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 `@pkg/kubeletclient/kubeletclient.go` around lines 160 - 179, Optionally simplify CDI name parsing in getDRAResources by replacing strings.Split plus the len check with strings.Cut for the single “=” separator, preserving the existing device-ID extraction and invalid-format logging behavior.
🤖 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 @.github/workflows/codeql.yml:
- Around line 29-39: Update the CodeQL workflow steps Initialize CodeQL,
Autobuild, and Perform CodeQL Analysis to a supported github/codeql-action major
version, and pin each action reference to its complete commit SHA instead of the
current `@v2` tags. Keep the existing language and queries configuration
unchanged.
- Around line 27-39: Pin every active uses: action reference to an immutable
full commit SHA, replacing tag-based references while preserving each action and
workflow behavior. Apply this across .github/workflows/codeql.yml:27-39,
image-push-master.yml:14-29, 46-61, 75-75, image-push-release.yml:14-37, 54-77,
91-91, kind-e2e.yml:36-42, 52-52, 111-111, and stale-issues-prs.yml:10-10;
update all affected action invocations, including Initialize CodeQL, Autobuild,
and Perform CodeQL Analysis, without changing their configuration.
In `@cmd/cert-approver/main.go`:
- Line 341: Fix the Eventf call in the CSR denial handling by making its format
string match the supplied arguments, ensuring the denial message is
intentionally included and no extra-argument formatting artifact is produced.
Update the call containing "CSRDenied" while preserving the existing event type
and context.
In `@cmd/multus-daemon/main.go`:
- Around line 171-177: Update the metrics-serving block around
utilwait.UntilWithContext so the /metrics handler is registered only once on a
dedicated ServeMux, avoiding repeated global http.Handle registration across
retries. Preserve retry behavior while wiring the metrics server to ctx
cancellation so shutdown is graceful via the server’s Shutdown method.
In `@deployments/multus-daemonset.yml`:
- Line 22: Restore the nad short name alongside net-attach-def in the
deployments/multus-daemonset.yml CRD definition at lines 22-22 and
deployments/multus-daemonset-crio.yml at lines 22-22, preserving kubectl get nad
compatibility in both manifests.
In `@Dockerfile.microshift`:
- Around line 14-17: Update the multi-source COPY instruction in
Dockerfile.microshift to use an explicit trailing slash on the destination
directory `/usr/src/multus-cni/bin/`, preserving both source files unchanged.
In `@docs/how-to-use.md`:
- Around line 742-744: Update the “Launch pod with DRA Resource Claim” example
description to use grammatically correct wording: “The following command
launches a Pod with a primary network …”, correcting “primiry” to “primary” and
preserving the existing network and ResourceClaim names.
- Around line 688-707: Update the “Create network attachment definition with
resource name” documentation to state that k8s.v1.cni.cncf.io/resourceName must
use the ResourceClaim name, not the ResourceClass name, and change the example
annotation from sf-pool-1 to sf. Keep the explanation consistent with the
kubelet resource map lookup keyed by DynamicResource.ClaimName.
In `@pkg/multus/multus.go`:
- Around line 299-318: The conflistAdd function currently prefers the stale
cniConfList whenever it already has plugins, ignoring the updated rawnetconflist
bytes passed by DelegateAdd. Change this selection path to parse and use
rawnetconflist via libcni.NetworkConfFromBytes for conflist delegates, while
preserving the existing error handling and only reusing cniConfList when the
delegate bytes are not the source of truth.
In `@pkg/server/server.go`:
- Around line 498-501: The empty override path in
overrideCNIConfigWithServerConfig bypasses readinessindicatorfile stripping.
Ensure ignoreReadinessIndicator is applied even when overrideConf is empty by
normalizing the empty override to an empty JSON object or handling that path
separately, while preserving the existing return behavior when stripping is not
requested.
---
Outside diff comments:
In `@deployments/multus-daemonset.yml`:
- Around line 65-72: In deployments/multus-daemonset.yml lines 65-72 and
deployments/multus-daemonset-crio.yml lines 65-72, split the pods and
pods/status RBAC entries into separate rules: grant pods get, list, update, and
watch, while keeping pods/status restricted to get and update.
In `@pkg/k8sclient/k8sclient.go`:
- Around line 295-302: Update the Eventf call in the GetNetAttachDef error path
to pass a constant format string and supply errMsg as its argument, rather than
using errMsg directly as the format string. Keep the existing errMsg
construction and returned error unchanged.
In `@pkg/types/conf.go`:
- Around line 85-109: Update LoadDelegateNetConfFromConfList to preserve the
original raw plugin JSON when constructing delegateConf.Bytes. After marshaling
netConfList, merge or reconstruct the raw conf-list and plugin bytes from
confList so top-level and arbitrary plugin fields are retained, then perform any
existing deviceID or CNI-args mutation afterward.
In `@pkg/types/types.go`:
- Around line 101-126: Exclude DelegateNetConf.CNINetworkConfigList from JSON
persistence while retaining it for runtime use by
LoadDelegateNetConfFromConfList and related flows. Update the field’s
serialization metadata so saveDelegates no longer writes the duplicate network
configuration and cache loading remains compatible.
---
Nitpick comments:
In `@docs/how-to-use.md`:
- Line 677: Update the four fenced examples in docs/how-to-use.md, including the
blocks near lines 677, 699, 728, and 746, to comply with the repository’s
markdownlint configuration by converting them to the configured indented style
or retaining fences with explicit language identifiers and the necessary lint
configuration.
In `@pkg/kubeletclient/kubeletclient.go`:
- Around line 160-179: Optionally simplify CDI name parsing in getDRAResources
by replacing strings.Split plus the len check with strings.Cut for the single
“=” separator, preserving the existing device-ID extraction and invalid-format
logging behavior.
In `@pkg/server/server.go`:
- Around line 513-520: Replace the deprecated sets.NewString usage in the
override-copy logic with the generic sets.Set[string] type and its supported
constructor/initialization. Preserve the existing ignoreKeys.Insert and
ignoreKeys.Has behavior, including conditional exclusion of
readinessindicatorfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@b8d3b6e8af63cde30bdc382c0bc28114f4346c88 # v2 | ||
| uses: github/codeql-action/init@v2 | ||
| with: | ||
| languages: ${{ matrix.language }} | ||
| queries: +security-and-quality | ||
|
|
||
| - name: Autobuild | ||
| uses: github/codeql-action/autobuild@b8d3b6e8af63cde30bdc382c0bc28114f4346c88 # v2 | ||
| uses: github/codeql-action/autobuild@v2 | ||
|
|
||
| - name: Perform CodeQL Analysis | ||
| uses: github/codeql-action/analyze@b8d3b6e8af63cde30bdc382c0bc28114f4346c88 # v2 | ||
| uses: github/codeql-action/analyze@v2 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked workflow files:\n'
git ls-files .github/workflows | sed -n '1,120p'
printf '\nAction refs in workflows (uses line):\n'
rg -n 'uses:\s*[^#\s]+' .github/workflows || true
printf '\nWorkflow snippets with line numbers:\n'
for f in .github/workflows/codeql.yml .github/workflows/image-push-master.yml .github/workflows/image-push-release.yml .github/workflows/kind-e2e.yml .github/workflows/stale-issues-prs.yml; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '1,180p'
else
echo "missing $f"
fi
done
printf '\nClassify each uses ref as tag-only vs full commit-ish vs local:\n'
python3 - <<'PY'
import os, re
root=".github/workflows"
for fn in sorted(os.listdir(root)):
p=os.path.join(root,fn)
if not os.path.isfile(p): continue
with open(p, encoding="utf-8") as f:
for i,line in enumerate(f,1):
m=re.search(r'uses:\s*([^\s#]+)', line)
if not m: continue
ref=m.group(1)
if ref.startswith('./'):
cat='local'
else:
parts=ref.rsplit('@',1)
token=parts[1] if len(parts)==2 else ''
if re.fullmatch(r'[0-9a-fA-F]{40}', token):
cat='full_sha'
elif ':' in token or ref.startswith('git://')):
cat='url'
else:
cat='tag_or_branch'
print(f"{p}:{i}:{cat}:{ref}")
PY
printf '\nDiff summary focused on workflows:\n'
git diff --stat -- .github/workflows || true
git diff -- .github/workflows | sed -n '1,240p' || trueRepository: openshift/multus-cni
Length of output: 5363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Classify workflow action refs:\n'
python3 - <<'PY'
import os, re
root=".github/workflows"
tag_or_branch_pat=re.compile(r'^[A-Za-z._0-9-]+$|^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$')
for fn in sorted(os.listdir(root)):
p=os.path.join(root,fn)
if not os.path.isfile(p): continue
with open(p, encoding="utf-8", errors="replace") as f:
for i,line in enumerate(f,1):
m=re.search(r'uses:\s*([^,\s#\r\n]+)', line)
if not m: continue
ref=m.group(1).strip()
if ref.startswith('./') or ref.startswith('.\\'):
cat='local'
ref='.'.join(ref.split('@',1))
else:
if ref.count('@') < 1:
cat='no_at_ref'
else:
parts=ref.rsplit('@',1)
token=parts[1]
if re.fullmatch(r'[0-9a-fA-F]{40}', token):
cat='full_sha'
elif ':' in token or token.startswith('git://'):
cat='url'
elif re.fullmatch(r'aqua-security/trivy-action/v[\d.]+', token):
cat='semver_tag'
elif re.fullmatch(r'[\d.]+', token) or token.startswith('v') and set(token[1:]) <= set('0123456789.'):
cat='semver_like_tag'
elif tag_or_branch_pat.fullmatch(token):
cat='tag_or_branch'
else:
cat='other'
print(f"{p}:{i}:{cat}:{ref}")
PY
printf '\nFocused workflow contents:\n'
for f in .github/workflows/codeql.yml \
.github/workflows/image-push-master.yml \
.github/workflows/image-push-release.yml \
.github/workflows/kind-e2e.yml \
.github/workflows/stale-issues-prs.yml \
.github/workflows/image-build.yml \
.github/workflows/test.yml \
.github/workflows/release.yml \
.github/workflows/build.yml; do
if [ -f "$f" ]; then
echo "--- $f ---"
awk '{printf "%6d %s\n", NR, $0}' "$f"
fi
doneRepository: openshift/multus-cni
Length of output: 29395
Restore immutable GitHub Actions references across the workflows.
The workflow files run action tags (@v*, major version tags, and semver-like tags) instead of full commit SHAs, so CI/E2E/image-publishing actions can change without review. Pin every active uses: action reference to its full commit SHA.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 30-30: the runner of "github/codeql-action/init@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
[error] 36-36: the runner of "github/codeql-action/autobuild@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
[error] 39-39: the runner of "github/codeql-action/analyze@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📍 Affects 5 files
.github/workflows/codeql.yml#L27-L39(this comment).github/workflows/image-push-master.yml#L14-L29.github/workflows/image-push-master.yml#L46-L61.github/workflows/image-push-master.yml#L75-L75.github/workflows/image-push-release.yml#L14-L37.github/workflows/image-push-release.yml#L54-L77.github/workflows/image-push-release.yml#L91-L91.github/workflows/kind-e2e.yml#L36-L42.github/workflows/kind-e2e.yml#L52-L52.github/workflows/kind-e2e.yml#L111-L111.github/workflows/stale-issues-prs.yml#L10-L10
🤖 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 @.github/workflows/codeql.yml around lines 27 - 39, Pin every active uses:
action reference to an immutable full commit SHA, replacing tag-based references
while preserving each action and workflow behavior. Apply this across
.github/workflows/codeql.yml:27-39, image-push-master.yml:14-29, 46-61, 75-75,
image-push-release.yml:14-37, 54-77, 91-91, kind-e2e.yml:36-42, 52-52, 111-111,
and stale-issues-prs.yml:10-10; update all affected action invocations,
including Initialize CodeQL, Autobuild, and Perform CodeQL Analysis, without
changing their configuration.
Source: Path instructions
| - name: Initialize CodeQL | ||
| uses: github/codeql-action/init@b8d3b6e8af63cde30bdc382c0bc28114f4346c88 # v2 | ||
| uses: github/codeql-action/init@v2 | ||
| with: | ||
| languages: ${{ matrix.language }} | ||
| queries: +security-and-quality | ||
|
|
||
| - name: Autobuild | ||
| uses: github/codeql-action/autobuild@b8d3b6e8af63cde30bdc382c0bc28114f4346c88 # v2 | ||
| uses: github/codeql-action/autobuild@v2 | ||
|
|
||
| - name: Perform CodeQL Analysis | ||
| uses: github/codeql-action/analyze@b8d3b6e8af63cde30bdc382c0bc28114f4346c88 # v2 | ||
| uses: github/codeql-action/analyze@v2 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '1,120p' .github/workflows/codeql.yml | cat -n
echo
echo "== actionlint availability/name =="
command -v actionlint || true
echo
echo "== workflow/actionlint usage in repo =="
rg -n "actionlint|github/codeql-action|codeql" .github/workflows || trueRepository: openshift/multus-cni
Length of output: 1719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== actionlint version =="
actionlint -version || true
echo
echo "== actionlint on codeql.yml =="
actionlint .github/workflows/codeql.yml
echo
echo "== all action pins in workflows =="
python3 - <<'PY'
from pathlib import Path
import re
for p in sorted(Path(".github/workflows").glob("*.yml")) + sorted(Path(".github/workflows").glob("*.yaml")):
txt = p.read_text()
for i,line in enumerate(txt.splitlines(), 1):
m=re.search(r"uses:\s*([^\s]+)@([^\s#]+)", line)
if m:
print(f"{p}:{i}:{m.group(1)}@{m.group(2)}")
PY
echo
echo "== fetch public codeql-action v2 ref object =="
if command -v gh >/dev/null 2>&1; then
gh api repos/github/codeql-action/git/refs/tags/v2 --jq '{ref, object}'
else
echo "gh not available"
fiRepository: openshift/multus-cni
Length of output: 1177
Upgrade the CodeQL action version.
github/codeql-action/init@v2, autobuild@v2, and analyze@v2 are too old for GitHub Actions runners and violate the CI/CD requirement to pin actions by full SHA. Move these steps to a supported major version and pin them to full commit SHAs.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 30-30: the runner of "github/codeql-action/init@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
[error] 36-36: the runner of "github/codeql-action/autobuild@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
[error] 39-39: the runner of "github/codeql-action/analyze@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 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 @.github/workflows/codeql.yml around lines 29 - 39, Update the CodeQL
workflow steps Initialize CodeQL, Autobuild, and Perform CodeQL Analysis to a
supported github/codeql-action major version, and pin each action reference to
its complete commit SHA instead of the current `@v2` tags. Keep the existing
language and queries configuration unchanged.
Source: Linters/SAST tools
| ) | ||
|
|
||
| c.recorder.Eventf(csr, corev1.EventTypeWarning, "CSRDenied", "The CSR %q has been denied by %s: %s", csr.Name, ControllerName, message) | ||
| c.recorder.Eventf(csr, corev1.EventTypeWarning, "CSRDenied", "The CSR %q has been denied by: %s", csr.Name, ControllerName, message) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Format string has 2 verbs but 3 args — message will render as a %!(EXTRA ...) artifact.
🐛 Proposed fix
- c.recorder.Eventf(csr, corev1.EventTypeWarning, "CSRDenied", "The CSR %q has been denied by: %s", csr.Name, ControllerName, message)
+ c.recorder.Eventf(csr, corev1.EventTypeWarning, "CSRDenied", "The CSR %q has been denied by %s: %s", csr.Name, ControllerName, message)📝 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.
| c.recorder.Eventf(csr, corev1.EventTypeWarning, "CSRDenied", "The CSR %q has been denied by: %s", csr.Name, ControllerName, message) | |
| c.recorder.Eventf(csr, corev1.EventTypeWarning, "CSRDenied", "The CSR %q has been denied by %s: %s", csr.Name, ControllerName, message) |
🤖 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 `@cmd/cert-approver/main.go` at line 341, Fix the Eventf call in the CSR denial
handling by making its format string match the supplied arguments, ensuring the
denial message is intentionally included and no extra-argument formatting
artifact is produced. Update the call containing "CSRDenied" while preserving
the existing event type and context.
| if daemonConfig.MetricsPort != nil { | ||
| mux := http.NewServeMux() | ||
| mux.Handle("/metrics", promhttp.Handler()) | ||
| if daemonConfig.EnablePprof != nil && *daemonConfig.EnablePprof { | ||
| mux.HandleFunc("/debug/pprof/", pprof.Index) | ||
| mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) | ||
| mux.HandleFunc("/debug/pprof/profile", pprof.Profile) | ||
| mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) | ||
| mux.HandleFunc("/debug/pprof/trace", pprof.Trace) | ||
| logging.Verbosef("pprof endpoints enabled on metrics port %d", *daemonConfig.MetricsPort) | ||
| } | ||
| metricsSrv := &http.Server{ | ||
| Addr: fmt.Sprintf(":%d", *daemonConfig.MetricsPort), | ||
| Handler: mux, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| } | ||
| logging.Debugf("metrics port: %d", *daemonConfig.MetricsPort) | ||
| go utilwait.UntilWithContext(ctx, func(_ context.Context) { | ||
| if err := metricsSrv.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||
| logging.Debugf("metrics server error: %v", err) | ||
| } | ||
| http.Handle("/metrics", promhttp.Handler()) | ||
| logging.Debugf("metrics port: %d", *daemonConfig.MetricsPort) | ||
| logging.Debugf("metrics: %s", http.ListenAndServe(fmt.Sprintf(":%d", *daemonConfig.MetricsPort), nil)) | ||
| }, 0) | ||
| go func() { | ||
| <-ctx.Done() | ||
| metricsSrv.Shutdown(context.Background()) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
http.Handle inside a retried loop will panic if the metrics listener ever restarts.
http.Handle registers /metrics on the global http.DefaultServeMux, which panics on a duplicate pattern registration. utilwait.UntilWithContext(ctx, f, 0) re-invokes f (and thus re-calls http.Handle) every time http.ListenAndServe returns an error (e.g. port already in use, transient bind failure). The second invocation will panic on "http: multiple registrations for /metrics" instead of retrying gracefully. This also drops the previous graceful-shutdown wiring (ctx.Done() → Shutdown()) for the metrics server.
🐛 Proposed fix: register once, use a dedicated mux, and support shutdown
if daemonConfig.MetricsPort != nil {
- go utilwait.UntilWithContext(ctx, func(_ context.Context) {
- http.Handle("/metrics", promhttp.Handler())
- logging.Debugf("metrics port: %d", *daemonConfig.MetricsPort)
- logging.Debugf("metrics: %s", http.ListenAndServe(fmt.Sprintf(":%d", *daemonConfig.MetricsPort), nil))
- }, 0)
+ metricsMux := http.NewServeMux()
+ metricsMux.Handle("/metrics", promhttp.Handler())
+ metricsSrv := &http.Server{Addr: fmt.Sprintf(":%d", *daemonConfig.MetricsPort), Handler: metricsMux}
+ go func() {
+ logging.Debugf("metrics port: %d", *daemonConfig.MetricsPort)
+ if err := metricsSrv.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+ logging.Errorf("metrics server error: %v", err)
+ }
+ }()
+ go func() {
+ <-ctx.Done()
+ _ = metricsSrv.Shutdown(context.Background())
+ }()
}📝 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.
| if daemonConfig.MetricsPort != nil { | |
| mux := http.NewServeMux() | |
| mux.Handle("/metrics", promhttp.Handler()) | |
| if daemonConfig.EnablePprof != nil && *daemonConfig.EnablePprof { | |
| mux.HandleFunc("/debug/pprof/", pprof.Index) | |
| mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) | |
| mux.HandleFunc("/debug/pprof/profile", pprof.Profile) | |
| mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol) | |
| mux.HandleFunc("/debug/pprof/trace", pprof.Trace) | |
| logging.Verbosef("pprof endpoints enabled on metrics port %d", *daemonConfig.MetricsPort) | |
| } | |
| metricsSrv := &http.Server{ | |
| Addr: fmt.Sprintf(":%d", *daemonConfig.MetricsPort), | |
| Handler: mux, | |
| ReadHeaderTimeout: 10 * time.Second, | |
| } | |
| logging.Debugf("metrics port: %d", *daemonConfig.MetricsPort) | |
| go utilwait.UntilWithContext(ctx, func(_ context.Context) { | |
| if err := metricsSrv.ListenAndServe(); err != nil && err != http.ErrServerClosed { | |
| logging.Debugf("metrics server error: %v", err) | |
| } | |
| http.Handle("/metrics", promhttp.Handler()) | |
| logging.Debugf("metrics port: %d", *daemonConfig.MetricsPort) | |
| logging.Debugf("metrics: %s", http.ListenAndServe(fmt.Sprintf(":%d", *daemonConfig.MetricsPort), nil)) | |
| }, 0) | |
| go func() { | |
| <-ctx.Done() | |
| metricsSrv.Shutdown(context.Background()) | |
| }() | |
| } | |
| if daemonConfig.MetricsPort != nil { | |
| metricsMux := http.NewServeMux() | |
| metricsMux.Handle("/metrics", promhttp.Handler()) | |
| metricsSrv := &http.Server{Addr: fmt.Sprintf(":%d", *daemonConfig.MetricsPort), Handler: metricsMux} | |
| go func() { | |
| logging.Debugf("metrics port: %d", *daemonConfig.MetricsPort) | |
| if err := metricsSrv.ListenAndServe(); err != nil && err != http.ErrServerClosed { | |
| logging.Errorf("metrics server error: %v", err) | |
| } | |
| }() | |
| go func() { | |
| <-ctx.Done() | |
| _ = metricsSrv.Shutdown(context.Background()) | |
| }() | |
| } |
🤖 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 `@cmd/multus-daemon/main.go` around lines 171 - 177, Update the metrics-serving
block around utilwait.UntilWithContext so the /metrics handler is registered
only once on a dedicated ServeMux, avoiding repeated global http.Handle
registration across retries. Preserve retry behavior while wiring the metrics
server to ctx cancellation so shutdown is graceful via the server’s Shutdown
method.
| shortNames: | ||
| - nad | ||
| - net-attach-def | ||
| - net-attach-def |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'deployments/*' | sed -n '1,120p'
echo "== manifests around line 22 =="
for f in deployments/multus-daemonset.yml deployments/multus-daemonset-crio.yml; do
if [ -f "$f" ]; then
echo "-- $f lines 1-60 --"
sed -n '1,60p' "$f" | nl -ba
fi
done
echo "== searches for nad alias and net-attach-def =="
rg -n --hidden --glob '!vendor/**' --glob '!node_modules/**' '(\bnad\b|net-attach-def|multus-daemonset|DaemonSet|ClusterRole|ClusterRoleBinding|rbac)' deployments .github README* 2>/dev/null || trueRepository: openshift/multus-cni
Length of output: 402
🏁 Script executed:
#!/bin/sh
set -euo pipefail
echo "== manifests around line 22 =="
for f in deployments/multus-daemonset.yml deployments/multus-daemonset-crio.yml; do
if [ -f "$f" ]; then
echo "-- $f lines 1-60 --" -
awk 'NR<=60{printf "%4d\t%s\n", NR, $0}' "$f"
fi
done
echo "== searches for nad alias / net-attach-def / multus and RBAC =="
rg -n '(\bnad\b|net-attach-def|multus-daemonset|DaemonSet|ClusterRole|ClusterRoleBinding|rbac|roleRef|subjects)' deployments README.md 2>/dev/null || true
echo "== local git diff stat/name/status =="
git diff --stat 2>/dev/null || true
git diff -- deployments/multus-daemonset.yml deployments/multus-daemonset-crio.yml 2>/dev/null | awk 'NR<=220' || trueRepository: openshift/multus-cni
Length of output: 8482
🌐 Web query:
Multus CNI NetworkAttachmentDefinition shortNames nad net-attach-def
💡 Result:
The NetworkAttachmentDefinition custom resource, used by Multus CNI to define network attachments for pods, includes official Kubernetes short names to simplify command-line operations [1][2][3]. The defined short names are: - net-attach-def: The original and primary short name associated with the NetworkAttachmentDefinition CustomResourceDefinition (CRD) [1][4][5]. - nad: A more recent, shorter alias added to improve the user experience and CLI convenience [6][4]. These aliases can be used interchangeably with the full resource name (network-attachment-definition) when running kubectl commands (e.g., kubectl get nad or kubectl get net-attach-def) [6][4]. Additionally, the abbreviation "NAD" is widely used in documentation and technical discussions as shorthand for NetworkAttachmentDefinition [3][7].
Citations:
- 1: https://k8snetworkplumbingwg.github.io/multus-cni/docs/how-to-use.html
- 2: https://raw.githubusercontent.com/k8snetworkplumbingwg/multus-cni/master/deployments/multus-daemonset-crio.yml
- 3: https://docs.okd.io/latest/networking/multiple_networks/primary_networks/about-primary-nwt-nad.html
- 4: Network Attachment Definition CRD name with dashes k8snetworkplumbingwg/multus-cni#1249
- 5: https://github.com/sunya-ch/kwok/blob/v0.0.1-modified/netattachdef-crd.yaml
- 6: Add "nad" as shortName k8snetworkplumbingwg/multus-cni#1476
- 7: https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html-single/multiple_networks/index
🌐 Web query:
MultiNetCNI k8s.cni.cncf.io NetworkAttachmentDefinition shortNames nad net-attach-def
💡 Result:
The NetworkAttachmentDefinition (NAD) is a Custom Resource Definition (CRD) defined by the Network Plumbing Working Group, typically used with the Multus CNI to enable multi-homed pods in Kubernetes [1][2][3]. The resource is defined under the API group k8s.cni.cncf.io [4][3]. Historically, it has been configured with the following shortNames: 1. net-attach-def: This is the established, primary short name used across most documentation and deployments to refer to NetworkAttachmentDefinition resources via kubectl [1][3][5]. 2. nad: This has been added in more recent versions as an additional short name to improve command-line usability and convenience [6][7]. Because the official CRD name (network-attachment-definitions.k8s.cni.cncf.io) includes dashes, using these short names is the standard way for administrators to interact with the resource using tools like kubectl [4][5]. For example, you can list these resources using either command [8][5]: kubectl get net-attach-def kubectl get nad
Citations:
- 1: https://github.com/k8snetworkplumbingwg/multi-net-spec/blob/master/v1.0/%5Bv1%5D%20Kubernetes%20Network%20Custom%20Resource%20Definition%20De-facto%20Standard.md
- 2: https://raw.githubusercontent.com/k8snetworkplumbingwg/multus-cni/v4.0.1/deployments/multus-daemonset.yml
- 3: https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/deployments/multus-daemonset.yml
- 4: Network Attachment Definition CRD name with dashes k8snetworkplumbingwg/multus-cni#1249
- 5: oc/kubectl get doesn't work for kind 'NetworkAttachmentDefinition' k8snetworkplumbingwg/multus-cni#295
- 6: Add "nad" as shortName k8snetworkplumbingwg/multus-cni#1476
- 7: https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/deployments/multus-daemonset-crio.yml
- 8: Unable to use network-attachment-definitions at Cluster scope k8snetworkplumbingwg/multus-cni#713
Restore the nad short name for public compatibility.
Multus already provides net-attach-def and the newer nad alias; removing nad from both deployment CRDs breaks kubectl get nad and scripts relying on the documented convenience alias.
📍 Affects 2 files
deployments/multus-daemonset.yml#L22-L22(this comment)deployments/multus-daemonset-crio.yml#L22-L22
🤖 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 `@deployments/multus-daemonset.yml` at line 22, Restore the nad short name
alongside net-attach-def in the deployments/multus-daemonset.yml CRD definition
at lines 22-22 and deployments/multus-daemonset-crio.yml at lines 22-22,
preserving kubectl get nad compatibility in both manifests.
| COPY --from=rhel9 \ | ||
| /usr/src/multus-cni/bin/thin_entrypoint \ | ||
| /usr/src/multus-cni/bin/multus \ | ||
| /usr/src/multus-cni/bin |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching Dockerfile.microshift =="
git ls-files | grep -F 'Dockerfile.microshift' || true
echo
echo "== Dockerfile.microshift relevant lines =="
if [ -f Dockerfile.microshift ]; then
nl -ba Dockerfile.microshift | sed -n '1,80p'
fi
echo
echo "== any multi-source COPY instructions in Dockerfile.microshift =="
rg -n "^(FROM|COPY|ARG|USER|WORKDIR|HEALTHCHECK|RUN)" Dockerfile.microshift || true
echo
echo "== compare COPY behavior for two sources with and without trailing slash in bash cp =="
tmp=$(mktemp -d)
mkdir -p "$tmp/src" "$tmp/dest"
touch "$tmp/src/a" "$tmp/src/b"
echo "with leading slash:"
cp "$tmp/src/a" "$tmp/src/b" "$tmp/src/bin" && ls -la "$tmp/src/bin"
rm -rf "$tmp/src/bin"
mkdir -p "$tmp/src/bin"
echo "with trailing slash:"
cp "$tmp/src/a" "$tmp/src/b" "$tmp/src/bin/" && ls -la "$tmp/src/bin"
rm -rf "$tmp"Repository: openshift/multus-cni
Length of output: 313
🌐 Web query:
Docker COPY multiple sources destination must be directory ending with / Dockerfile COPY semantics
💡 Result:
In Docker, when the COPY instruction specifies multiple source files or directories (either directly or through the use of wildcards), the destination path must be a directory, and it must explicitly end with a trailing slash (/) [1][2][3]. This requirement ensures that Docker treats the destination as a directory to house the multiple items being copied [4]. If the destination does not end with a slash in this multi-source scenario, the build will fail [1][4]. Key points regarding COPY semantics with multiple sources: - Mandatory Trailing Slash: If multiple sources are provided, the destination argument must end with / to be interpreted as a directory [1][2]. - Behavior with Wildcards: The same rule applies when wildcards (e.g., *.txt) result in multiple files being matched [1][2]. - Single Source Behavior: If you specify only a single source file, the trailing slash is not strictly required, though it is still common practice to include it if you intend the destination to be a directory [2]. If the source is a file and the destination does not end in a slash, Docker will copy the file to the destination path as a regular file (renaming it if the path name differs) [2]. - Missing Directories: If the specified destination directory does not exist, Docker will create it along with any necessary parent directories [2][3].
Citations:
- 1: https://docs.docker.com/reference/dockerfile/
- 2: https://github.com/docker/docker-ce/blob/master/components/cli/docs/reference/builder.md
- 3: https://web.archive.org/web/20200218205347/https:/docs.docker.com/engine/reference/builder/
- 4: https://stackoverflow.com/questions/30256386/how-to-copy-multiple-files-in-one-layer-using-a-dockerfile
Make the multi-source COPY destination explicit.
This instruction copies two source files, so Docker expects the destination to be an existing directory. Add the trailing slash to avoid reliance on any build-time existing directory at /usr/src/multus-cni/bin.
Proposed fix
COPY --from=rhel9 \
/usr/src/multus-cni/bin/thin_entrypoint \
/usr/src/multus-cni/bin/multus \
- /usr/src/multus-cni/bin
+ /usr/src/multus-cni/bin/📝 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.
| COPY --from=rhel9 \ | |
| /usr/src/multus-cni/bin/thin_entrypoint \ | |
| /usr/src/multus-cni/bin/multus \ | |
| /usr/src/multus-cni/bin | |
| COPY --from=rhel9 \ | |
| /usr/src/multus-cni/bin/thin_entrypoint \ | |
| /usr/src/multus-cni/bin/multus \ | |
| /usr/src/multus-cni/bin/ |
🧰 Tools
🪛 Hadolint (2.14.0)
[error] 14-14: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
🤖 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 `@Dockerfile.microshift` around lines 14 - 17, Update the multi-source COPY
instruction in Dockerfile.microshift to use an explicit trailing slash on the
destination directory `/usr/src/multus-cni/bin/`, preserving both source files
unchanged.
Source: Linters/SAST tools
| #### Create network attachment definition with resource name | ||
|
|
||
| The `k8s.v1.cni.cncf.io/resourceName` should match the `ResourceClass` name defined in the section above. | ||
| In this example it is `sf-pool-1`. Multus query the K8s PodResource API to fetch the `resourceClass` name and also | ||
| query the NetworkAttachmentDefinition `k8s.v1.cni.cncf.io/resourceName`. If both has the same name multus send the | ||
| CDI device name in the DeviceID argument. | ||
|
|
||
| Following command creates a `DeviceClass` for the `ResourceClaimTemplate` to request devices from. | ||
| ##### NetworkAttachmentDefinition for ovn-kubernetes example: | ||
|
|
||
| Following command creates NetworkAttachmentDefinition. CNI config is in `config:` field. | ||
|
|
||
| ``` | ||
| # Execute following command at Kubernetes master | ||
| cat <<EOF | kubectl create -f - | ||
| apiVersion: resource.k8s.io/v1 | ||
| kind: DeviceClass | ||
| apiVersion: "k8s.cni.cncf.io/v1" | ||
| kind: NetworkAttachmentDefinition | ||
| metadata: | ||
| name: sriovnetwork.k8snetworkplumbingwg.io | ||
| name: default | ||
| annotations: | ||
| k8s.v1.cni.cncf.io/resourceName: sf-pool-1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the ResourceClaim name in resourceName, not the ResourceClass name.
The kubelet resource map is keyed by DynamicResource.ClaimName, and Multus looks up that map using the NAD annotation. With this example, the map key is sf, but the annotation is sf-pool-1, so no device ID is found and the DRA network attachment will not receive the allocation.
Update the explanation and annotation to use sf (or change the runtime contract and tests to key by ResourceClass name).
Proposed documentation fix
-The `k8s.v1.cni.cncf.io/resourceName` should match the `ResourceClass` name defined in the section above.
-In this example it is `sf-pool-1`. Multus query the K8s PodResource API to fetch the `resourceClass` name and also
+The `k8s.v1.cni.cncf.io/resourceName` should match the `ResourceClaim` name used by the Pod.
+In this example it is `sf`. Multus queries the K8s PodResource API for the claim name and uses the matching
...
- k8s.v1.cni.cncf.io/resourceName: sf-pool-1
+ k8s.v1.cni.cncf.io/resourceName: sf📝 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.
| #### Create network attachment definition with resource name | |
| The `k8s.v1.cni.cncf.io/resourceName` should match the `ResourceClass` name defined in the section above. | |
| In this example it is `sf-pool-1`. Multus query the K8s PodResource API to fetch the `resourceClass` name and also | |
| query the NetworkAttachmentDefinition `k8s.v1.cni.cncf.io/resourceName`. If both has the same name multus send the | |
| CDI device name in the DeviceID argument. | |
| Following command creates a `DeviceClass` for the `ResourceClaimTemplate` to request devices from. | |
| ##### NetworkAttachmentDefinition for ovn-kubernetes example: | |
| Following command creates NetworkAttachmentDefinition. CNI config is in `config:` field. | |
| ``` | |
| # Execute following command at Kubernetes master | |
| cat <<EOF | kubectl create -f - | |
| apiVersion: resource.k8s.io/v1 | |
| kind: DeviceClass | |
| apiVersion: "k8s.cni.cncf.io/v1" | |
| kind: NetworkAttachmentDefinition | |
| metadata: | |
| name: sriovnetwork.k8snetworkplumbingwg.io | |
| name: default | |
| annotations: | |
| k8s.v1.cni.cncf.io/resourceName: sf-pool-1 | |
| #### Create network attachment definition with resource name | |
| The `k8s.v1.cni.cncf.io/resourceName` should match the `ResourceClaim` name used by the Pod. | |
| In this example it is `sf`. Multus queries the K8s PodResource API for the claim name and uses the matching | |
| resourceName to fetch the device name. If both have the same name, Multus sends the | |
| CDI device name in the DeviceID argument. | |
| ##### NetworkAttachmentDefinition for ovn-kubernetes example: | |
| Following command creates NetworkAttachmentDefinition. CNI config is in `config:` field. | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 699-699: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 699-699: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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 `@docs/how-to-use.md` around lines 688 - 707, Update the “Create network
attachment definition with resource name” documentation to state that
k8s.v1.cni.cncf.io/resourceName must use the ResourceClaim name, not the
ResourceClass name, and change the example annotation from sf-pool-1 to sf. Keep
the explanation consistent with the kubelet resource map lookup keyed by
DynamicResource.ClaimName.
| #### Launch pod with DRA Resource Claim | ||
|
|
||
| Following command launches a Pod with the secondary network `sriov-net` and a DRA resource claim named `sriov`. | ||
| The NAD `resourceName` must match the driver’s `k8s.cni.cncf.io/resourceName` on the allocated device. | ||
| Following command Launch a Pod with primiry network `default` and `ResourceClaim` `sf`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the Pod example wording.
Use “The following command launches a Pod with a primary network…”; primiry is misspelled and “Following command Launch” is grammatically incorrect.
🧰 Tools
🪛 LanguageTool
[grammar] ~744-~744: Ensure spelling is correct
Context: ...im Following command Launch a Pod with primiry network default and ResourceClaim `...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/how-to-use.md` around lines 742 - 744, Update the “Launch pod with DRA
Resource Claim” example description to use grammatically correct wording: “The
following command launches a Pod with a primary network …”, correcting “primiry”
to “primary” and preserving the existing network and ResourceClaim names.
Source: Linters/SAST tools
| func conflistAdd(rt *libcni.RuntimeConf, rawnetconflist []byte, cniConfList *libcni.NetworkConfigList, multusNetconf *types.NetConf, exec invoke.Exec) (cnitypes.Result, error) { | ||
| logging.Debugf("conflistAdd: %v, %s", rt, string(rawnetconflist)) | ||
| // In part, adapted from K8s pkg/kubelet/dockershim/network/cni/cni.go | ||
| binDirs := filepath.SplitList(os.Getenv("CNI_PATH")) | ||
| binDirs = append([]string{multusNetconf.BinDir}, binDirs...) | ||
| cniNet := libcni.NewCNIConfigWithCacheDir(binDirs, multusNetconf.CNIDir, exec) | ||
|
|
||
| confList, err := libcni.NetworkConfFromBytes(rawnetconflist) | ||
| if err != nil { | ||
| return nil, logging.Errorf("conflistAdd: error converting the raw bytes into a conflist: %v", err) | ||
| var confList *libcni.NetworkConfigList | ||
| var err error | ||
|
|
||
| // This may wind up being set during parsing the default network config. | ||
| // In this case -- we'll use it as passed. Otherwise, we'll recalculate it. | ||
| if len(cniConfList.Plugins) > 0 { | ||
| confList = cniConfList | ||
| } else { | ||
| confList, err = libcni.NetworkConfFromBytes(rawnetconflist) | ||
| if err != nil { | ||
| return nil, logging.Errorf("conflistAdd: error converting the raw bytes into a conflist: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -B2 -A2 '\bconflistAdd\(' pkg/multusRepository: openshift/multus-cni
Length of output: 986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant sections and inspect the concrete call sites/config parsing paths.
sed -n '400,450p' pkg/multus/multus.go
printf '\n--- deleg field/type references ---\n'
rg -n '\bCNINetworkConfigList\|ConfListPlugin\|LoadDelegateNetConfFromConfList\|InjectCNIVersionIn' pkg/multus pkg/types -g '*.go'
printf '\n--- candidate parsing files outlines ---\n'
fd -a 'conf|multus' pkg -d 2 | sed 's#^\./##' | head -50Repository: openshift/multus-cni
Length of output: 1883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact delegate/conf config references ---'
rg -n 'CNINetworkConfigList|ConfListPlugin|LoadDelegateNetConfFromConfList|InjectCNIVersionIn' pkg --glob '*.go' --glob '*.py' --glob '*.yaml' --glob '*.yml' || true
printf '%s\n' '--- multus.go conflistAdd implementation ---'
sed -n '290,345p' pkg/multus/multus.go
printf '%s\n' '--- types/conf.go relevant parser structs/types ---'
fd -a -i 'conf.go$' pkg | while read -r f; do
echo "### $f"
wc -l "$f"
rg -n 'type .*CNINetworkConfigList|type .*Delegate|LoadDelegateNetConfFromConfList|InjectCNIVersionIn' "$f" || true
doneRepository: openshift/multus-cni
Length of output: 5425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pkg/types/conf.go Loader ---'
sed -n '86,130p' pkg/types/conf.go
printf '%s\n' '--- LoadDelegateNetConfFromConfList injection helpers nearby ---'
sed -n '600,720p' pkg/types/conf.go
printf '%s\n' '--- delegated injection call paths ---'
sed -n '590,635p' pkg/k8sclient/k8sclient.go
sed -n '608,630p' pkg/k8sclient/k8sclient.goRepository: openshift/multus-cni
Length of output: 7729
Use delegate.Bytes for conflists when it has plugins already.
DelegateAdd intentionally passes delegate.Bytes into conflistAdd, and current conflist delegates parsed via LoadDelegateNetConfFromConfList always populate delegate.CNINetworkConfigList before parsing delegate.Bytes. conflistAdd therefore takes the stale CNINetworkConfigList and executes a config that never received deviceID, cni-args, or CNIVersion changes.
🤖 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 `@pkg/multus/multus.go` around lines 299 - 318, The conflistAdd function
currently prefers the stale cniConfList whenever it already has plugins,
ignoring the updated rawnetconflist bytes passed by DelegateAdd. Change this
selection path to parse and use rawnetconflist via libcni.NetworkConfFromBytes
for conflist delegates, while preserving the existing error handling and only
reusing cniConfList when the delegate bytes are not the source of truth.
| func overrideCNIConfigWithServerConfig(cniConf []byte, overrideConf []byte, ignoreReadinessIndicator bool) ([]byte, error) { | ||
| // If there is no server-side override config AND we don't need to strip any keys, | ||
| // return the client config unchanged. | ||
| if len(overrideConf) == 0 && !ignoreReadinessIndicator { | ||
| return cniConf, nil | ||
| } | ||
| // Treat a missing server config as an empty object so the key-stripping logic below | ||
| // still runs when ignoreReadinessIndicator is true. | ||
| if len(overrideConf) == 0 { | ||
| overrideConf = []byte("{}") | ||
| return cniConf, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how overrideConf is constructed/populated for the daemon to see if it can legitimately be empty.
rg -nP -B3 -A15 'func NewCNIServer' pkg/server/server.goRepository: openshift/multus-cni
Length of output: 1069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server.go outline around relevant symbols =="
ast-grep outline pkg/server/server.go --view expanded | rg -n 'NewCNIServer|Server|overrideCNIConfigWithServerConfig|ServerConfig' || true
echo
echo "== server.go imports/header =="
sed -n '1,90p' pkg/server/server.go
echo
echo "== NewCNIServer body =="
sed -n '220,320p' pkg/server/server.go
echo
echo "== overrideCNIConfigWithServerConfig tests/usages =="
rg -n 'overrideCNIConfigWithServerConfig|ignoreReadinessIndicator|readinessindicatorfile|ServerConfig' pkg/server/server.go pkg -g '*.go'
echo
echo "== server config fields and defaults =="
rg -n 'type .*Server|ServerConfig|ignoreReadinessIndicator|readinessindicatorfile' pkg -g '*.go'Repository: openshift/multus-cni
Length of output: 22303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== overrideCNIConfigWithServerConfig implementation =="
sed -n '488,530p' pkg/server/server.go
echo
echo "== call sites =="
sed -n '528,580p' pkg/server/server.go
echo
echo "== server tests around overrideCNIConfigWithServerConfig =="
sed -n '40,90p' pkg/server/server_test.go
echo
echo "== server config/config generation relevant fields =="
sed -n '1,120p' pkg/server/config/generator.go
echo
echo "== local callers and tests pass serverConfig as empty? =="
rg -n 'NewCNIServer\(|startCNIServer\(|\[\]byte\(.*ServerConfig|Serve\|SetListenAddress|ignoreReadinessIndicator' pkg -g '*.go'Repository: openshift/multus-cni
Length of output: 11174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find request handling/api config extraction =="
rg -n 'api\.Request|UnmarshalJSON|extractCniData|extract.*cni|StdinData|request.*Config|ReadinessIndicatorFile' pkg/server pkg/server/api -g '*.go'
echo
echo "== api package files =="
git ls-files pkg/server/api pkg/server/config pkg/server/types.go pkg/server/server.go
echo
echo "== api request parsing implementations =="
for f in $(git ls-files 'pkg/server/api/*.go'); do
echo "--- $f"
wc -l "$f"
rg -n 'Request|Env|Config|UnmarshalJSON|MarshalJSON|Get|post|CNI_COMMAND' "$f"
doneRepository: openshift/multus-cni
Length of output: 3966
Keep readinessindicatorfile stripping unconditional.
overrideCNIConfigWithServerConfig now returns the original CNI config unchanged for empty serverConfig, so ignoreReadinessIndicator: true no longer strips readinessindicatorfile unless the deployment also supplies a JSON override. Normalize []byte{} to an empty JSON object before applying the ignore list, or handle the empty override path separately.
🤖 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 `@pkg/server/server.go` around lines 498 - 501, The empty override path in
overrideCNIConfigWithServerConfig bypasses readinessindicatorfile stripping.
Ensure ignoreReadinessIndicator is applied even when overrideConf is empty by
normalizing the empty override to an empty JSON object or handling that path
separately, while preserving the existing return behavior when stripping is not
requested.
|
/payload-job periodic-ci-openshift-eng-ocp-perfscale-main-aws-5.0-nightly-x86-payload-control-plane-6nodes |
|
@mcornea: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/51124110-8b63-11f1-825a-5e4af65b6c65-0 |
|
@mcornea Let me know if this is the root cause and we can change it upstream as well. |
|
@wizhaoredhat — Based on the bisection results, multus-cni#335 is a secondary contributor to the Bisection summary:
The OVN-K team (Marty Power) is looking at OCPBUGS-100275 to identify the specific change within the OVN-K downstream merge. For the multus side, the additional latency likely comes from the |
|
@redhat-chai-bot What is the pods per node tested with kube burner node density? |
|
@wizhaoredhat The workload used here is This workload uses a dynamic iteration model rather than a fixed pods-per-node parameter:
The calculation logic from the step registry: iteration_multiplier=${UDN_ITERATION_MULTIPLIER:-${ITERATION_MULTIPLIER_ENV:-3}}
ITERATIONS=$(awk "BEGIN {printf \"%d\", $iteration_multiplier * $current_worker_count}")So the effective density is ~12 iterations per worker node, with pods distributed across namespaces. The default multiplier is 3 if not overridden. |
|
@redhat-chai-bot What would be the equivalent running with 2 worker node kind cluster with node-density? i.e. what is the recommended number of pods per node. |
|
@wizhaoredhat The original PERFSCALE-5389 regression was tested with For a 2-worker kind cluster: If reproducing the UDN regression specifically (requires OVN-Kubernetes on kind):
If doing general density testing with
The key distinction: AI-generated. Review for accuracy. |
3 participants
Summary
This reverts commit f099946 (merge commit for PR #335), the upstream merge from 07/24/2026.
Why
Reverting to bisect a
podReadyLatency_P99regression (+64%) detected in theudn-density-podsworkload on OCP 5.0 nightlies starting around5.0.0-0.nightly-2026-07-15. The upstream merge is the top suspect due to changes in the CNI conflist delegation hot path, a newconnectionLimitfeature, and behavioral changes in server config overrides.See PERFSCALE-5389 for full triage details.
Validation Plan
Run
periodic-ci-openshift-eng-ocp-perfscale-main-aws-5.0-nightly-x86-payload-control-plane-6nodesagainst this revert to confirm whetherpodReadyLatency_P99recovers to baseline (~31,000-42,000 ms).Key Changes Reverted
rawConfListBytes())connectionLimitconfig option for the multus daemon Unix socketoverrideCNIConfigWithServerConfigbehavioral change (readiness indicator stripping)@mcornea requested in Slack thread
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Maintenance