[WIP] CORENET-7073: Run OTE tests against KIND cluster#3254
[WIP] CORENET-7073: Run OTE tests against KIND cluster#3254pperiyasamy wants to merge 2 commits into
Conversation
Enables both local and SSH based command execution by accepting Runner as a parameter instead of creating it internally. TODO: Consume this change from an upstream commit. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
Enables OTE binary to run E2E tests against kind cluster via SSH. Uses KIND_HOST, KIND_HOST_SSH_KEY and CONTAINER_RUNTIME environment variables for remote host connectivity. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@pperiyasamy: This pull request references CORENET-7073 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 "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. |
|
cc @jluhrsen |
Walkthrough
ChangesKind Provider DI and OpenShift SSH Init
Sequence Diagram(s)sequenceDiagram
participant main as main() / BeforeSuite
participant initInfra as initializeInfraProvider
participant InitializeKindInfra
participant hostSshCmdRunner
participant kindNew as kind.New(ce, runner)
rect rgba(100, 150, 255, 0.5)
Note over main,kindNew: OpenShift test binary (kind cluster)
main->>initInfra: initializeInfraProvider(cfg)
initInfra->>InitializeKindInfra: InitializeKindInfra()
InitializeKindInfra->>hostSshCmdRunner: build SSH runner from KIND_HOST + KIND_HOST_SSH_KEY
hostSshCmdRunner-->>InitializeKindInfra: ssh.Runner
InitializeKindInfra->>kindNew: New(containerRuntimeExec, sshRunner)
kindNew-->>InitializeKindInfra: api.Provider
InitializeKindInfra-->>initInfra: api.Provider
initInfra-->>main: nil (infraprovider set)
end
rect rgba(100, 200, 100, 0.5)
Note over main,kindNew: e2e suite (direct/local kind cluster)
main->>kindNew: New(GetContainerRuntime(), NewDirectRunner())
kindNew-->>main: api.Provider
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pperiyasamy 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.
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)
test/e2e/infraprovider/providers/kind/kind.go (1)
99-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse runner-based context lookup for cluster name in remote mode.
Line 100 calls
kindClusterName(), but Line 142 resolves context via localexec.Command. In SSH mode, preload/load commands run on the remote host while cluster name is discovered locally, which can produce an empty or wrong name and break image preload targeting.💡 Proposed fix
@@ -import ( +import ( "context" "fmt" "os" - "os/exec" "strings" "time" @@ - clusterName := kindClusterName() + clusterName := k.kindClusterName() @@ -func kindClusterName() string { - currentCtx, err := exec.Command("kubectl", "config", "current-context").CombinedOutput() +func (k *kind) kindClusterName() string { + currentCtx, err := k.cmdRunner.Run("kubectl", "config", "current-context") if err != nil { return "" } - ctx := strings.TrimSpace(string(currentCtx)) + ctx := strings.TrimSpace(currentCtx) // KIND contexts are named "kind-<cluster-name>" if strings.HasPrefix(ctx, "kind-") { return strings.TrimPrefix(ctx, "kind-") } return "" }Also applies to: 141-149
🤖 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/e2e/infraprovider/providers/kind/kind.go` around lines 99 - 103, The `PreloadImages` method calls `kindClusterName()` locally to determine the cluster name on line 100, but the image preload commands around lines 141-149 execute on the remote host via exec.Command. In SSH/remote mode, this causes the cluster name to be resolved locally while commands run remotely, producing an empty or incorrect name. Replace the local `kindClusterName()` call with a runner-based context lookup that resolves the cluster name in the same execution context (remote or local) where the preload commands will actually be executed, ensuring consistency between cluster name discovery and command execution.
🤖 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 83-99: The deploymentconfig.Set() call using
ocpdeploymentconfig.New() is currently unconditional and applies to both kind
and OpenShift infrastructure types, but it should be provider-specific. Move the
deployment config initialization inside the conditional blocks: for the kind
provider branch (within the if infraprovider.IsKind() block), call
deploymentconfig.Set() with deploymentkind.New(), and for the OpenShift provider
branch (in the else block after ocpinfraprovider.New()), call
deploymentconfig.Set() with ocpdeploymentconfig.New(). This ensures kind
clusters use kind-specific deployment configuration while OpenShift clusters use
OpenShift-specific deployment configuration.
---
Outside diff comments:
In `@test/e2e/infraprovider/providers/kind/kind.go`:
- Around line 99-103: The `PreloadImages` method calls `kindClusterName()`
locally to determine the cluster name on line 100, but the image preload
commands around lines 141-149 execute on the remote host via exec.Command. In
SSH/remote mode, this causes the cluster name to be resolved locally while
commands run remotely, producing an empty or incorrect name. Replace the local
`kindClusterName()` call with a runner-based context lookup that resolves the
cluster name in the same execution context (remote or local) where the preload
commands will actually be executed, ensuring consistency between cluster name
discovery and command execution.
🪄 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: 3731e960-79d9-4097-b35d-e41e8795fbce
📒 Files selected for processing (5)
openshift/cmd/ovn-kubernetes-tests-ext/main.goopenshift/test/infraprovider/kind.gotest/e2e/e2e_suite_test.gotest/e2e/infraprovider/providers/kind/kind.gotest/e2e/infraprovider/providers/kind/runtime.go
| if infraprovider.IsKind() { | ||
| infra, err := ocpinfraprovider.InitializeKindInfra() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to initialize kind infrastructure: %w", err) | ||
| } | ||
| infraprovider.Set(infra) | ||
| } else { | ||
| infra, err := ocpinfraprovider.New(cfg) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to initialize OpenShift infrastructure: %w", err) | ||
| } | ||
| infraprovider.Set(infra) | ||
| // Set ocpInfra only for OpenShift clusters (not for kind) | ||
| ocpInfra = infra | ||
| } | ||
| deploymentconfig.Set(ocpdeploymentconfig.New()) | ||
| return nil |
There was a problem hiding this comment.
Set deployment config per provider type (kind vs OpenShift).
Line 98 unconditionally applies ocpdeploymentconfig.New(), including the kind branch. That conflicts with the kind test path (see test/e2e/e2e_suite_test.go, Line 74 using deploymentkind.New()), and can route kind runs through OpenShift-specific deployment assumptions.
💡 Proposed fix
@@
import (
"fmt"
"os"
"strings"
@@
"github.com/ovn-kubernetes/ovn-kubernetes/test/e2e/deploymentconfig"
+ deploymentkind "github.com/ovn-kubernetes/ovn-kubernetes/test/e2e/deploymentconfig/configs/kind"
"github.com/ovn-kubernetes/ovn-kubernetes/test/e2e/infraprovider"
@@
func initializeInfraProvider(cfg *rest.Config) error {
if infraprovider.IsKind() {
infra, err := ocpinfraprovider.InitializeKindInfra()
if err != nil {
return fmt.Errorf("failed to initialize kind infrastructure: %w", err)
}
infraprovider.Set(infra)
+ deploymentconfig.Set(deploymentkind.New())
} else {
infra, err := ocpinfraprovider.New(cfg)
if err != nil {
return fmt.Errorf("failed to initialize OpenShift infrastructure: %w", err)
}
infraprovider.Set(infra)
// Set ocpInfra only for OpenShift clusters (not for kind)
ocpInfra = infra
+ deploymentconfig.Set(ocpdeploymentconfig.New())
}
- deploymentconfig.Set(ocpdeploymentconfig.New())
return nil
}📝 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.
| if infraprovider.IsKind() { | |
| infra, err := ocpinfraprovider.InitializeKindInfra() | |
| if err != nil { | |
| return fmt.Errorf("failed to initialize kind infrastructure: %w", err) | |
| } | |
| infraprovider.Set(infra) | |
| } else { | |
| infra, err := ocpinfraprovider.New(cfg) | |
| if err != nil { | |
| return fmt.Errorf("failed to initialize OpenShift infrastructure: %w", err) | |
| } | |
| infraprovider.Set(infra) | |
| // Set ocpInfra only for OpenShift clusters (not for kind) | |
| ocpInfra = infra | |
| } | |
| deploymentconfig.Set(ocpdeploymentconfig.New()) | |
| return nil | |
| if infraprovider.IsKind() { | |
| infra, err := ocpinfraprovider.InitializeKindInfra() | |
| if err != nil { | |
| return fmt.Errorf("failed to initialize kind infrastructure: %w", err) | |
| } | |
| infraprovider.Set(infra) | |
| deploymentconfig.Set(deploymentkind.New()) | |
| } else { | |
| infra, err := ocpinfraprovider.New(cfg) | |
| if err != nil { | |
| return fmt.Errorf("failed to initialize OpenShift infrastructure: %w", err) | |
| } | |
| infraprovider.Set(infra) | |
| // Set ocpInfra only for OpenShift clusters (not for kind) | |
| ocpInfra = infra | |
| deploymentconfig.Set(ocpdeploymentconfig.New()) | |
| } | |
| return nil |
🤖 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 83 - 99, The
deploymentconfig.Set() call using ocpdeploymentconfig.New() is currently
unconditional and applies to both kind and OpenShift infrastructure types, but
it should be provider-specific. Move the deployment config initialization inside
the conditional blocks: for the kind provider branch (within the if
infraprovider.IsKind() block), call deploymentconfig.Set() with
deploymentkind.New(), and for the OpenShift provider branch (in the else block
after ocpinfraprovider.New()), call deploymentconfig.Set() with
ocpdeploymentconfig.New(). This ensures kind clusters use kind-specific
deployment configuration while OpenShift clusters use OpenShift-specific
deployment configuration.
|
@pperiyasamy: 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
This PR enables the OTE binary to run E2E tests against kind cluster using SSH-based remote execution. It parameterizes the kind infrastructure provider to accept a
Runnerinterface, allowing commands to be executed either locally (DirectRunner) or remotely via SSH (SSHRunner).TODO: Run a test container that includes the OTE binary, SSH configuration, and necessary environment variables set, E2E tests against a remote kind cluster.
Summary by CodeRabbit
Refactor
New Features