OCPEDGE-2381: Verify etc pod should start with oom_score_adj as system-node-critical#30867
OCPEDGE-2381: Verify etc pod should start with oom_score_adj as system-node-critical#30867kasturinarra wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@kasturinarra: This pull request references OCPEDGE-2381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughTest file modified to add three new constants for etcd OOM configuration queries and two duplicate test blocks that verify etcd OOM score adjustment equals -997 across control-plane nodes via pcs resource checks and direct file reads. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@kasturinarra: This pull request references OCPEDGE-2381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kasturinarra 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.
🧹 Nitpick comments (1)
test/extended/two_node/tnf_topology.go (1)
20-20: Remove redundant PCS metadata check or switch to live resource configuration.
pcs resource describe ocf:heartbeat:podman-etcdreturns only OCF agent metadata and default parameter values, not the live per-resource-instance configuration in your cluster. The actual validation at lines 152-158 that reads/proc/$pid/oom_score_adjis the definitive end-to-end check. Either remove the PCS assertion (lines 145-150) and its const (line 20), or replace it withpcs resource showto verify the live resource instance configuration.♻️ Minimal cleanup if removing the redundant check
-const getPcsEtcdOOMDefault = "pcs resource describe ocf:heartbeat:podman-etcd" const expectedOOMScoreAdj = "-997" ... - g.By("Verifying pcs resource describes etcd with OOM default of -997") - firstNode := nodes.Items[0] - pcsOutput, err := exutil.DebugNodeRetryWithOptionsAndChroot(oc, firstNode.Name, "openshift-etcd", "bash", "-c", getPcsEtcdOOMDefault) - o.Expect(err).To(o.BeNil(), fmt.Sprintf("expected pcs resource describe to succeed on Node %s: error %v", firstNode.Name, err)) - o.Expect(pcsOutput).To(o.ContainSubstring("Default: -997"), - fmt.Sprintf("expected pcs resource describe to show OOM default of -997 on Node %s, got: %s", firstNode.Name, pcsOutput)) - g.By("Ensuring etcd process OOM score adjustment is -997 on each node")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/two_node/tnf_topology.go` at line 20, The PCS metadata check is redundant because const getPcsEtcdOOMDefault (the "pcs resource describe ocf:heartbeat:podman-etcd" string) only returns agent metadata not live instance config; either remove the const and the associated PCS assertion block that uses it, or change the check to use "pcs resource show <resource>" (i.e., call pcs resource show for the specific ocf:heartbeat:podman-etcd resource) so you validate the live per-instance configuration instead of the OCF defaults; update any references in tnf_topology.go to use the new command or remove them along with the assertion that precedes the /proc/$pid/oom_score_adj validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/two_node/tnf_topology.go`:
- Line 20: The PCS metadata check is redundant because const
getPcsEtcdOOMDefault (the "pcs resource describe ocf:heartbeat:podman-etcd"
string) only returns agent metadata not live instance config; either remove the
const and the associated PCS assertion block that uses it, or change the check
to use "pcs resource show <resource>" (i.e., call pcs resource show for the
specific ocf:heartbeat:podman-etcd resource) so you validate the live
per-instance configuration instead of the OCF defaults; update any references in
tnf_topology.go to use the new command or remove them along with the assertion
that precedes the /proc/$pid/oom_score_adj validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cd76ffb-11a8-4251-b953-4956df8efad6
📒 Files selected for processing (1)
test/extended/two_node/tnf_topology.go
|
@dhensel-rh @jaypoulz could you please help review ? thanks !! |
|
Scheduling required tests: |
|
/retest |
1 similar comment
|
/retest |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-metal-ovn-two-node-fencing-techpreview |
|
@kasturinarra: 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/58390e60-1eee-11f1-96d0-85647d1d0cd4-0 |
|
/test e2e-aws-ovn-microshift |
|
@kasturinarra: all tests passed! 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 7a768c8
New tests seen in this PR at sha: 7a768c8
|
3 participants
Summary by CodeRabbit
Release Notes