USHIFT-6744: Migrate workload gingko tests from OTP#6631
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
WalkthroughAdds OTP workload test scenarios and suite validations across RHEL 9.8 and 10.2 platforms in both periodic and release pipelines. New scenario scripts define VMs, environment setup, and test execution for OTP workload tests. Existing scenario files are extended to include OTP test suites alongside their primary test jobs. Four new Robot Framework test suites validate KCM flags, OC CLI behavior, SOS report plugins, and StatefulSet with PVCs. ChangesOTP Workloads Test Scenarios
OTP Workloads Test Suites
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kasturinarra The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@kasturinarra: This pull request references USHIFT-6744 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 task to target the "5.0.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. |
|
@CodeRabbit help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/retest |
|
/test e2e-aws-tests-release e2e-aws-tests-bootc-el9 e2e-aws-tests-release-arm e2e-aws-tests e2e-aws-tests-bootc-el10 |
|
/test e2e-aws-tests-release e2e-aws-tests-release-arm |
|
@agullon could you please help review ? |
| suites/osconfig/clusterid.robot \ | ||
| suites/osconfig/systemd-resolved.robot | ||
| suites/osconfig/systemd-resolved.robot \ | ||
| suites/osconfig/sos-report-plugins.robot |
There was a problem hiding this comment.
why you only add it in scenarios?
I think this should be add it in all the scenario where suites/osconfig/clusterid.robot is.
There was a problem hiding this comment.
The reason i did not add is, in general we are running these gingko cases only in release scenarios right, so i thought adding there alone would be more appropriate than expanding it to other scenarios
290fee5 to
31663a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
test/suites/standard2/dns-custom-config-validation.robot (1)
23-23: ⚡ Quick winPrefer a TEST-NET IP for deterministic test isolation
Use RFC 5737 ranges (for example
198.51.100.97) instead of a globally routable address.🤖 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 `@test/suites/standard2/dns-custom-config-validation.robot` at line 23, The test sets ${FAKE_LISTEN_IP} to a globally routable address (99.99.99.97); update the variable to use an RFC 5737 TEST-NET address (e.g., 198.51.100.97) so tests are deterministic and isolated—replace the value assigned to ${FAKE_LISTEN_IP} in dns-custom-config-validation.robot accordingly.test/suites/otp-workloads/kcm-flags.robot (1)
35-36: ⚡ Quick winScope journal queries to the current boot to reduce stale-log false positives.
Line 35 and Line 59 read the full unit journal. On long-lived hosts this can match old kube-controller-manager starts and make the assertion pass for the wrong run.
Suggested patch
- ... journalctl -u microshift --no-pager | grep kube-controller-manager | grep FLAG + ... journalctl -u microshift -b --no-pager | grep kube-controller-manager | grep FLAG ... - ... journalctl -u microshift --no-pager | grep kube-controller-manager | grep openshift-config + ... journalctl -u microshift -b --no-pager | grep kube-controller-manager | grep openshift-configAlso applies to: 59-60
🤖 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 `@test/suites/otp-workloads/kcm-flags.robot` around lines 35 - 36, The journalctl calls in the test (the command lines that currently use "journalctl -u microshift --no-pager | grep kube-controller-manager | grep FLAG") are reading the entire journal and can match entries from previous boots; update those invocations (lines referencing the kube-controller-manager grep) to scope to the current boot by adding the boot filter (e.g., use "journalctl -u microshift -b --no-pager ..." or "--boot" / "-b 0") so the grep checks only logs from the current boot; apply the same change to both occurrences (the commands around the kube-controller-manager grep on the lines mentioned).
🤖 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 `@test/scenarios/releases/el98-lrel`@osconfig.sh:
- Line 21: The commit commented out the run-stage image guard by disabling the
call to exit_if_commit_not_found; restore that validation by re-enabling the
call to exit_if_commit_not_found "${start_image}" (remove the leading comment)
so the scenario exits early when the start image cannot be resolved; ensure no
alternate bypass logic was added and run the scenario tests to confirm the guard
triggers as expected.
In `@test/scenarios/releases/el98-lrel`@otp-workloads.sh:
- Line 21: The commented-out guard exit_if_commit_not_found "${start_image}"
should not be left as dead code; either restore it by uncommenting the call to
exit_if_commit_not_found with the "${start_image}" argument so the scenario
enforces the commit check, or remove the commented line entirely to keep the
script consistent with other scenario flows; locate the commented string
"exit_if_commit_not_found \"${start_image}\"" and perform one of those two
actions.
In `@test/suites/otp-workloads/sos-report-plugins.robot`:
- Around line 57-67: The current comparisons use raw `ls -l` output (variables
${system_manifests}/${report_manifests} and
${system_manifests_d}/${report_manifests_d}) which is brittle; replace the `ls
-l` commands with deterministic listings or checksums — e.g., run a normalized
command that lists filenames sorted (or computes checksums like md5sum) under
/etc/microshift/manifests and /etc/microshift/manifests.d in both system and
${sos_report_extracted}, store those into the same variables, and then use
Should Be Equal As Strings to compare the normalized/sorted filename lists or
checksum outputs so comparisons are stable across metadata formatting
differences.
In `@test/suites/otp-workloads/statefulset-pvc.robot`:
- Around line 73-75: Replace the format-dependent labels fetch with a direct
jsonpath query of the specific label key: instead of calling Run With Kubeconfig
to get {.metadata.labels} into ${labels} and using Should Contain, call oc get
pvc ${pvc_name} -n ${namespace} -o jsonpath='{.metadata.labels["${label_key}"]}'
(or {.metadata.labels.${label_key}}) into a variable like ${label_value_actual}
and then assert equality with Should Be Equal ${label_value_actual}
${label_value}; update references to ${labels}, Should Contain, and the jsonpath
string accordingly so the test compares the label value exactly.
In `@test/suites/standard2/dns-custom-config-validation.robot`:
- Around line 141-144: The journal cursor is captured too late which can miss
the corefile removal event; move the Get Journal Cursor call so it happens
before the step that deletes the Corefile (i.e., call Get Journal Cursor
immediately prior to the "Remove Custom Corefile" action), then keep the
subsequent "Pattern Should Appear In Log Output" that uses that cursor to assert
the "watched file.*was removed, keeping last known ConfigMap content" message.
- Around line 130-132: The teardown currently runs "Command Should Work rm
-rf /etc/microshift/dns" which can remove unrelated production data; change the
teardown to a safe removal that only deletes test-created files or verifies the
directory is a test sandbox before removing it. Replace the direct rm -rf call
in the [Teardown] Run Keywords sequence (the "Command Should Work" invocation)
with a guarded approach: either remove only a known test subdirectory or
specific files, or first check a marker/ownership (e.g. test-created flag or
unique temp path) before calling removal, or use Robot keywords like "Directory
Should Exist" + "Remove Directory" on a test-scoped path; keep "Cleanup
Validation Test" as the final teardown step.
---
Nitpick comments:
In `@test/suites/otp-workloads/kcm-flags.robot`:
- Around line 35-36: The journalctl calls in the test (the command lines that
currently use "journalctl -u microshift --no-pager | grep
kube-controller-manager | grep FLAG") are reading the entire journal and can
match entries from previous boots; update those invocations (lines referencing
the kube-controller-manager grep) to scope to the current boot by adding the
boot filter (e.g., use "journalctl -u microshift -b --no-pager ..." or "--boot"
/ "-b 0") so the grep checks only logs from the current boot; apply the same
change to both occurrences (the commands around the kube-controller-manager grep
on the lines mentioned).
In `@test/suites/standard2/dns-custom-config-validation.robot`:
- Line 23: The test sets ${FAKE_LISTEN_IP} to a globally routable address
(99.99.99.97); update the variable to use an RFC 5737 TEST-NET address (e.g.,
198.51.100.97) so tests are deterministic and isolated—replace the value
assigned to ${FAKE_LISTEN_IP} in dns-custom-config-validation.robot accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9e51e208-8ecc-4b72-8dd9-96794339df61
📒 Files selected for processing (15)
test/scenarios-bootc/el10/periodics/el102-src@osconfig-lifecycle.shtest/scenarios-bootc/el10/releases/el102-lrel@osconfig-router.shtest/scenarios-bootc/el10/releases/el102-lrel@otp-workloads.shtest/scenarios-bootc/el9/periodics/el98-src@osconfig-lifecycle.shtest/scenarios-bootc/el9/releases/el98-lrel@osconfig-router.shtest/scenarios-bootc/el9/releases/el98-lrel@otp-workloads.shtest/scenarios/periodics/el98-src@osconfig-lifecycle.shtest/scenarios/releases/el98-lrel@osconfig.shtest/scenarios/releases/el98-lrel@otp-workloads.shtest/suites/otp-workloads/kcm-flags.robottest/suites/otp-workloads/oc-cli.robottest/suites/otp-workloads/sos-report-plugins.robottest/suites/otp-workloads/statefulset-pvc.robottest/suites/standard2/dns-custom-config-validation.robottest_cases_USHIFT-6900.md
✅ Files skipped from review due to trivial changes (2)
- test/scenarios-bootc/el10/releases/el102-lrel@osconfig-router.sh
- test_cases_USHIFT-6900.md
| ${system_manifests}= Command Should Work ls -l /etc/microshift/manifests | ||
| ${report_manifests}= Command Should Work | ||
| ... ls -l ${sos_report_extracted}/etc/microshift/manifests | ||
| Should Be Equal As Strings ${report_manifests} ${system_manifests} | ||
| ... msg=Files inside /etc/microshift/manifests directory does not match between system and sosreport | ||
|
|
||
| ${system_manifests_d}= Command Should Work ls -l /etc/microshift/manifests.d/ | ||
| ${report_manifests_d}= Command Should Work | ||
| ... ls -l ${sos_report_extracted}/etc/microshift/manifests.d/ | ||
| Should Be Equal As Strings ${report_manifests_d} ${system_manifests_d} | ||
| ... msg=Files inside /etc/microshift/manifests.d directory does not match between system and sosreport |
There was a problem hiding this comment.
Use deterministic comparisons instead of raw ls -l output.
These assertions are brittle and can fail on metadata formatting differences. Compare normalized filenames (or checksums) to avoid flaky results.
Suggested fix
- ${system_manifests}= Command Should Work ls -l /etc/microshift/manifests
+ ${system_manifests}= Command Should Work
+ ... find /etc/microshift/manifests -mindepth 1 -maxdepth 1 -printf '%f\n' | sort
${report_manifests}= Command Should Work
- ... ls -l ${sos_report_extracted}/etc/microshift/manifests
+ ... find ${sos_report_extracted}/etc/microshift/manifests -mindepth 1 -maxdepth 1 -printf '%f\n' | sort
Should Be Equal As Strings ${report_manifests} ${system_manifests}
... msg=Files inside /etc/microshift/manifests directory does not match between system and sosreport
- ${system_manifests_d}= Command Should Work ls -l /etc/microshift/manifests.d/
+ ${system_manifests_d}= Command Should Work
+ ... find /etc/microshift/manifests.d -mindepth 1 -maxdepth 1 -printf '%f\n' | sort
${report_manifests_d}= Command Should Work
- ... ls -l ${sos_report_extracted}/etc/microshift/manifests.d/
+ ... find ${sos_report_extracted}/etc/microshift/manifests.d -mindepth 1 -maxdepth 1 -printf '%f\n' | sort
Should Be Equal As Strings ${report_manifests_d} ${system_manifests_d}
... msg=Files inside /etc/microshift/manifests.d directory does not match between system and sosreport🤖 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 `@test/suites/otp-workloads/sos-report-plugins.robot` around lines 57 - 67, The
current comparisons use raw `ls -l` output (variables
${system_manifests}/${report_manifests} and
${system_manifests_d}/${report_manifests_d}) which is brittle; replace the `ls
-l` commands with deterministic listings or checksums — e.g., run a normalized
command that lists filenames sorted (or computes checksums like md5sum) under
/etc/microshift/manifests and /etc/microshift/manifests.d in both system and
${sos_report_extracted}, store those into the same variables, and then use
Should Be Equal As Strings to compare the normalized/sorted filename lists or
checksum outputs so comparisons are stable across metadata formatting
differences.
31663a5 to
e3dd561
Compare
|
/retitle USHIFT-6744: Migrate workload gingko tests from OTP |
| Verify Default StorageClass Exists | ||
| [Documentation] Skip test if no default StorageClass is available. | ||
| ${output} ${rc}= Run With Kubeconfig | ||
| ... oc get sc -o jsonpath\='{.items[?(@.metadata.annotations.storageclass\\.kubernetes\\.io/is-default-class=="true")].metadata.name}' |
There was a problem hiding this comment.
can you reuse the Oc Get and other keywords from oc.resource files? You can tell Claude to do it
e3dd561 to
94d6a08
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/suites/standard2/dns-custom-config-validation.robot (1)
131-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
rm -rfon/etc/microshift/dnsin teardownLine 132 is still unsafe: recursive delete on a host path can remove non-test data. Prefer removing only the test file and optionally removing the directory if empty.
Safer teardown change
- [Teardown] Run Keywords - ... Command Should Work rm -rf /etc/microshift/dns - ... AND Cleanup Validation Test + [Teardown] Run Keywords + ... Remove Custom Corefile + ... AND Command Should Work rmdir /etc/microshift/dns || true + ... AND Cleanup Validation Test🤖 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 `@test/suites/standard2/dns-custom-config-validation.robot` around lines 131 - 133, The teardown is using a dangerous recursive delete (the "Command Should Work rm -rf /etc/microshift/dns" call under the [Teardown] / Run Keywords block); replace it with a safe two-step cleanup that only removes the specific test artifact(s) and then removes the directory only if empty. Concretely, update the teardown to call "Command Should Work" (or the Robot "Remove File"/"Run" variant) to delete the exact test file(s) you created in /etc/microshift/dns, and then run an idempotent directory removal (e.g., "rmdir --ignore-fail-on-non-empty /etc/microshift/dns" or perform a check that the directory is empty before removing) instead of using rm -rf, keeping the rest of the teardown (including the "Cleanup Validation Test" keyword) intact.
🧹 Nitpick comments (1)
test/suites/standard2/dns-custom-config-validation.robot (1)
240-243: ⚡ Quick winRestore service state in this teardown too
This teardown removes config artifacts but does not restart MicroShift. Add restart for deterministic cleanup, consistent with
Cleanup Validation Test.Teardown consistency fix
[Teardown] Run Keywords ... Remove Custom Corefile ... AND Remove Drop In MicroShift Config 20-dns-custom + ... AND Restart MicroShift🤖 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 `@test/suites/standard2/dns-custom-config-validation.robot` around lines 240 - 243, The teardown currently calls the keywords Remove Custom Corefile and Remove Drop In MicroShift Config 20-dns-custom but does not restart MicroShift; update the [Teardown] sequence in dns-custom-config-validation.robot to include the same restart step used in the Cleanup Validation Test (i.e., add the keyword that restarts MicroShift after Remove Drop In MicroShift Config) so the service is deterministically restored during cleanup.
🤖 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 `@test/suites/standard2/dns-custom-config-validation.robot`:
- Around line 376-384: The test "CoreDNS Pod Should Not Be Ready" uses container
restartCount > 0 which can reflect past restarts; instead query the pod's
current readiness flag (e.g., use jsonpath
{.items[0].status.containerStatuses[0].ready} or
{.items[0].status.conditions[?(@.type=="Ready")].status}) and assert it is
"False" (or boolean false) to detect an unhealthy CoreDNS pod; update the Run
With Kubeconfig call in that test to fetch the readiness field and replace the
restartCount-based assertions with a check that readiness == False and an
explanatory failure message referencing the pod readiness state.
---
Duplicate comments:
In `@test/suites/standard2/dns-custom-config-validation.robot`:
- Around line 131-133: The teardown is using a dangerous recursive delete (the
"Command Should Work rm -rf /etc/microshift/dns" call under the [Teardown] /
Run Keywords block); replace it with a safe two-step cleanup that only removes
the specific test artifact(s) and then removes the directory only if empty.
Concretely, update the teardown to call "Command Should Work" (or the Robot
"Remove File"/"Run" variant) to delete the exact test file(s) you created in
/etc/microshift/dns, and then run an idempotent directory removal (e.g., "rmdir
--ignore-fail-on-non-empty /etc/microshift/dns" or perform a check that the
directory is empty before removing) instead of using rm -rf, keeping the rest of
the teardown (including the "Cleanup Validation Test" keyword) intact.
---
Nitpick comments:
In `@test/suites/standard2/dns-custom-config-validation.robot`:
- Around line 240-243: The teardown currently calls the keywords Remove Custom
Corefile and Remove Drop In MicroShift Config 20-dns-custom but does not restart
MicroShift; update the [Teardown] sequence in dns-custom-config-validation.robot
to include the same restart step used in the Cleanup Validation Test (i.e., add
the keyword that restarts MicroShift after Remove Drop In MicroShift Config) so
the service is deterministically restored during cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0f14fa01-1db6-4fc1-9ed2-558967ca25d5
📒 Files selected for processing (15)
test/scenarios-bootc/el10/periodics/el102-src@osconfig-lifecycle.shtest/scenarios-bootc/el10/releases/el102-lrel@osconfig-router.shtest/scenarios-bootc/el10/releases/el102-lrel@otp-workloads.shtest/scenarios-bootc/el9/periodics/el98-src@osconfig-lifecycle.shtest/scenarios-bootc/el9/releases/el98-lrel@osconfig-router.shtest/scenarios-bootc/el9/releases/el98-lrel@otp-workloads.shtest/scenarios/periodics/el98-src@osconfig-lifecycle.shtest/scenarios/releases/el98-lrel@osconfig.shtest/scenarios/releases/el98-lrel@otp-workloads.shtest/suites/otp-workloads/kcm-flags.robottest/suites/otp-workloads/oc-cli.robottest/suites/otp-workloads/sos-report-plugins.robottest/suites/otp-workloads/statefulset-pvc.robottest/suites/standard2/dns-custom-config-validation.robottest_cases_USHIFT-6900.md
✅ Files skipped from review due to trivial changes (1)
- test_cases_USHIFT-6900.md
🚧 Files skipped from review as they are similar to previous changes (12)
- test/scenarios-bootc/el9/releases/el98-lrel@osconfig-router.sh
- test/scenarios/releases/el98-lrel@otp-workloads.sh
- test/scenarios-bootc/el9/periodics/el98-src@osconfig-lifecycle.sh
- test/scenarios/periodics/el98-src@osconfig-lifecycle.sh
- test/scenarios-bootc/el10/periodics/el102-src@osconfig-lifecycle.sh
- test/scenarios-bootc/el10/releases/el102-lrel@osconfig-router.sh
- test/scenarios-bootc/el9/releases/el98-lrel@otp-workloads.sh
- test/suites/otp-workloads/sos-report-plugins.robot
- test/suites/otp-workloads/statefulset-pvc.robot
- test/suites/otp-workloads/kcm-flags.robot
- test/scenarios-bootc/el10/releases/el102-lrel@otp-workloads.sh
- test/suites/otp-workloads/oc-cli.robot
94d6a08 to
3201c52
Compare
|
@kasturinarra: 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. |
Summary by CodeRabbit