Skip to content

DNM: Test: disable pod restart workaround after file-system restore#2133

Closed
weshayutin wants to merge 1 commit intooadp-devfrom
disable-pod-restart-after-fs-restore
Closed

DNM: Test: disable pod restart workaround after file-system restore#2133
weshayutin wants to merge 1 commit intooadp-devfrom
disable-pod-restart-after-fs-restore

Conversation

@weshayutin
Copy link
Contributor

Summary

  • Comments out the post-restore pod deletion workaround for KOPIA (file-system) backups in both backup_restore_suite_test.go and backup_restore_cli_suite_test.go
  • This workaround was added because OVN-Kubernetes didn't fully wire the network namespace for pods recreated by Velero with a restore-wait init container
  • Goal: determine whether this workaround is still needed by running E2E tests without it

Test plan

  • KOPIA backup/restore E2E tests pass without the pod restart workaround
  • CLI KOPIA backup/restore E2E tests pass without the pod restart workaround
  • If tests fail with networking issues, revert this change and keep the workaround

Made with Cursor

Comment out the KOPIA post-restore pod deletion to test whether the
OVN-Kubernetes networking workaround is still required. If E2E tests
pass without it, this workaround can be removed entirely.

Made-with: Cursor
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 24, 2026

Walkthrough

The post-restore pod restart workaround for KOPIA (file-system) backups has been disabled across two e2e test files by commenting out the corresponding logic blocks and adding TODO comments for future reassessment of the workaround's necessity.

Changes

Cohort / File(s) Summary
E2E Test Pod Restart Workaround Disablement
tests/e2e/backup_restore_cli_suite_test.go, tests/e2e/backup_restore_suite_test.go
Commented out KOPIA-specific post-restore pod deletion/restart logic that previously targeted pods with label selector e2e-app=true; added TODO comments to reassess whether the workaround remains necessary for file-system restore networking.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch disable-pod-restart-after-fs-restore

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci bot requested a review from eemcmullan March 24, 2026 17:44
@openshift-ci
Copy link

openshift-ci bot commented Mar 24, 2026

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/e2e/backup_restore_cli_suite_test.go (1)

3-15: ⚠️ Potential issue | 🔴 Critical

Remove unused imports that cause compilation failure.

After the workaround code at lines 178-194 was commented out, the imports context (line 4) and metav1 (line 12) are no longer used anywhere in the file. Go treats unused imports as a compilation error, so this file will not compile.

Remove both imports:

Fix
 import (
-	"context"
 	"fmt"
 	"log"
 	"strings"
 	"time"
 
 	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/gomega"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/openshift/oadp-operator/tests/e2e/lib"
 )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/backup_restore_cli_suite_test.go` around lines 3 - 15, Remove the
now-unused imports "context" and "metav1" from the import block introduced in
the tests e2e import list (they were only needed for the workaround commented
out at lines ~178-194); edit the import block to delete the symbols context and
metav1 so the file compiles without unused imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tests/e2e/backup_restore_cli_suite_test.go`:
- Around line 3-15: Remove the now-unused imports "context" and "metav1" from
the import block introduced in the tests e2e import list (they were only needed
for the workaround commented out at lines ~178-194); edit the import block to
delete the symbols context and metav1 so the file compiles without unused
imports.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ddc4c80d-432b-4569-9c8e-59b21c8c36c5

📥 Commits

Reviewing files that changed from the base of the PR and between f2f8c5e and a647769.

📒 Files selected for processing (2)
  • tests/e2e/backup_restore_cli_suite_test.go
  • tests/e2e/backup_restore_suite_test.go

@weshayutin weshayutin changed the title Test: disable pod restart workaround after file-system restore DNM: Test: disable pod restart workaround after file-system restore Mar 24, 2026
@openshift-ci
Copy link

openshift-ci bot commented Mar 24, 2026

@weshayutin: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.23-e2e-test-aws a647769 link false /test 4.23-e2e-test-aws
ci/prow/4.22-e2e-test-cli-aws a647769 link true /test 4.22-e2e-test-cli-aws
ci/prow/unit-test a647769 link true /test unit-test
ci/prow/5.0-e2e-test-aws a647769 link true /test 5.0-e2e-test-aws
ci/prow/4.22-e2e-test-aws a647769 link true /test 4.22-e2e-test-aws
ci/prow/5.0-e2e-test-cli-aws a647769 link true /test 5.0-e2e-test-cli-aws

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@kaovilai
Copy link
Member

kaovilai commented Mar 25, 2026

Note

Responses generated with Claude

Closing this PR because it was pushed directly to the protected openshift/oadp-operator branch and cannot be amended to. Reopening from fork branch instead: #2134

@kaovilai kaovilai closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants