OCPBUGS-98983: improve guest cluster state management reliability - #9198
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@ironcladlou: This pull request references Jira Issue OCPBUGS-98983, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
/pipeline help |
📝 WalkthroughWalkthroughThe e2e v2 lifecycle now writes cluster identities to a shared JSON manifest before provisioning. Platform specifications and test matrices identify clusters by variants. Test execution resolves variants through the manifest. Dump and destroy commands use manifest entries instead of derived names, cluster files, and shared namespace settings. Manifest read/write, lookup, and variant resolution behavior has new tests. Sequence Diagram(s)sequenceDiagram
participant create_guests
participant lifecycle_manifest
participant run_tests
participant destroy_guests
create_guests->>lifecycle_manifest: Write cluster manifest before provisioning
run_tests->>lifecycle_manifest: Read manifest and resolve test variants
lifecycle_manifest-->>run_tests: Return cluster name and namespace
destroy_guests->>lifecycle_manifest: Read manifest for cleanup identities
lifecycle_manifest-->>destroy_guests: Return name, namespace, and infrastructure ID
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/platform/aws
|
/test ? |
|
/test e2e-v2-aws |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/v2/cmd/destroy-guests/main.go (1)
92-116: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd context/timeout for external
hypershiftcommands in destroy and dump helpers.destroyClusteranddumpClustercallexec.Command.Run()directly and can block forever ifhypershifthangs, while the parallel goroutines wait onwg.Wait(). Use a boundedcontext.Contextandexec.CommandContextfor thehypershift destroy clusterandhypershift dump clustercalls.🤖 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/v2/cmd/destroy-guests/main.go` around lines 92 - 116, The external hypershift invocations can hang indefinitely, blocking parallel cleanup completion. In test/e2e/v2/cmd/destroy-guests/main.go:92-116, update destroyCluster to create a bounded context and use exec.CommandContext for the destroy command; in test/e2e/v2/cmd/dump-guests/main.go:67-94, apply the same context timeout and CommandContext pattern to dumpCluster, preserving the existing command arguments and error reporting.Source: Path instructions
🤖 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.
Outside diff comments:
In `@test/e2e/v2/cmd/destroy-guests/main.go`:
- Around line 92-116: The external hypershift invocations can hang indefinitely,
blocking parallel cleanup completion. In
test/e2e/v2/cmd/destroy-guests/main.go:92-116, update destroyCluster to create a
bounded context and use exec.CommandContext for the destroy command; in
test/e2e/v2/cmd/dump-guests/main.go:67-94, apply the same context timeout and
CommandContext pattern to dumpCluster, preserving the existing command arguments
and error reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c535fb89-ce9c-4496-8fc0-bbb0bbf843e0
📒 Files selected for processing (9)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/aws.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/lifecycle/manifest.gotest/e2e/v2/lifecycle/manifest_test.gotest/e2e/v2/lifecycle/platform.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9198 +/- ##
=======================================
Coverage 44.94% 44.94%
=======================================
Files 778 778
Lines 97427 97427
=======================================
Hits 43790 43790
Misses 50616 50616
Partials 3021 3021
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
f33df5f to
3906fb9
Compare
|
/pipeline required |
|
Scheduling tests matching the |
openshift-ci-robot
added
jira/valid-bug
|
@ironcladlou: This pull request references Jira Issue OCPBUGS-98983, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/v2/lifecycle/manifest.go (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported manifest API.
ManifestFileName,WriteManifest, andReadManifesthave no Go doc comments.LookupClusterdocuments only the success case. Document unknown-variant errors and nil-receiver behavior.As per path instructions, “Keep exported manifest APIs documented with actual behavior, including panic/error conditions.”
Also applies to: 30-30, 42-42, 55-56
🤖 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/v2/lifecycle/manifest.go` at line 12, Document the exported manifest API symbols ManifestFileName, WriteManifest, ReadManifest, and LookupCluster with Go doc comments describing their actual behavior. Include LookupCluster’s unknown-variant error behavior and nil-receiver behavior, plus any panic or error conditions for the other exported APIs; keep the documentation adjacent to each declaration.Source: Path instructions
🤖 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/e2e/v2/lifecycle/manifest.go`:
- Around line 30-36: Add a shared manifest validator covering non-nil manifests,
required identity fields, and non-empty unique variants; invoke it in
WriteManifest before serialization and in ReadManifest after deserialization.
Update LookupCluster to nil-check the manifest and return contextual errors for
invalid or duplicate variants instead of selecting the first match. Add tests
covering missing identity or variants, duplicate variants, serialization
validation, and lookup failures.
---
Nitpick comments:
In `@test/e2e/v2/lifecycle/manifest.go`:
- Line 12: Document the exported manifest API symbols ManifestFileName,
WriteManifest, ReadManifest, and LookupCluster with Go doc comments describing
their actual behavior. Include LookupCluster’s unknown-variant error behavior
and nil-receiver behavior, plus any panic or error conditions for the other
exported APIs; keep the documentation adjacent to each declaration.
🪄 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: Pro Plus
Run ID: eaf0f3e3-8039-46ca-9cb6-c140511e9c7e
📒 Files selected for processing (9)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/aws.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/lifecycle/manifest.gotest/e2e/v2/lifecycle/manifest_test.gotest/e2e/v2/lifecycle/platform.go
🚧 Files skipped from review as they are similar to previous changes (8)
- test/e2e/v2/cmd/run-tests/main.go
- test/e2e/v2/lifecycle/manifest_test.go
- test/e2e/v2/cmd/destroy-guests/main.go
- test/e2e/v2/lifecycle/aws.go
- test/e2e/v2/lifecycle/platform.go
- test/e2e/v2/lifecycle/azure.go
- test/e2e/v2/cmd/dump-guests/main.go
- test/e2e/v2/cmd/create-guests/main.go
| func WriteManifest(dir string, m *ClusterManifest) error { | ||
| data, err := json.MarshalIndent(m, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("marshaling cluster manifest: %w", err) | ||
| } | ||
| path := filepath.Join(dir, ManifestFileName) | ||
| if err := os.WriteFile(path, data, 0600); err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject incomplete and ambiguous manifests before resolution.
WriteManifest accepts a nil manifest and serializes it as null. ReadManifest accepts missing identity fields and duplicate Variant values. LookupCluster returns the first duplicate and dereferences m without a nil check. This can select the wrong Name, InfraID, or Namespace, or prevent cleanup from using a valid infrastructure ID.
Add a shared validator for non-nil manifests, required identity fields, and unique non-empty variants. Call it before writing and after reading. Make LookupCluster return contextual errors for nil manifests and duplicate variants. Add tests for missing and duplicate variants.
Suggested validation hooks
func WriteManifest(dir string, m *ClusterManifest) error {
+ if err := validateManifest(m); err != nil {
+ return fmt.Errorf("validating cluster manifest before write: %w", err)
+ }
data, err := json.MarshalIndent(m, "", " ")
func ReadManifest(dir string) (*ClusterManifest, error) {
...
if err := json.Unmarshal(data, &m); err != nil {
return nil, fmt.Errorf("unmarshaling cluster manifest: %w", err)
}
+ if err := validateManifest(&m); err != nil {
+ return nil, fmt.Errorf("validating cluster manifest from %s: %w", path, err)
+ }
func (m *ClusterManifest) LookupCluster(variant string) (ClusterEntry, error) {
+ if m == nil {
+ return ClusterEntry{}, fmt.Errorf("looking up cluster variant %q: manifest is nil", variant)
+ }
...
}As per path instructions, “manifest and variant resolution failures fail loudly,” “always nil-check pointers before dereferencing,” and “add or update tests for serialization, missing/duplicate variants, and lookup failures.” Based on the supplied lifecycle consumer and PR objective, LookupCluster participates in variant resolution and the manifest is required for downstream cleanup.
Also applies to: 42-52, 55-62
🤖 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/v2/lifecycle/manifest.go` around lines 30 - 36, Add a shared
manifest validator covering non-nil manifests, required identity fields, and
non-empty unique variants; invoke it in WriteManifest before serialization and
in ReadManifest after deserialization. Update LookupCluster to nil-check the
manifest and return contextual errors for invalid or duplicate variants instead
of selecting the first match. Add tests covering missing identity or variants,
duplicate variants, serialization validation, and lookup failures.
Source: Path instructions
Test Resultse2e-aws
e2e-aks
|
Before this commit, only guest cluster names were tracked, and infra IDs were auto generated and not persisted outside the hostedcluster. So, if infra setup fails and the hostedcluster is never persisted, downstream steps can no longer identify the infra for cleanup, causing destroy-guests to fail and infra to be leaked. This commit solves the problem and also makes the overall design more predictable and flexible by introducing a `ClusterManifest` that captures the identity of a guest cluster before any infra is provisioned, serving as an up-front declaration of intent that downstream steps consume. This eliminates the old cluster name files entirely and provides a more stable and robust interface for inter-process communication between steps. Guests can now be reliably destroyed in the absence of a hostedcluster.
3906fb9 to
5b6a3a7
Compare
|
@ironcladlou: This pull request references Jira Issue OCPBUGS-98983, which is valid. 3 validation(s) were run on this bug
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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/v2/cmd/destroy-guests/main.go (1)
87-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd cancellation and a timeout to each destroy command.
Pass a
context.ContextintodestroyCluster, create a bounded context inmain, and useexec.CommandContext. A hunghypershift destroyprocess can block the goroutine andwaitGroup.Wait(), leaving cleanup without cancellation.🤖 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/v2/cmd/destroy-guests/main.go` around lines 87 - 108, Update destroyCluster to accept a context.Context and create commands with exec.CommandContext so cancellation terminates each hypershift destroy process. In main, derive a bounded timeout context for cleanup and pass it through every destroyCluster invocation, ensuring the existing waitGroup flow can exit when the timeout or parent cancellation occurs.Source: Path instructions
🤖 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.
Outside diff comments:
In `@test/e2e/v2/cmd/destroy-guests/main.go`:
- Around line 87-108: Update destroyCluster to accept a context.Context and
create commands with exec.CommandContext so cancellation terminates each
hypershift destroy process. In main, derive a bounded timeout context for
cleanup and pass it through every destroyCluster invocation, ensuring the
existing waitGroup flow can exit when the timeout or parent cancellation occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e1e67253-9f4e-43f3-b1c8-b167215fa484
📒 Files selected for processing (9)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/aws.gotest/e2e/v2/lifecycle/azure.gotest/e2e/v2/lifecycle/manifest.gotest/e2e/v2/lifecycle/manifest_test.gotest/e2e/v2/lifecycle/platform.go
🚧 Files skipped from review as they are similar to previous changes (7)
- test/e2e/v2/cmd/dump-guests/main.go
- test/e2e/v2/lifecycle/azure.go
- test/e2e/v2/lifecycle/aws.go
- test/e2e/v2/cmd/run-tests/main.go
- test/e2e/v2/lifecycle/platform.go
- test/e2e/v2/lifecycle/manifest.go
- test/e2e/v2/lifecycle/manifest_test.go
|
/pipeline required |
|
Scheduling tests matching the |
ironcladlou
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-aws |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/verified by e2e regression analysis |
|
@ironcladlou: No Jira issue with key OCPBUGS-98983 exists in the tracker at https://redhat.atlassian.net. 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. |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
A couple of nits, otherwise lgtm
| sharedDir := os.Getenv("SHARED_DIR") | ||
| manifest, err := lifecycle.ReadManifest(sharedDir) | ||
| if err != nil { | ||
| log.Fatalf("Failed to read cluster manifest: %v", err) |
There was a problem hiding this comment.
Consider logging an error and just exiting here. If for some reason the step to create the manifest failed, this shouldn't result in further failures.
There was a problem hiding this comment.
log.Fatalf exits after logging
| namespace := os.Getenv("HYPERSHIFT_NAMESPACE") | ||
| if namespace == "" { | ||
| namespace = "clusters" | ||
| log.Fatalf("Failed to read cluster manifest: %v", err) |
There was a problem hiding this comment.
Same as with destroy guests. If you can't read the manifest, just log the error and exit.
|
/override ci/prow/e2e-kubevirt-aws-ovn-reduced |
csrwng
added
the
approved
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/e2e-kubevirt-aws-ovn-reduced 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: ironcladlou 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 |
|
/verified by e2e regression analysis |
openshift-ci-robot
added
the
verified
|
@ironcladlou: This PR has been marked as verified by 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. |
|
/verified by e2e |
|
@csrwng: This PR has been marked as verified by 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. |
|
@ironcladlou: 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. |
|
@ironcladlou: Jira Issue OCPBUGS-98983: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-98983 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
6 participants
Before this commit, only guest cluster names were tracked, and infra IDs were auto generated and not persisted outside the hostedcluster. So, if infra setup fails and the hostedcluster is never persisted, downstream steps can no longer identify the infra for cleanup, causing destroy-guests to fail and infra to be leaked.
This commit solves the problem and also makes the overall design more predictable and flexible by introducing a
ClusterManifestthat captures the identity of a guest cluster before any infra is provisioned, serving as an up-front declaration of intent that downstream steps consume.This eliminates the old cluster name files entirely and provides a more stable and robust interface for inter-process communication between steps. Guests can now be reliably destroyed in the absence of a hostedcluster.
Summary by CodeRabbit
New Features
Bug Fixes
Tests