CORENET-7129 Add OVN metrics tests as OTP tests#3217
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBootstraps OTP tests and exutil cleanup, refreshes Go module deps, replaces go-bindata with embed.FS, adds many networking fixtures/templates, and introduces OVN metrics and NetworkPolicy OTP test suites. ChangesOTP Test Integration and Fixtures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asood-rh 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 |
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
openshift/test/otp/metrics.go (1)
42-50: 💤 Low valueRedundant retry wrapper around helper with internal assertions.
The
GetOVNMetricsInSpecificContainerhelper already performs its ownwait.Pollinternally (per context snippet 3) and includeso.Expect(metricValue).ShouldNot(o.BeEmpty()). If the inner function fails, the test panics immediately—the outer poll won't retry. Theif metricValue != ""check at line 44 is also unreachable when the helper returns successfully (since it guarantees non-empty).Consider either:
- Simplifying to a single retry loop by removing the outer poll and relying on the helper's behavior, or
- Creating a variant helper that returns an error instead of asserting, so the outer poll can actually retry.
The current code works (both succeed or inner fails first), but the pattern is misleading.
🤖 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 `@openshift/test/otp/metrics.go` around lines 42 - 50, The outer wait.Poll is redundant because otputils.GetOVNMetricsInSpecificContainer already does its own wait.Poll and internally asserts (o.Expect/ShouldNot) so the outer retry never meaningfully runs; either remove the outer wait.Poll and directly call otputils.GetOVNMetricsInSpecificContainer(oc, ovncmName, podName, prometheusURL, metricName) (and drop the o.Expect(metricsOutput)... line), or implement a non-asserting variant (e.g., GetOVNMetricsInSpecificContainerNoAssert that returns (string, error)) and replace the helper call inside the outer wait.Poll with that so the poll can examine the error/value and retry properly. Ensure you update call sites to use the chosen approach (remove wait.Poll wrapper or swap to the new non-asserting helper).openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yaml (1)
3-4: ⚡ Quick winTemplate
metadata.namenaming is not used by this test flowmetadata: name: ipBlockEgressDual_multiple_cidrs_templateThis
metadata.namecontains uppercase letters and underscores, which violates Kubernetes DNS-subdomain naming rules. However, this template is only processed viaoc process(ApplyResourceFromTemplateByAdmin) and the processed objects are what get applied; other template fixtures in the same testdata also use uppercase/underscoremetadata.namevalues, so this doesn’t block the intended usage.Suggested fix
metadata: - name: ipBlockEgressDual_multiple_cidrs_template + name: ipblock-egress-dual-multiple-cidrs-template🤖 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 `@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yaml` around lines 3 - 4, The template's metadata.name "ipBlockEgressDual_multiple_cidrs_template" violates Kubernetes DNS-subdomain rules (uppercase and underscores); update the template name used in this file by replacing the value of metadata.name with a DNS-compliant string (lowercase letters, digits and hyphens only), e.g. "ipblock-egress-dual-multiple-cidrs-template", and ensure any test code that references this template via oc process / ApplyResourceFromTemplateByAdmin uses the new name; locate the metadata.name entry for the template identifier ipBlockEgressDual_multiple_cidrs_template and rename it accordingly.openshift/pkg/otp/testdata/networking/testpod.yaml (1)
22-41: ⚡ Quick winApply the same pod hardening here to keep the fixture admission-safe.
The RC pod template currently relies on default security settings, unlike the other new pod fixtures in this PR. Adding
runAsNonRoot,seccompProfile: RuntimeDefault, and container-levelallowPrivilegeEscalation: false/ dropped capabilities will make this fixture much less likely to trip restricted admission.Proposed fix
"spec": { + "securityContext": { + "runAsNonRoot": true, + "seccompProfile": { + "type": "RuntimeDefault" + } + }, "containers": [ { "image": "quay.io/openshifttest/hello-sdn@sha256:c89445416459e7adea9a5a416b3365ed3d74f2491beb904d61dc8d1eb89a72a4", "name": "test-pod", - "imagePullPolicy": "IfNotPresent" + "imagePullPolicy": "IfNotPresent", + "securityContext": { + "allowPrivilegeEscalation": false, + "capabilities": { + "drop": ["ALL"] + } + } } ],🤖 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 `@openshift/pkg/otp/testdata/networking/testpod.yaml` around lines 22 - 41, The pod template under "spec" for the container named "test-pod" needs the same hardening as other fixtures: add a pod-level securityContext with runAsNonRoot: true and seccompProfile.type: RuntimeDefault, and add a container-level securityContext on the "test-pod" container setting allowPrivilegeEscalation: false and dropping capabilities (e.g., drop: ["ALL"]); update the "containers" entry for the "test-pod" to include this securityContext so the RC pod template is admission-safe.openshift/pkg/otp/testdata/networking/statefulset-hello.yaml (1)
18-29: ⚡ Quick winHarden this pod template the same way as the other new fixtures.
This StatefulSet is the outlier in this batch: it omits
runAsNonRoot,seccompProfile: RuntimeDefault, and container-levelallowPrivilegeEscalation: false/ dropped capabilities. That can make the fixture fail or get mutated under restricted admission, even though the equivalent pod templates in this PR are already hardened.Proposed fix
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - image: quay.io/openshifttest/hello-sdn@sha256:c89445416459e7adea9a5a416b3365ed3d74f2491beb904d61dc8d1eb89a72a4 name: hello + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: ["ALL"] ports: - containerPort: 8080 name: web🤖 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 `@openshift/pkg/otp/testdata/networking/statefulset-hello.yaml` around lines 18 - 29, The StatefulSet pod template is missing the same hardening as other fixtures; update the spec.template.spec and the container named "hello" to include securityContext.runAsNonRoot: true on the pod spec, pod-level seccompProfile: { type: RuntimeDefault } under securityContext, and for the container "hello" add securityContext.allowPrivilegeEscalation: false and securityContext.capabilities: { drop: ["ALL"] } so the pod matches the other hardened fixtures and won't be mutated or rejected by restricted admission.
🤖 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 `@openshift/cmd/ovn-kubernetes-tests-ext/main.go`:
- Around line 42-62: The current blocker classifier uses title substrings
(otpBlockingTests + isOTPBlocking) which is brittle; change otpBlockingTests to
contain the stable 5-digit OTP case IDs (or the exact metadata token used in
specs) instead of full titles and update isOTPBlocking to search for those IDs
(e.g., strings.Contains(name, otpID)) or parse test metadata for an explicit
otp_id field; ensure you update any test registration or comparison logic that
builds/consumes otpBlockingTests and keep the variable names otpBlockingTests
and isOTPBlocking so the change is easy to locate.
In `@openshift/pkg/otp/testdata/bindata.go`:
- Around line 1745-1749: The extracted fixture files are being written with mode
0 because every bindataFileInfo uses os.FileMode(0); update the write so it
falls back to sane permissions when info.Mode() == 0: in the restore path where
ioutil.WriteFile(_filePath(dir, name), data, info.Mode()) is called (the code
that uses bindataFileInfo), compute a writeMode := info.Mode(); if writeMode ==
0 { writeMode = 0644 } and use writeMode for WriteFile (keep the existing
MkdirAll with 0755 for directories).
In `@openshift/pkg/otp/testdata/networking/hello-pod-daemonset.yaml`:
- Around line 14-15: Replace the deprecated node label key in the DaemonSet YAML
by changing the nodeSelector entry from "beta.kubernetes.io/os" to the stable
"kubernetes.io/os"; update the same key in all other occurrences mentioned
(e.g., entries in openshift/pkg/otp/testdata/bindata.go and
helm/ovn-kubernetes/charts/ovn-ipsec/templates/daemonset.yaml) so
fixtures/templates remain consistent and DaemonSets schedule correctly.
In `@openshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yaml`:
- Around line 3-4: The manifest's metadata.name "allow-all-ingress" is
misleading and may collide with real allow-all fixtures; update the
metadata.name in ingress_49696.yaml to a unique, intent-matching value (for
example include the selector/intent like "allow-operations-test-pods-ingress" or
"allow-team-operations-ingress") so it accurately reflects that it only allows
traffic from team=operations/name=test-pods; ensure any tests or references that
expect the old name are updated to the new name.
In
`@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yaml`:
- Around line 3-4: The manifest's metadata.name
"ipBlock_egress_dual_cidrs_template" is not DNS-1123 compliant (contains
uppercase/underscores); update the Template's metadata.name to a lowercase,
hyphen-separated, unique value (e.g., convert underscores to '-' and lowercase)
ensuring it does not collide with other fixtures; locate the name in the
template where metadata.name is set and replace it (refer to the symbol/value
ipBlock_egress_dual_cidrs_template and the surrounding Template YAML) so it
follows Kubernetes DNS-1123 label rules.
In
`@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yaml`:
- Around line 3-4: The Template metadata.name
"ipBlockEgressSingle_cidr_template" is not DNS-compliant; update the value used
in the file (metadata.name) to a lowercase, RFC1123-compatible label using only
lowercase alphanumerics, '-' or '.', for example
"ipblock-egress-single-cidr-template", and ensure any references to that name in
templates/tests are updated to match.
In `@openshift/pkg/otp/testdata/networking/service-generic-template.yaml`:
- Around line 17-34: The Service template currently hardcodes the port value
(port: 27017) while declaring a template parameter PORT; update the ports
mapping so the port field uses the PORT parameter (e.g., replace the literal
27017 with the template variable for PORT) or if you intend not to vary the
port, remove the PORT parameter from the parameters list; target the ports ->
port entry and the parameters list (PORT) in service-generic-template.yaml to
make the template behavior consistent.
In `@openshift/test/otp/networkpolicy.go`:
- Around line 1682-1707: The test contains three identical defer cleanup blocks
calling oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy",
policyName, "-n", ns).Execute(), which will attempt to delete the same
NetworkPolicy three times; remove the duplicate defers that wrap the
createAclCmd and setAclPropertiesCmd sections and keep only the initial defer
(the one before createAclCmd) so cleanup runs exactly once.
---
Nitpick comments:
In
`@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yaml`:
- Around line 3-4: The template's metadata.name
"ipBlockEgressDual_multiple_cidrs_template" violates Kubernetes DNS-subdomain
rules (uppercase and underscores); update the template name used in this file by
replacing the value of metadata.name with a DNS-compliant string (lowercase
letters, digits and hyphens only), e.g.
"ipblock-egress-dual-multiple-cidrs-template", and ensure any test code that
references this template via oc process / ApplyResourceFromTemplateByAdmin uses
the new name; locate the metadata.name entry for the template identifier
ipBlockEgressDual_multiple_cidrs_template and rename it accordingly.
In `@openshift/pkg/otp/testdata/networking/statefulset-hello.yaml`:
- Around line 18-29: The StatefulSet pod template is missing the same hardening
as other fixtures; update the spec.template.spec and the container named "hello"
to include securityContext.runAsNonRoot: true on the pod spec, pod-level
seccompProfile: { type: RuntimeDefault } under securityContext, and for the
container "hello" add securityContext.allowPrivilegeEscalation: false and
securityContext.capabilities: { drop: ["ALL"] } so the pod matches the other
hardened fixtures and won't be mutated or rejected by restricted admission.
In `@openshift/pkg/otp/testdata/networking/testpod.yaml`:
- Around line 22-41: The pod template under "spec" for the container named
"test-pod" needs the same hardening as other fixtures: add a pod-level
securityContext with runAsNonRoot: true and seccompProfile.type: RuntimeDefault,
and add a container-level securityContext on the "test-pod" container setting
allowPrivilegeEscalation: false and dropping capabilities (e.g., drop: ["ALL"]);
update the "containers" entry for the "test-pod" to include this securityContext
so the RC pod template is admission-safe.
In `@openshift/test/otp/metrics.go`:
- Around line 42-50: The outer wait.Poll is redundant because
otputils.GetOVNMetricsInSpecificContainer already does its own wait.Poll and
internally asserts (o.Expect/ShouldNot) so the outer retry never meaningfully
runs; either remove the outer wait.Poll and directly call
otputils.GetOVNMetricsInSpecificContainer(oc, ovncmName, podName, prometheusURL,
metricName) (and drop the o.Expect(metricsOutput)... line), or implement a
non-asserting variant (e.g., GetOVNMetricsInSpecificContainerNoAssert that
returns (string, error)) and replace the helper call inside the outer wait.Poll
with that so the poll can examine the error/value and retry properly. Ensure you
update call sites to use the chosen approach (remove wait.Poll wrapper or swap
to the new non-asserting helper).
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 2cece5a8-b715-43d9-ae31-2287ae6d318d
⛔ Files ignored due to path filters (1)
openshift/go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
openshift/cmd/ovn-kubernetes-tests-ext/main.goopenshift/go.modopenshift/pkg/otp/testdata/bindata.goopenshift/pkg/otp/testdata/networking/OWNERSopenshift/pkg/otp/testdata/networking/hello-pod-daemonset.yamlopenshift/pkg/otp/testdata/networking/hellosdn.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/service-generic-template.yamlopenshift/pkg/otp/testdata/networking/statefulset-hello.yamlopenshift/pkg/otp/testdata/networking/testpod.yamlopenshift/pkg/otp/utils.goopenshift/test/otp/metrics.goopenshift/test/otp/networkpolicy.go
| var otpBlockingTests = []string{ | ||
| "service domain can be resolved when egress type is enabled", | ||
| "Networkpolicy egress rule should work for statefulset pods", | ||
| "Check ACL audit logs can be extracted", | ||
| "Check networkpolicy ACL audit message is logged with correct policy name", | ||
| "OVN address_set referenced in acl should not miss when networkpolicy name includes dot", | ||
| "Duplicate transactions should not be executed for network policy for every pod update", | ||
| "Creating egress network policies for allowing to same namespace and openshift dns in namespace prevents the pod from reaching its own service", | ||
| "Pod IP is missing from OVN DB AddressSet when using allow-namespace-only network policy", | ||
| "Add OVN flow count metric", | ||
| "Record update to cache versus port binding", | ||
| "Add mechanism to record duration for k8 kinds", | ||
| } | ||
|
|
||
| func isOTPBlocking(name string) bool { | ||
| for _, title := range otpBlockingTests { | ||
| if strings.Contains(name, title) { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use stable OTP IDs for blocker classification.
This classifier keys off title substrings, so a harmless test rename can silently flip a blocking OTP case to informing. Since these specs already carry stable 5-digit OTP case IDs, classify on those IDs (or explicit registration metadata) instead of free-form titles.
🤖 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 `@openshift/cmd/ovn-kubernetes-tests-ext/main.go` around lines 42 - 62, The
current blocker classifier uses title substrings (otpBlockingTests +
isOTPBlocking) which is brittle; change otpBlockingTests to contain the stable
5-digit OTP case IDs (or the exact metadata token used in specs) instead of full
titles and update isOTPBlocking to search for those IDs (e.g.,
strings.Contains(name, otpID)) or parse test metadata for an explicit otp_id
field; ensure you update any test registration or comparison logic that
builds/consumes otpBlockingTests and keep the variable names otpBlockingTests
and isOTPBlocking so the change is easy to locate.
| nodeSelector: | ||
| beta.kubernetes.io/os: linux |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openshift/pkg/otp/testdata/networking/hello-pod-daemonset.yaml"
echo "== File: $FILE =="
if [ -f "$FILE" ]; then
nl -ba "$FILE" | sed -n '1,60p'
else
echo "Missing: $FILE"
fi
echo
echo "== Occurrences of beta.kubernetes.io/os =="
rg -n "beta\.kubernetes\.io/os" -S . || trueRepository: openshift/ovn-kubernetes
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openshift/pkg/otp/testdata/networking/hello-pod-daemonset.yaml"
echo "== Check file exists =="
if [ -f "$FILE" ]; then echo "FOUND: $FILE"; else echo "MISSING: $FILE"; exit 1; fi
echo
echo "== File: $FILE (lines 1-60) =="
# Use awk to add line numbers (since nl isn't available)
awk 'NR>=1 && NR<=60 {printf "%4d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Occurrences of beta.kubernetes.io/os in repo =="
rg -n "beta\.kubernetes\.io/os" -S . || trueRepository: openshift/ovn-kubernetes
Length of output: 1440
🌐 Web query:
"beta.kubernetes.io/os" deprecated "kubernetes.io/os" standard node label documentation
💡 Result:
The node label beta.kubernetes.io/os has been officially deprecated in favor of the standard label kubernetes.io/os [1][2]. Key points regarding this migration: - Deprecation Status: The beta.kubernetes.io/os label was deprecated starting in Kubernetes v1.14 [3][4][5]. - Removal: As of May 2026, the Kubernetes project has moved to remove these beta labels entirely [6]. Recent updates have stopped the kubelet from applying beta.kubernetes.io/os and beta.kubernetes.io/arch to nodes and removed their reconciliation from the node lifecycle controller [6]. - Migration Path: Users should update any node selectors, pod affinity rules, or Persistent Volume (PV) node affinity configurations to use the GA (General Availability) equivalents: kubernetes.io/os and kubernetes.io/arch [6]. - Impact: While existing nodes with the old labels may not immediately break, any new nodes or systems relying on these labels will fail to function as expected if they use the deprecated beta keys [6]. The system has been issuing warnings (Warning: 299) via kubectl during application/deployment to alert users to this required migration since v1.25 [6].
Citations:
- 1: https://kubernetes.io/docs/reference/labels-annotations-taints/
- 2: https://www.bookstack.cn/read/kubernetes-1.20-en/a36187689c2f6c95.md
- 3: [Trival] beta.kubernetes.io/os deprecated since v1.14, are targeted for removal in v1.18 kubernetes/kubernetes#89460
- 4: https://stackoverflow.com/questions/61755657/understanding-built-in-labels
- 5: Warning - beta.kubernetes.io/os is deprecated since v1.14; use "kubernetes.io/os" instead coredns/helm#83
- 6: Drop beta os arch node labels kubernetes/kubernetes#138861
Replace deprecated beta.kubernetes.io/os with kubernetes.io/os in nodeSelector
beta.kubernetes.io/os is deprecated; use kubernetes.io/os to avoid DaemonSets not scheduling on clusters that only expose the stable node label. https://kubernetes.io/docs/reference/labels-annotations-taints/
Also update the other repo occurrences of beta.kubernetes.io/os to keep fixtures/templates consistent (e.g., openshift/pkg/otp/testdata/bindata.go, helm/ovn-kubernetes/charts/ovn-ipsec/templates/daemonset.yaml).
Proposed fix
nodeSelector:
- beta.kubernetes.io/os: linux
+ kubernetes.io/os: linux📝 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.
| nodeSelector: | |
| beta.kubernetes.io/os: linux | |
| nodeSelector: | |
| kubernetes.io/os: linux |
🤖 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 `@openshift/pkg/otp/testdata/networking/hello-pod-daemonset.yaml` around lines
14 - 15, Replace the deprecated node label key in the DaemonSet YAML by changing
the nodeSelector entry from "beta.kubernetes.io/os" to the stable
"kubernetes.io/os"; update the same key in all other occurrences mentioned
(e.g., entries in openshift/pkg/otp/testdata/bindata.go and
helm/ovn-kubernetes/charts/ovn-ipsec/templates/daemonset.yaml) so
fixtures/templates remain consistent and DaemonSets schedule correctly.
| metadata: | ||
| name: allow-all-ingress |
There was a problem hiding this comment.
Give this policy a unique, intent-matching name.
Line 4 reuses allow-all-ingress, but this manifest only allows traffic from team=operations/name=test-pods. Reusing the real allow-all policy name makes the fixture misleading and risks collisions if both fixtures are applied in the same namespace.
🤖 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 `@openshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yaml`
around lines 3 - 4, The manifest's metadata.name "allow-all-ingress" is
misleading and may collide with real allow-all fixtures; update the
metadata.name in ingress_49696.yaml to a unique, intent-matching value (for
example include the selector/intent like "allow-operations-test-pods-ingress" or
"allow-team-operations-ingress") so it accurately reflects that it only allows
traffic from team=operations/name=test-pods; ensure any tests or references that
expect the old name are updated to the new name.
| metadata: | ||
| name: ipBlock_egress_dual_cidrs_template |
There was a problem hiding this comment.
Rename the Template metadata.name to a DNS-1123-compliant, unique value
metadata.name is ipBlock_egress_dual_cidrs_template, which contains uppercase letters and underscores; Kubernetes metadata.name must be a DNS-1123 label (lowercase alphanumerics and - only). Ensure the renamed value is unique across the related fixtures.
Suggested fix
metadata:
- name: ipBlock_egress_dual_cidrs_template
+ name: ipblock-egress-except-dual-cidrs-template📝 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.
| metadata: | |
| name: ipBlock_egress_dual_cidrs_template | |
| metadata: | |
| name: ipblock-egress-except-dual-cidrs-template |
🤖 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
`@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yaml`
around lines 3 - 4, The manifest's metadata.name
"ipBlock_egress_dual_cidrs_template" is not DNS-1123 compliant (contains
uppercase/underscores); update the Template's metadata.name to a lowercase,
hyphen-separated, unique value (e.g., convert underscores to '-' and lowercase)
ensuring it does not collide with other fixtures; locate the name in the
template where metadata.name is set and replace it (refer to the symbol/value
ipBlock_egress_dual_cidrs_template and the surrounding Template YAML) so it
follows Kubernetes DNS-1123 label rules.
| metadata: | ||
| name: ipBlockEgressSingle_cidr_template |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yaml"
echo "=== File content (lines with metadata.name) ==="
if [ -f "$FILE" ]; then
nl -ba "$FILE" | sed -n '1,120p'
else
echo "Missing file: $FILE" >&2
exit 1
fi
echo
echo "=== Find similar *_template.yaml files for naming patterns ==="
# list template testdata files in same directory tree
DIR="openshift/pkg/otp/testdata/networking/networkpolicy/ipblock"
fd -t f "*-template.yaml" "$DIR" | head -n 50 | sed 's|^|FOUND: |'
echo
echo "=== Extract metadata.name values from nearby template YAMLs ==="
fd -t f "*-template.yaml" "$DIR" | xargs -r -I{} sh -c "echo '--- {}'; rg -n \"^\\s*name:\\s*\" {} | head -n 5" | head -n 200Repository: openshift/ovn-kubernetes
Length of output: 177
🌐 Web query:
Kubernetes validation rules for metadata.name RFC 1123 DNS subdomain lowercase digits hyphen underscore invalid
💡 Result:
In Kubernetes, the metadata.name field for most resource types must conform to the RFC 1123 DNS subdomain standard [1][2]. This validation is enforced by the Kubernetes API server [2][3]. The validation rules for metadata.name are as follows: - Character set: Must consist only of lowercase alphanumeric characters (a-z and 0-9), hyphens (-), and periods (.) [1][3]. - Underscores (_): Underscores are explicitly invalid in metadata.name for resources that follow the RFC 1123 DNS subdomain standard [2][3]. The API server will reject any name containing an underscore with a validation error [2][3]. - Start and end: The name must start and end with an alphanumeric character [1][2]. - Length: The total length must be no more than 253 characters [1][2]. Each individual label (the segments separated by periods) must be no more than 63 characters long [4][5]. It is important to distinguish between metadata.name and Kubernetes labels (metadata.labels). While metadata.name is strictly constrained by DNS subdomain rules, Kubernetes label values and keys have different, more permissive requirements that do allow for underscores [6][7]. For resources like Services that are used in DNS resolution, these naming constraints are strictly enforced to ensure compatibility with DNS standards, where underscores are typically not permitted in hostnames [2].
Citations:
- 1: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
- 2: https://kubemastery.com/en/courses/ckad/object-names-uids-and-dns-rules
- 3: Adding support for _ (underscore) to the configmap metadata.name field kubernetes/kubernetes#83290
- 4: https://github.com/kubernetes/apimachinery/blob/02cfb53916346d085a6c6c7c66f882e3c6b0eca6/pkg/util/validation/validation.go
- 5: Incorrect max length check on metadata.name kubernetes/kubernetes#79351
- 6: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/validate/strfmt.go
- 7: https://cast.ai/blog/kubernetes-labels-expert-guide-with-10-best-practices/
🌐 Web query:
OpenShift Template metadata.name naming validation allowed characters
💡 Result:
In an OpenShift Template (kind Template, API group template.openshift.io), the Template’s metadata.name is the Kubernetes object name for the Template, and it is validated using the Kubernetes object-name rules for “DNS label” style names. Allowed characters (practical rules) - Lowercase letters a-z, digits 0-9, and hyphen “-”. - It must start with a letter (a-z) and end with a letter or digit (a-z, 0-9). - Length: at most 63 characters. These are the RFC 1035 “DNS label” constraints that Kubernetes documents as the name standard for many resource types and that govern metadata.name validation in practice for Kubernetes objects like Templates.[1] Notes / implications - Uppercase letters, underscores, dots “.”, and other characters are not allowed under the DNS-label rule Kubernetes documents for RFC 1035 label names.[1] - OpenShift Template API docs confirm that Template.metadata.name “must be set” and describe it as “name of the Template”, but they don’t repeat the character-level validation rules; those rules come from Kubernetes’ object-name validation.[2][3][1] Example of a valid Template metadata.name - abc-123 Example of invalid Template metadata.name - MyTemplate (uppercase “M” not allowed)[1] - my_template (underscore not allowed)[1] - -bad (can’t start with “-”)[1] - bad- (can’t end with “-”)[1]
Citations:
- 1: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
- 2: https://docs.openshift.com/container-platform/latest/rest_api/template_apis/template-template-openshift-io-v1.html
- 3: https://docs.okd.io/4.10/rest_api/template_apis/template-template-openshift-io-v1.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
DIR="openshift/pkg/otp/testdata/networking/networkpolicy/ipblock"
rg -n "ipBlockEgressSingle_cidr_template|ipBlockEgressSingle" "$DIR" -S || true
rg -n "invalid|must fail|negative|should fail" "$DIR" -S || trueRepository: openshift/ovn-kubernetes
Length of output: 530
Make Template metadata.name DNS-compliant.
metadata.name: ipBlockEgressSingle_cidr_template contains uppercase letters and _, which are rejected by Kubernetes object-name validation for Template resources (DNS label/RFC 1123: lowercase alphanumerics with -/. only). Rename it to a lowercase/dash form.
Suggested fix
metadata:
- name: ipBlockEgressSingle_cidr_template
+ name: ipblock-egress-single-cidr-template📝 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.
| metadata: | |
| name: ipBlockEgressSingle_cidr_template | |
| metadata: | |
| name: ipblock-egress-single-cidr-template |
🤖 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
`@openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yaml`
around lines 3 - 4, The Template metadata.name
"ipBlockEgressSingle_cidr_template" is not DNS-compliant; update the value used
in the file (metadata.name) to a lowercase, RFC1123-compatible label using only
lowercase alphanumerics, '-' or '.', for example
"ipblock-egress-single-cidr-template", and ensure any references to that name in
templates/tests are updated to match.
| ports: | ||
| - name: http | ||
| port: 27017 | ||
| protocol: "${PROTOCOL}" | ||
| targetPort: 8080 | ||
| selector: | ||
| name: "${SELECTOR}" | ||
| type: "${serviceType}" | ||
| parameters: | ||
| - name: SERVICENAME | ||
| - name: NAMESPACE | ||
| - name: internalTrafficPolicy | ||
| - name: externalTrafficPolicy | ||
| - name: ipFamilyPolicy | ||
| - name: PROTOCOL | ||
| - name: SELECTOR | ||
| - name: serviceType | ||
| - name: PORT |
There was a problem hiding this comment.
Wire PORT into the rendered Service or drop it.
PORT is declared as a template parameter, but Line 19 still hard-codes 27017. Any caller passing PORT gets ignored, which makes this "generic" fixture behave unexpectedly.
Proposed fix
ports:
- name: http
- port: 27017
+ port: ${PORT}
protocol: "${PROTOCOL}"
targetPort: 8080📝 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.
| ports: | |
| - name: http | |
| port: 27017 | |
| protocol: "${PROTOCOL}" | |
| targetPort: 8080 | |
| selector: | |
| name: "${SELECTOR}" | |
| type: "${serviceType}" | |
| parameters: | |
| - name: SERVICENAME | |
| - name: NAMESPACE | |
| - name: internalTrafficPolicy | |
| - name: externalTrafficPolicy | |
| - name: ipFamilyPolicy | |
| - name: PROTOCOL | |
| - name: SELECTOR | |
| - name: serviceType | |
| - name: PORT | |
| ports: | |
| - name: http | |
| port: ${PORT} | |
| protocol: "${PROTOCOL}" | |
| targetPort: 8080 | |
| selector: | |
| name: "${SELECTOR}" | |
| type: "${serviceType}" | |
| parameters: | |
| - name: SERVICENAME | |
| - name: NAMESPACE | |
| - name: internalTrafficPolicy | |
| - name: externalTrafficPolicy | |
| - name: ipFamilyPolicy | |
| - name: PROTOCOL | |
| - name: SELECTOR | |
| - name: serviceType | |
| - name: PORT |
🤖 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 `@openshift/pkg/otp/testdata/networking/service-generic-template.yaml` around
lines 17 - 34, The Service template currently hardcodes the port value (port:
27017) while declaring a template parameter PORT; update the ports mapping so
the port field uses the PORT parameter (e.g., replace the literal 27017 with the
template variable for PORT) or if you intend not to vary the port, remove the
PORT parameter from the parameters list; target the ports -> port entry and the
parameters list (PORT) in service-generic-template.yaml to make the template
behavior consistent.
| defer func() { | ||
| if os.Getenv("DELETE_NAMESPACE") != "false" { | ||
| oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute() | ||
| } | ||
| }() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| g.By("Create a duplicate ACL") | ||
| createAclCmd := fmt.Sprintf("ovn-nbctl --id=@copyacl create acl name=copyacl direction=%s action=%s -- add port_group %s acl @copyacl", aclMap["direction"], aclMap["action"], pgMap["_uuid"]) | ||
| idOutput, listErr := otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", createAclCmd) | ||
| o.Expect(listErr).NotTo(o.HaveOccurred()) | ||
| o.Expect(idOutput).NotTo(o.BeEmpty()) | ||
| e2e.Logf(idOutput) | ||
| defer func() { | ||
| if os.Getenv("DELETE_NAMESPACE") != "false" { | ||
| oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute() | ||
| } | ||
| }() | ||
| g.By("Set properties of duplicate ACL") | ||
| setAclPropertiesCmd := fmt.Sprintf("ovn-nbctl set acl %s match='%s' priority=%s meter=%s", idOutput, aclMap["match"], aclMap["priority"], aclMap["meter"]) | ||
| _, listErr = otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", setAclPropertiesCmd) | ||
| o.Expect(listErr).NotTo(o.HaveOccurred()) | ||
| defer func() { | ||
| if os.Getenv("DELETE_NAMESPACE") != "false" { | ||
| oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute() | ||
| } | ||
| }() |
There was a problem hiding this comment.
Duplicate defer statements will attempt cleanup three times.
Lines 1682-1686, 1694-1698, and 1703-1707 all contain identical defer blocks that delete the same networkpolicy. When the test completes, all three will execute sequentially, with the second and third attempts failing because the resource was already deleted.
Keep only one defer (the first one at lines 1682-1686) and remove the other two.
🐛 Proposed fix
defer func() {
if os.Getenv("DELETE_NAMESPACE") != "false" {
oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute()
}
}()
o.Expect(err).NotTo(o.HaveOccurred())
g.By("Create a duplicate ACL")
createAclCmd := fmt.Sprintf("ovn-nbctl --id=`@copyacl` create acl name=copyacl direction=%s action=%s -- add port_group %s acl `@copyacl`", aclMap["direction"], aclMap["action"], pgMap["_uuid"])
idOutput, listErr := otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", createAclCmd)
o.Expect(listErr).NotTo(o.HaveOccurred())
o.Expect(idOutput).NotTo(o.BeEmpty())
e2e.Logf(idOutput)
- defer func() {
- if os.Getenv("DELETE_NAMESPACE") != "false" {
- oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute()
- }
- }()
g.By("Set properties of duplicate ACL")
setAclPropertiesCmd := fmt.Sprintf("ovn-nbctl set acl %s match='%s' priority=%s meter=%s", idOutput, aclMap["match"], aclMap["priority"], aclMap["meter"])
_, listErr = otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", setAclPropertiesCmd)
o.Expect(listErr).NotTo(o.HaveOccurred())
- defer func() {
- if os.Getenv("DELETE_NAMESPACE") != "false" {
- oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute()
- }
- }()
g.By("Set name of duplicate ACL")📝 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.
| defer func() { | |
| if os.Getenv("DELETE_NAMESPACE") != "false" { | |
| oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute() | |
| } | |
| }() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| g.By("Create a duplicate ACL") | |
| createAclCmd := fmt.Sprintf("ovn-nbctl --id=@copyacl create acl name=copyacl direction=%s action=%s -- add port_group %s acl @copyacl", aclMap["direction"], aclMap["action"], pgMap["_uuid"]) | |
| idOutput, listErr := otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", createAclCmd) | |
| o.Expect(listErr).NotTo(o.HaveOccurred()) | |
| o.Expect(idOutput).NotTo(o.BeEmpty()) | |
| e2e.Logf(idOutput) | |
| defer func() { | |
| if os.Getenv("DELETE_NAMESPACE") != "false" { | |
| oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute() | |
| } | |
| }() | |
| g.By("Set properties of duplicate ACL") | |
| setAclPropertiesCmd := fmt.Sprintf("ovn-nbctl set acl %s match='%s' priority=%s meter=%s", idOutput, aclMap["match"], aclMap["priority"], aclMap["meter"]) | |
| _, listErr = otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", setAclPropertiesCmd) | |
| o.Expect(listErr).NotTo(o.HaveOccurred()) | |
| defer func() { | |
| if os.Getenv("DELETE_NAMESPACE") != "false" { | |
| oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute() | |
| } | |
| }() | |
| defer func() { | |
| if os.Getenv("DELETE_NAMESPACE") != "false" { | |
| oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName, "-n", ns).Execute() | |
| } | |
| }() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| g.By("Create a duplicate ACL") | |
| createAclCmd := fmt.Sprintf("ovn-nbctl --id=`@copyacl` create acl name=copyacl direction=%s action=%s -- add port_group %s acl `@copyacl`", aclMap["direction"], aclMap["action"], pgMap["_uuid"]) | |
| idOutput, listErr := otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", createAclCmd) | |
| o.Expect(listErr).NotTo(o.HaveOccurred()) | |
| o.Expect(idOutput).NotTo(o.BeEmpty()) | |
| e2e.Logf(idOutput) | |
| g.By("Set properties of duplicate ACL") | |
| setAclPropertiesCmd := fmt.Sprintf("ovn-nbctl set acl %s match='%s' priority=%s meter=%s", idOutput, aclMap["match"], aclMap["priority"], aclMap["meter"]) | |
| _, listErr = otputils.RemoteShPodWithBashSpecifyContainer(oc, "openshift-ovn-kubernetes", ovnKNodePod, "ovnkube-controller", setAclPropertiesCmd) | |
| o.Expect(listErr).NotTo(o.HaveOccurred()) |
🤖 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 `@openshift/test/otp/networkpolicy.go` around lines 1682 - 1707, The test
contains three identical defer cleanup blocks calling
oc.AsAdmin().WithoutNamespace().Run("delete").Args("networkpolicy", policyName,
"-n", ns).Execute(), which will attempt to delete the same NetworkPolicy three
times; remove the duplicate defers that wrap the createAclCmd and
setAclPropertiesCmd sections and keep only the initial defer (the one before
createAclCmd) so cleanup runs exactly once.
|
/retitle CORENET-7129 Add OVN metrics tests as blocking OTP tests |
openshift-ci
Bot
changed the title
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openshift/test/otp/networkpolicy.go (1)
1781-1790: ⚡ Quick winAvoid shell-wrapped curl calls for dynamic route host.
Use
strings.TrimSpaceand callexec.Commandwith arguments directly; this removes shell parsing risk/flakiness and satisfies the static-analysis finding.♻️ Proposed refactor
- svcRoute, routeErr := oc.AsAdmin().Run("get").Args("route", serviceName, "-n", ns, "-o=jsonpath={.spec.host}").Output() + svcRoute, routeErr := oc.AsAdmin().Run("get").Args("route", serviceName, "-n", ns, "-o=jsonpath={.spec.host}").Output() o.Expect(routeErr).NotTo(o.HaveOccurred()) + svcRoute = strings.TrimSpace(svcRoute) o.Expect(svcRoute).ShouldNot(o.Equal("")) @@ - routeCurlOutput, svcErr = exec.Command("bash", "-c", "curl -sI "+svcRoute).Output() + routeCurlOutput, svcErr = exec.Command("curl", "-sI", svcRoute).Output() @@ - routeCurlOutput, svcErr = exec.Command("bash", "-c", "curl -sI "+svcRoute).Output() + routeCurlOutput, svcErr = exec.Command("curl", "-sI", svcRoute).Output()Also applies to: 1803-1804
🤖 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 `@openshift/test/otp/networkpolicy.go` around lines 1781 - 1790, The test currently invokes exec.Command with "bash -c" and a concatenated svcRoute string which risks shell parsing issues; change the logic in the route access block that uses svcRoute, svcErr, routeCurlOutput and exec.Command("bash","-c","curl -sI "+svcRoute) to first trim svcRoute with strings.TrimSpace(svcRoute) and then call exec.Command with explicit args (e.g., exec.Command("curl", "-sI", trimmedRoute)) so the host is passed as a single argument and avoid shell-wrapping; keep the existing error handling around svcErr and routeCurlOutput unchanged.Source: Linters/SAST tools
🤖 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 `@openshift/test/otp/networkpolicy.go`:
- Around line 2105-2110: The test currently compares raw byte output of
checkMemory2 vs checkMemory1 inside the anonymous function passed to
wait.PollUntilContextTimeout, which is flaky; instead parse the memory values
from checkMemoryCmd output (use checkMemory2 and checkMemory1), convert to a
numeric value (e.g., KiB/MiB as integers), compute the absolute difference (add
a small helper like abs(int) if needed), and return success if the difference is
within a tolerance window (e.g., a few percent or fixed KB); update the
anonymous function logic to log differences and only fail on parse errors or
when the difference exceeds the tolerance.
---
Nitpick comments:
In `@openshift/test/otp/networkpolicy.go`:
- Around line 1781-1790: The test currently invokes exec.Command with "bash -c"
and a concatenated svcRoute string which risks shell parsing issues; change the
logic in the route access block that uses svcRoute, svcErr, routeCurlOutput and
exec.Command("bash","-c","curl -sI "+svcRoute) to first trim svcRoute with
strings.TrimSpace(svcRoute) and then call exec.Command with explicit args (e.g.,
exec.Command("curl", "-sI", trimmedRoute)) so the host is passed as a single
argument and avoid shell-wrapping; keep the existing error handling around
svcErr and routeCurlOutput unchanged.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 7bb65623-8602-4374-9f2a-720a00ba90bb
⛔ Files ignored due to path filters (1)
openshift/go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
openshift/cmd/ovn-kubernetes-tests-ext/main.goopenshift/go.modopenshift/pkg/otp/testdata/bindata.goopenshift/pkg/otp/testdata/networking/OWNERSopenshift/pkg/otp/testdata/networking/hello-pod-daemonset.yamlopenshift/pkg/otp/testdata/networking/hellosdn.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-from-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yamlopenshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yamlopenshift/pkg/otp/testdata/networking/ping-for-pod-template.yamlopenshift/pkg/otp/testdata/networking/service-generic-template.yamlopenshift/pkg/otp/testdata/networking/statefulset-hello.yamlopenshift/pkg/otp/testdata/networking/testpod.yamlopenshift/pkg/otp/utils/utils.goopenshift/test/otp/metrics.goopenshift/test/otp/networkpolicy.go
✅ Files skipped from review due to trivial changes (8)
- openshift/pkg/otp/testdata/networking/networkpolicy/default-allow-egress.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-to-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/OWNERS
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/netpol-30920-75540.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-ingress-red.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-single-multiple-CIDRs-template.yaml
🚧 Files skipped from review as they are similar to previous changes (25)
- openshift/pkg/otp/testdata/networking/networkpolicy/ingress-allow-all.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-egress-red.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/generic-networkpolicy-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-multiple-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-to-openshift-dns.yaml
- openshift/pkg/otp/testdata/networking/ping-for-pod-specific-node-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress_49696.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-from-all-namespaces.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/default-deny-ingress.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/vap-npprotection-blockdeletion.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ingress_49696.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/default-deny-egress.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/allow-same-namespace.yaml
- openshift/pkg/otp/testdata/networking/service-generic-template.yaml
- openshift/test/otp/metrics.go
- openshift/pkg/otp/testdata/networking/ping-for-pod-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-except-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-ingress-single-CIDR-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/ipblock/ipBlock-egress-dual-CIDRs-template.yaml
- openshift/pkg/otp/testdata/networking/networkpolicy/egress-ingress-62524.yaml
- openshift/cmd/ovn-kubernetes-tests-ext/main.go
- openshift/go.mod
| checkErr = wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 300*time.Second, false, func(cxt context.Context) (bool, error) { | ||
| checkMemory2, err := exec.Command("bash", "-c", checkMemoryCmd).CombinedOutput() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| if string(checkMemory2) == string(checkMemory1) { | ||
| e2e.Logf("Memory usage is the same as previous.") | ||
| return true, nil |
There was a problem hiding this comment.
Exact memory-string equality makes this stress test flaky.
oc adm top pod values naturally fluctuate; requiring byte-for-byte equality can fail healthy runs. Compare parsed memory against a tolerance window instead.
🧪 Suggested stabilization
- if string(checkMemory2) == string(checkMemory1) {
+ base := strings.TrimSpace(string(checkMemory1))
+ curr := strings.TrimSpace(string(checkMemory2))
+ parseMi := func(s string) (int, error) {
+ fields := strings.Fields(s)
+ if len(fields) < 2 {
+ return 0, fmt.Errorf("unexpected top output: %q", s)
+ }
+ mem := strings.TrimSuffix(fields[1], "Mi")
+ return strconv.Atoi(mem)
+ }
+ baseMi, err1 := parseMi(base)
+ currMi, err2 := parseMi(curr)
+ if err1 == nil && err2 == nil && abs(currMi-baseMi) <= 20 {
e2e.Logf("Memory usage is the same as previous.")
return true, nil
}func abs(v int) int {
if v < 0 {
return -v
}
return v
}🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 2106-2106: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
🤖 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 `@openshift/test/otp/networkpolicy.go` around lines 2105 - 2110, The test
currently compares raw byte output of checkMemory2 vs checkMemory1 inside the
anonymous function passed to wait.PollUntilContextTimeout, which is flaky;
instead parse the memory values from checkMemoryCmd output (use checkMemory2 and
checkMemory1), convert to a numeric value (e.g., KiB/MiB as integers), compute
the absolute difference (add a small helper like abs(int) if needed), and return
success if the difference is within a tolerance window (e.g., a few percent or
fixed KB); update the anonymous function logic to log differences and only fail
on parse errors or when the difference exceeds the tolerance.
|
Tests are passing e2e-aws-ovn job started: 0/151/510 "[sig-networking] OVN metrics [OTP] 45841-Add OVN flow count metric" passed: (20.2s) 2026-06-08T23:33:06 "[sig-networking] OVN metrics [OTP] 45841-Add OVN flow count metric" started: 0/257/510 "[sig-networking] OVN metrics [OTP] 47471-Record update to cache versus port binding" passed: (1m7s) 2026-06-08T23:34:55 "[sig-networking] OVN metrics [OTP] 47471-Record update to cache versus port binding" started: 2/436/510 "[sig-networking] OVN metrics [OTP] 52072-Add mechanism to record duration for k8 kinds" passed: (17s) 2026-06-08T23:36:16 "[sig-networking] OVN metrics [OTP] 52072-Add mechanism to record duration for k8 kinds" |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
openshift/test/otp/metrics.go (1)
382-382: ⚡ Quick winHandle error in deferred cleanup to avoid silent failures.
The deferred label removal at line 382 discards the result. If the cleanup fails, the namespace label will persist without any indication in the test output.
♻️ Proposed fix
oc.SetupProject() - defer oc.AsAdmin().WithoutNamespace().Run("label").Args("ns", oc.Namespace(), "name-").Output() + defer func() { + if _, err := oc.AsAdmin().WithoutNamespace().Run("label").Args("ns", oc.Namespace(), "name-").Output(); err != nil { + e2e.Logf("Warning: failed to remove namespace label: %v", err) + } + }() _, labelErr := oc.AsAdmin().WithoutNamespace().Run("label").Args("ns", oc.Namespace(), "name=test").Output()🤖 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 `@openshift/test/otp/metrics.go` at line 382, The deferred cleanup currently discards output from oc.AsAdmin().WithoutNamespace().Run("label").Args("ns", oc.Namespace(), "name-").Output(), so capture its (out, err) in a defer func and handle errors instead of ignoring them; e.g., wrap the call in defer func() { out, err := oc.AsAdmin().WithoutNamespace().Run("label").Args("ns", oc.Namespace(), "name-").Output(); if err != nil { t.Logf("failed to remove namespace label: %v, output: %s", err, out) } }() (use the test context variable like t to log the failure).
🤖 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 `@openshift/test/otp/metrics.go`:
- Line 274: The strconv.Atoi calls currently ignore errors (e.g.,
metricValue1Int, _ := strconv.Atoi(metricValue1)); update each to capture and
handle the error: use metricValue1Int, err := strconv.Atoi(metricValue1) and if
err != nil { t.Fatalf("failed to parse metricValue1=%q: %v", metricValue1, err)
} (or the test's equivalent error-fatal helper) and apply the same pattern for
the other occurrences (e.g., metricValue2/metricValueN at the other Atoi sites)
so parsing failures fail the test instead of silently using 0.
- Line 178: The current exec.Command usage building a shell pipeline with the
untrusted variable output (seen as metricOutput := exec.Command("bash", "-c",
"cat "+output+" | grep ...")) is vulnerable to command injection; replace both
occurrences with pure-Go file parsing: open the file named by output, scan lines
(bufio.Scanner), filter lines that contain the metric string
"openshift_unidle_events_total", count to the third match (to replicate awk
'NR==3') and split that line on whitespace to extract the second field, then use
that value instead of metricOutput; update the code paths that use
metricOutput/exec.Command (and any callers like GetOVNMetrics) to accept the
parsed string or value.
- Line 410: The call to strconv.Atoi that assigns metricValue1Int from
metricValue1 currently ignores the error; update the code around
metricValue1Int, strconv.Atoi, and metricValue1 to capture the returned error
(e.g., val, err := strconv.Atoi(metricValue1)) and handle it—if this is in a
test function, call t.Fatalf or t.Fatalf-like helper with a clear message
including metricValue1 and err, otherwise return the error or propagate it
appropriately—so invalid integer strings are detected instead of silently
becoming 0.
- Line 229: The current check uses string substring logic
(!strings.Contains(metricValue, metricNumber)) which fails for numeric
comparisons (e.g., "5" vs "15"); update the comparison in the metrics logic to
parse both metricValue and metricNumber as integers (using strconv.Atoi or
strconv.ParseInt), handle parse errors (log/return on error), and then compare
the numeric values for inequality to decide whether
openshift_unidle_events_total changed instead of using strings.Contains. Target
the variables metricValue and metricNumber (and the surrounding metric
evaluation/polling block) when making this change.
---
Nitpick comments:
In `@openshift/test/otp/metrics.go`:
- Line 382: The deferred cleanup currently discards output from
oc.AsAdmin().WithoutNamespace().Run("label").Args("ns", oc.Namespace(),
"name-").Output(), so capture its (out, err) in a defer func and handle errors
instead of ignoring them; e.g., wrap the call in defer func() { out, err :=
oc.AsAdmin().WithoutNamespace().Run("label").Args("ns", oc.Namespace(),
"name-").Output(); if err != nil { t.Logf("failed to remove namespace label: %v,
output: %s", err, out) } }() (use the test context variable like t to log the
failure).
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 8c19a4c3-6ad3-4982-ab39-44eb4fb2207f
📒 Files selected for processing (4)
openshift/pkg/otp/testdata/networking/egressip-config1-template.yamlopenshift/pkg/otp/testdata/networking/metrics/metrics-pod.yamlopenshift/pkg/otp/utils/utils.goopenshift/test/otp/metrics.go
asood-rh
changed the title
| @@ -0,0 +1,2117 @@ | |||
| package otp | |||
There was a problem hiding this comment.
@asood-rh Thanks. Seems like networkpolicy.go commit came here by mistake? I guess you just meant metrics.go only
There was a problem hiding this comment.
@anuragthehatter In order to use the utils library I created branch off your branch. I do have few changes to ensure metrics tests executed successfully. Will need to deal with merges unless you want to pick up changes in utils.go. I can rebase my branch with yours.
There was a problem hiding this comment.
@asood-rh np at all, as long as you are aware of
| } | ||
| }) | ||
|
|
||
| g.It("[OTP] 45841-Add OVN flow count metric", func() { |
There was a problem hiding this comment.
@asood-rh Thanks, Please add [JIRA:Networking] tag as well to all cases as per OTE guide requirement.
| o.Expect(metricsOutput).NotTo(o.HaveOccurred(), "Fail to get updated metric openshift_unidle_events_total") | ||
| }) | ||
|
|
||
| g.It("[Serial] [OTP] 60539-Verify metrics ovs_vswitchd_interfaces_total", func() { |
There was a problem hiding this comment.
[serial] wont work here. If we want to make all cases serial, I would just add [Suite:openshift/conformance/serial] tag in g.Describe.
There was a problem hiding this comment.
Or if we want to make specific cases serial, I would siffix them with g.serial suffix tag like you have done in one of them
There was a problem hiding this comment.
ah i might be wrong [serial] should also work. Unless dev has any recommendation to be consistent with g.serial or openshift/conformance/seria logic.
Migrate three OVN metrics tests from openshift-tests-private: - 45841: ovn_controller_integration_bridge_openflow_total (flow count) - 47471: pod-to-port-binding duration metrics (4 metrics) - 52072: network programming duration histogram metrics (2 metrics) All three registered as blocking via otpBlockingTests. Depends-On: openshift#3200" for utils.go Signed-off-by: Arti Sood <asood@redhat.com>
Signed-off-by: Arti Sood <asood@redhat.com>
Add tests 45689, 47524, 60539, 60704, 60708 from openshift-tests-private. Fix GetControllerManagerLeaderIP for new lease holderIdentity format. Fix oc idle to run as admin. Remove spurious quotes from GetSpecificPodLogs Signed-off-by: Arti Sood <asood@redhat.com>
1. utils.go — Added ExtractMetricValue() helper (pure Go replacement for shell pipeline)
2. metrics.go — Replaced two exec.Command("bash", "-c", ...) calls with otputils.ExtractMetricValue(); removed "os/exec" import
3. metrics.go — Replaced !strings.Contains(metricValue, metricNumber) with integer comparison (currentValue > baselineValue) in test 45689
4. metrics.go — Added error handling on baseline strconv.Atoi calls in tests 60539 and 60708
Signed-off-by: Arti Sood <asood@redhat.com>
- Blank import openshift/test/otp to register OTP Ginkgo specs
- Add exutil.WithCleanup() to initialize origin's test lifecycle
- Add OTP lifecycle logic: informing by default, blocking if matched
by otpBlockingTests
Signed-off-by: Arti Sood <asood@redhat.com>
openshift-ci
Bot
removed
the
do-not-merge/invalid-owners-file
|
Thanks Arti. |
|
|
@asood-rh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
2 participants
Stacked on #3200. Adds 8 metrics tests -
3 tests (45841, 47471, 52072) as blocking.
[sig-networking] OVN metrics [OTP] 45841-Add OVN flow count metric
[sig-networking] OVN metrics [OTP] 47471-Record update to cache versus port binding
[sig-networking] OVN metrics [OTP] 52072-Add mechanism to record duration for k8 kinds
5 tests as informing
[sig-networking] OVN metrics [OTP] 47524-Metrics for ovn-appctl stopwatch/show command
[sig-networking] OVN metrics [OTP] 45689-Metrics for idling enable/disabled
[sig-networking] OVN metrics [Serial] [OTP] 60539-Verify metrics ovs_vswitchd_interfaces_total
[sig-networking] OVN metrics [Serial] [OTP] 60704-Verify metrics ovs_vswitchd_interface_up_wait_seconds_total
[sig-networking] OVN metrics [Serial] [Slow] [OTP] 60708-Verify metrics ovnkube_resource_retry_failures_total
Summary by CodeRabbit
Tests
Chores
Tests
Style