run DC script only if DC detected + #2033
Conversation
WalkthroughReplaces todoListReady with verifyApplicationReady (expanded signature) across e2e tests, adds optional SCC checks, short‑circuits DC readiness when no DeploymentConfig exists, and converts multiple sample manifests from Deployment (apps/v1) to DeploymentConfig (apps.openshift.io/v1) while changing probes, startupProbe commands, and some container images. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
approved
|
@kaovilai as you have time, took your suggestion from the 1.3 pr to remove the parks-app verify. Let me know what you think sir. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/e2e/lib/apps.go (1)
251-263: Short-circuiting IsDCReady when no DCs exist is reasonableThe new pre-check via
HasDCsInNamespacecleanly avoids DC readiness checks in namespaces that don’t have any DeploymentConfigs, which aligns with “run DC logic only if DC detected”. When DCs are present, behavior remains as before (Get + status checks).This does add an extra list call per poll; if it ever becomes hot, you could instead rely on
Getand treat aNotFoundfordcNameas “no DC to wait on”, avoiding the list. Not critical, but something to consider.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
tests/e2e/backup_restore_cli_suite_test.go(10 hunks)tests/e2e/backup_restore_suite_test.go(12 hunks)tests/e2e/lib/apps.go(1 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml(1 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml(1 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml(1 hunks)tests/e2e/sample-applications/mongo-persistent/pvc/ibmcloud-block-mode.yaml(1 hunks)tests/e2e/virt_backup_restore_suite_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
tests/e2e/virt_backup_restore_suite_test.gotests/e2e/backup_restore_cli_suite_test.gotests/e2e/lib/apps.gotests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yamltests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yamltests/e2e/sample-applications/mongo-persistent/pvc/ibmcloud-block-mode.yamltests/e2e/backup_restore_suite_test.gotests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml
🪛 golangci-lint (2.5.0)
tests/e2e/backup_restore_suite_test.go
[error] 71-71: File is not properly formatted
(gci)
🔇 Additional comments (7)
tests/e2e/sample-applications/mongo-persistent/pvc/ibmcloud-block-mode.yaml (1)
18-18: No behavioral change in PVC spec
storage: 1Giis unchanged semantically; only formatting/EOF normalization appears to have changed. Safe to keep as-is.tests/e2e/virt_backup_restore_suite_test.go (1)
21-21: Comment reference updated correctlyThe TODO now references
verifyApplicationReadyinstead of the old helper, which matches the refactor and keeps the note accurate.tests/e2e/backup_restore_cli_suite_test.go (2)
186-186: Uppercasing CLI test state improves log readabilityPrinting
report.Statein uppercase makes the post-test status stand out in logs without affecting behavior. Looks good.
263-361: verifyApplicationReady wiring across CLI tests looks consistentThe new
verifyApplicationReadyusage for all CLI table entries is consistent:
preBackupState/postRestoretoggled via the first bool.twoVolset totrueonly for the twovol case.- App/route names
"todolist"and"todolist-route"match the sample manifests.- Database identifiers
"mysql"/"mongo"line up with the underlying app templates.isDCisfalsein all these cases, which is appropriate for the Deployment-based workloads.- SCC names (
mysql-persistent-scc,mongo-persistent-scc) match the security constraints in the sample manifests.This centralizes readiness checks and should make the CLI tests easier to maintain.
tests/e2e/backup_restore_suite_test.go (3)
46-69: Well-structured refactoring that generalizes application verification.The parameterized approach removes hard dependencies and makes the verification function reusable across different application types. The conditional logic for DC vs. Deployment checks and optional SCC validation is clean and appropriate.
317-317: Improved log visibility for test status.Uppercasing the status (PASSED/FAILED) makes it easier to scan logs, aligning with the PR objectives.
503-504: Verify the empty workloadName in Parks app PostRestoreVerify.The PreBackupVerify checks
workloadName: "restify"(line 503), but PostRestoreVerify has an emptyworkloadName: ""(line 504). This means no DC readiness check will be performed after restore, even thoughisDC: trueis still set.Is this intentional? If the workload readiness check is needed post-restore, consider:
Apply this diff if the workload check is needed:
- PostRestoreVerify: verifyApplicationReady(false, false, "restify", "restify", "", true, ""), + PostRestoreVerify: verifyApplicationReady(false, false, "restify", "restify", "restify", true, ""),If the empty workloadName is intentional (e.g., the RestoreHooks are sufficient or the DC name changes), please confirm this is the expected behavior.
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml
Show resolved
Hide resolved
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml
Show resolved
Hide resolved
|
/retest ai-retester: The e2e-test-aws job failed because the "Mongo application DATAMOVER" test case timed out after 600s, and the todolist container was waiting to start due to a |
|
ai-explains-test-failure: The job failed because the lint step broke. |
|
/retest ai-retester: The e2e tests failed during the |
|
ai-explains-test-failure: Because the lint step failed – |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
ai-explains-test-failure: The job died during the |
|
/hold |
|
/test all |
|
let's see if it fully passes two times :) |
openshift-ci
bot
removed
the
approved
* refactor verify todolist and parks-app verify into single app. thanks @tiger * update test calls * while testing mongo, use solid known pattern from oadp-1.5 * get DAT CAPS on PASSED/FAILED status
weshayutin
force-pushed
the
oadpdev-dc-check
branch
from
2b5915f to
89e9ec4
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: weshayutin 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 |
openshift-ci
bot
added
the
approved
|
/retest |
|
/test all |
1 similar comment
|
/test all |
|
/retest ai-retester: The "Mongo application DATAMOVER" e2e test failed because the "todolist" container in the test pod consistently failed to start due to a 'PodInitializing' state, eventually timing out the test. comment for /pull/2033 |
|
/test all |
|
/retest ai-retester: The e2e tests failed because the "Mongo application DATAMOVER" test timed out after 540 seconds, specifically waiting for the todolist pod to become ready ( comment for /pull/2033 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml (1)
17-35: SCC is extremely permissive; confine to tests and minimize privilege if possible
This SCC grants broad privileges (privileged containers, allowPrivilegeEscalation, all volume types, and includessystem:admin). If this is strictly for CI/e2e, consider tightening (or documenting why it must be this open) to avoid normalizing insecure defaults.tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml (1)
69-109: Privileged +runAsUser: 0for block-device init is high-risk; keep scope tightly test-only
Given this is formatting/mounting a block device, privilege may be necessary, but it should remain strictly confined to e2e assets and not be reused as a template elsewhere (Checkov warning is expected here).tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml (1)
17-35: SCC is extremely permissive; confine to tests and minimize privilege if possible
Same SCC concerns: privileged, allowPrivilegeEscalation, all volumes, includessystem:admin. Consider tightening or adding a comment explaining why this level is required for the e2e environment.tests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml (1)
17-36: Overly permissive SCC (allowPrivilegeEscalation/privileged) will keep tripping scanners; tighten if not strictly required
Even for e2e sample assets, this broad SCC increases risk and creates noisy security findings (matches the Checkov hint). If the workloads don’t truly require it, restrict the SCC and/or scope it to only what’s needed.tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml (2)
17-36: Permissive SCC still grants escalation/privileged; tighten to reduce security risk and Checkov noise
Right now the SCC allows more than the pods should need (especially onceprivileged: trueis removed).
75-89: Dropprivileged: truefor MariaDB—it is unnecessary and creates security riskMariaDB database does not require privileged mode; volume permissions are handled by
fsGroup: 27. This is inconsistent withmysql-persistent.yamlin the same directory, which correctly usesprivileged: falsewith proper hardening. The accompanying SecurityContextConstraints grants privilege capability, but the application container should not rely on or require it.Apply the suggested changes: set
privileged: falseand addallowPrivilegeEscalation: false, drop all capabilities, and useRuntimeDefaultseccomp profile.tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml (1)
30-49: SCC is much broader than the pod securityContext now—tighten it to match
Given the container is explicitly non-privileged and disallows escalation, the SCC allowing privileged/escalation is inconsistent and keeps security tooling noisy.
♻️ Duplicate comments (2)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml (1)
113-125: Fix MongostartupProbe:$(ENV_VAR)won’t expand in exec-array form (probe likely fails)Kubelet doesn’t perform
$(...)env expansion inexec.command[], somongoshwill receive literal strings and auth will likely fail, causing probe failures/restarts and e2e timeouts. This matches the prior review concern.startupProbe: exec: command: - - mongosh - - admin - - -u $(MONGO_INITDB_ROOT_USERNAME) - - -p $(MONGO_INITDB_ROOT_PASSWORD) - - --eval 'printjson(db.getCollectionNames())' + - /bin/bash + - -c + - | + mongosh admin \ + -u "$MONGO_INITDB_ROOT_USERNAME" \ + -p "$MONGO_INITDB_ROOT_PASSWORD" \ + --eval 'printjson(db.getCollectionNames())' initialDelaySeconds: 10 periodSeconds: 10tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml (1)
126-138: Fix MongostartupProbe:$(ENV_VAR)won’t expand in exec-array form (probe likely fails)
Same issue as previously noted: kubelet won’t expand$(MONGO_INITDB_ROOT_USERNAME/PASSWORD)here, so the probe likely fails and can trigger restarts/timeouts.startupProbe: exec: command: - - mongosh - - admin - - -u $(MONGO_INITDB_ROOT_USERNAME) - - -p $(MONGO_INITDB_ROOT_PASSWORD) - - --eval 'printjson(db.getCollectionNames())' + - /bin/bash + - -c + - | + mongosh admin \ + -u "$MONGO_INITDB_ROOT_USERNAME" \ + -p "$MONGO_INITDB_ROOT_PASSWORD" \ + --eval 'printjson(db.getCollectionNames())' initialDelaySeconds: 10 periodSeconds: 10
🧹 Nitpick comments (6)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml (2)
99-105:mysqllivenessProbe switched to TCP-only; consider restoring a readiness signal to avoid “Ready but not usable” flakes
WithreadinessProberemoved, the pod will generally become Ready as soon as containers start, even if MySQL isn’t accepting auth/queries yet. If e2e logic (orwait-for-ready) assumes true service readiness, this can reintroduce timeouts.livenessProbe: tcpSocket: port: mysql initialDelaySeconds: 30 periodSeconds: 10 timeoutSeconds: 5 + readinessProbe: + tcpSocket: + port: mysql + initialDelaySeconds: 5 + periodSeconds: 5 + timeoutSeconds: 2
145-188: VerifyDeploymentConfigrollouts: add explicit triggers (esp. ConfigChange) if CI shows DC not creating RC/pods
Some OpenShift setups expectspec.triggersonDeploymentConfigto reliably rollout on create / config changes. If CI ever shows the DC present but no RC/pods, add triggers explicitly.- apiVersion: apps.openshift.io/v1 kind: DeploymentConfig @@ spec: replicas: 1 selector: app: todolist service: todolist + triggers: + - type: ConfigChange strategy: type: Recreatetests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml (2)
107-113: Same readiness concern: TCP liveness alone doesn’t prevent early “Ready”
If CI flakes on “deployment not becoming ready” vs “service not usable”, consider restoring a lightweightreadinessProbe(TCP is fine).
154-190: DeploymentConfig: consider addingtriggers: [ConfigChange]for predictable rollouts
If you see DC objects created but no rollout in some CI environments, make triggers explicit.tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml (2)
120-125: Consider a readinessProbe (TCP is OK) to avoid mysql being marked Ready too early
Without readiness, endpoints may flip to Ready before the DB is truly accepting connections, which can amplify e2e flakiness.
168-203: DeploymentConfig: addtriggers: [ConfigChange]if you need consistent rollouts across clusters
Worth making explicit if CI ever shows DC present but no rollout / no RC created.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml(4 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml(4 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml(4 hunks)tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml(3 hunks)tests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml(3 hunks)tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yamltests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yamltests/e2e/sample-applications/mysql-persistent/mysql-persistent.yamltests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yamltests/e2e/sample-applications/mongo-persistent/mongo-persistent.yamltests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml
🪛 Checkov (3.2.334)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml
[medium] 150-186: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml
[medium] 191-227: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml
[medium] 167-203: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml
[medium] 154-190: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml
[medium] 163-199: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml
[medium] 145-188: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
🔇 Additional comments (5)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml (1)
150-185: DeploymentConfig migration fortodolistlooks consistent (selector/labels/image)
DC selector keys match pod template labels, and the service selector (app: todolist) will still match.tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml (2)
156-166: MongostartupProbechange tomongosh ... pingvia/bin/bashis solid
Avoids the env-var expansion pitfall and keeps probe semantics simple.
191-227: DeploymentConfig migration fortodolistlooks consistent (selector/labels/image)
Selector matches pod labels; image bump is straightforward.tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml (1)
163-199: DeploymentConfig migration fortodolistlooks consistent (selector/labels/image)
DC selector matches labels; service selector remains compatible.tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml (1)
88-102: Nice hardening on the mysql container securityContext
Dropping caps +allowPrivilegeEscalation: false+seccompProfile: RuntimeDefaultis a solid baseline.
|
/retest ai-retester: The e2e test failed because the comment for /pull/2033 |
|
/retest ai-retester: The ci-index image was built for amd64 but the runner is an arm64 node. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml (1)
154-190: Add health probes to the todolist container.The todolist container (lines 178-185) lacks both liveness and readiness probes, even though it serves HTTP traffic on port 8000. Without these probes:
- Kubernetes cannot detect if the application crashes or becomes unresponsive
- Traffic may be routed to unhealthy pods
- Failed containers won't be automatically restarted based on application health
This creates an inconsistency with the MySQL container (lines 82-129), which has both liveness and startup probes defined.
Consider adding probes similar to:
spec: containers: - name: todolist image: quay.io/migtools/oadp-ci-todolist-mariadb-go:latest env: - name: foo value: bar ports: - containerPort: 8000 protocol: TCP + livenessProbe: + httpGet: + path: / + port: 8000 + initialDelaySeconds: 30 + periodSeconds: 10 + timeoutSeconds: 5 + readinessProbe: + httpGet: + path: / + port: 8000 + initialDelaySeconds: 10 + periodSeconds: 5 + timeoutSeconds: 3 initContainers:
♻️ Duplicate comments (3)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml (1)
129-133: Startup probe environment variable expansion issue - already flagged.This concern was already raised in a previous review. The
$(MONGO_INITDB_ROOT_USERNAME)and$(MONGO_INITDB_ROOT_PASSWORD)tokens will not be expanded by Kubernetes when using the exec array form directly without a shell. This is likely causing the CI timeouts with "todolist container stuck in PodInitializing".tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml (1)
116-120: Startup probe environment variable expansion issue - already flagged.Same issue as noted in the previous review comment. The exec form without a shell wrapper won't expand environment variables, causing authentication failures.
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml (1)
159-163: Startup probe environment variable expansion issue - already flagged.Same issue as noted in previous review comments for the other mongo-persistent manifests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
tests/e2e/backup_restore_suite_test.go(11 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml(4 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml(4 hunks)tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml(4 hunks)tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml(3 hunks)tests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml(3 hunks)tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yamltests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yamltests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yamltests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yamltests/e2e/sample-applications/mysql-persistent/mysql-persistent.yamltests/e2e/backup_restore_suite_test.gotests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml
🧬 Code graph analysis (1)
tests/e2e/backup_restore_suite_test.go (1)
tests/e2e/lib/apps.go (5)
IsDCReady(249-288)IsDeploymentReady(290-311)AreApplicationPodsRunning(337-366)DoesSCCExist(139-145)VerifyBackupRestoreData(432-548)
🪛 Checkov (3.2.334)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml
[medium] 163-199: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml
[medium] 145-188: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml
[medium] 154-190: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml
[medium] 193-229: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml
[medium] 167-203: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml
[medium] 150-186: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
🪛 golangci-lint (2.5.0)
tests/e2e/backup_restore_suite_test.go
[error] 46-46: verifyApplicationReady - appName always receives "todolist"
(unparam)
🔇 Additional comments (12)
tests/e2e/sample-applications/mongo-persistent/mongo-persistent.yaml (1)
163-188: Migration to DeploymentConfig looks correct.The conversion from
apps/v1 Deploymenttoapps.openshift.io/v1 DeploymentConfigwith flat selectors and updated image tag aligns with the OpenShift-centric test patterns across this PR.tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml (1)
150-175: DeploymentConfig migration is consistent.Changes align with the broader OpenShift migration pattern across sample applications.
tests/e2e/sample-applications/mongo-persistent/mongo-persistent-block.yaml (1)
193-218: DeploymentConfig migration is consistent.Changes align with the broader OpenShift migration pattern.
tests/e2e/sample-applications/mysql-persistent/mysql-persistent-twovol-csi.yaml (2)
99-104: livenessProbe change to tcpSocket is appropriate.Using
tcpSocketfor liveness is simpler and avoids shell/env-var expansion issues. Good simplification.
145-158: DeploymentConfig migration is consistent.The conversion to
apps.openshift.io/v1 DeploymentConfigwith flat selectors follows the established pattern.tests/e2e/sample-applications/mysql-persistent/mysql-persistent.yaml (2)
120-125: livenessProbe simplification looks good.Switching to
tcpSocketprobe with reducedinitialDelaySecondsis reasonable for this test application.
167-180: DeploymentConfig migration is consistent.Changes follow the established OpenShift DeploymentConfig pattern.
tests/e2e/backup_restore_suite_test.go (4)
46-69: Well-structured refactor for conditional DC/Deployment readiness checks.The
verifyApplicationReadyfunction provides good flexibility with theisDCparameter to conditionally check DeploymentConfig vs Deployment readiness. The optional SCC verification whensccName != ""is also a clean approach.
49-55: Verify workloadName and isDC alignment with migrated manifests.The sample application YAMLs in this PR migrate the
todolistworkload fromDeploymenttoDeploymentConfig. However, all test entries passisDC=falsewithworkloadNameset to"mysql"or"mongo"(the database workload, which remains a Deployment).This appears intentional—the DC readiness check targets the specified
workloadName, not the todolist app. Please confirm that thetodolistDeploymentConfig readiness is adequately covered byAreApplicationPodsRunningat line 56, or whether a separate DC readiness check is needed fortodolist.
316-316: Minor logging improvement.Using
strings.ToUpperfor the report state improves log readability.
402-493: Test entries updated consistently.All backup/restore test cases are updated to use
verifyApplicationReadywith appropriate parameters for app name, route, workload, and SCC verification.tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml (1)
53-136: No changes needed. The mixed use of Deployment and DeploymentConfig in the manifest is intentional and consistent across sample applications: database services useapps/v1 Deploymentwhile application services useapps.openshift.io/v1 DeploymentConfig. This represents an architectural distinction between infrastructure and application layers, not an incomplete conversion.Likely an incorrect or invalid review comment.
| livenessProbe: | ||
| exec: | ||
| command: | ||
| - /usr/bin/timeout | ||
| - 1s | ||
| - /usr/bin/mysql | ||
| - $(MYSQL_DATABASE) | ||
| - -h | ||
| - 127.0.0.1 | ||
| - -u$(MYSQL_USER) | ||
| - -p$(MYSQL_PASSWORD) | ||
| - -e | ||
| - SELECT 1 | ||
| tcpSocket: | ||
| port: mysql | ||
| initialDelaySeconds: 30 | ||
| periodSeconds: 10 | ||
| timeoutSeconds: 5 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml | head -150Repository: openshift/oadp-operator
Length of output: 5018
🏁 Script executed:
wc -l tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yamlRepository: openshift/oadp-operator
Length of output: 143
🏁 Script executed:
cat -n tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml | sed -n '107,135p'Repository: openshift/oadp-operator
Length of output: 1204
🏁 Script executed:
cd tests/e2e/sample-applications/mysql-persistent && git log --oneline -20 mysql-persistent-csi.yamlRepository: openshift/oadp-operator
Length of output: 120
🏁 Script executed:
cd tests/e2e/sample-applications/mysql-persistent && git show 17974d5:mysql-persistent-csi.yaml | grep -A 25 "livenessProbe:" | head -30Repository: openshift/oadp-operator
Length of output: 342
🏁 Script executed:
git show 17974d5:tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml | grep -A 25 "livenessProbe:" | head -30Repository: openshift/oadp-operator
Length of output: 918
🏁 Script executed:
git log --all --oneline -- tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml | head -20Repository: openshift/oadp-operator
Length of output: 821
🏁 Script executed:
git show 02a9165:tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml | grep -A 25 "livenessProbe:" | head -30Repository: openshift/oadp-operator
Length of output: 890
🏁 Script executed:
git show 02a9165:tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml | grep -A 30 "readinessProbe:"Repository: openshift/oadp-operator
Length of output: 1885
🏁 Script executed:
git show 17974d5 --stat | head -30Repository: openshift/oadp-operator
Length of output: 1747
🏁 Script executed:
git diff 02a9165 17974d5 -- tests/e2e/sample-applications/mysql-persistent/mysql-persistent-csi.yaml | grep -A 30 "livenessProbe:"Repository: openshift/oadp-operator
Length of output: 1406
Remove tcpSocket liveness probe or restore exec-based health checks.
The livenessProbe was changed from an exec-based MySQL query to a simple tcpSocket check, and the readinessProbe was removed entirely. This degrades health monitoring: tcpSocket only verifies that port 3306 is listening, not whether MySQL is actually functional.
While the startupProbe (lines 113-129) performs a thorough MySQL query check, it runs only during pod startup. After the container is started, only livenessProbe remains, and tcpSocket cannot detect MySQL process hangs, database corruption, or connection pool issues. The missing readinessProbe means the pod won't be taken out of rotation if it becomes unready after startup.
Recommend restoring the exec-based probes or documenting the rationale for this trade-off.
|
@weshayutin: 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
How to test the changes made
run CI