Skip to content

OCPBUGS-98983: improve guest cluster state management reliability #9198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Aug 4, 2026
Merged

OCPBUGS-98983: improve guest cluster state management reliability #9198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 50 additions & 36 deletions test/e2e/v2/cmd/create-guests/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,17 +132,37 @@ func loadEnvConfig() envConfig {
func run(ctx context.Context, cfg envConfig) error {
specs := cfg.platform.ClusterSpecs(cfg.releaseImage, cfg.n1Image)

// Derive cluster names and build the name map.
// Phase 0: The manifest must exist before any infra is provisioned so
// destroy-guests can always clean up, even if create-guests fails
// before the HostedCluster CR is applied.
log.Println("Phase 0: Computing cluster identities and writing manifest")
named := make([]namedSpec, len(specs))
clusterNames := make(map[string]string) // outputFile -> name
clusterNames := make(map[string]string) // variant -> name
entries := make([]lifecycle.ClusterEntry, len(specs))
for i, spec := range specs {
name := lifecycle.DeriveClusterName(cfg.prowJobID, spec.Variant)
named[i] = namedSpec{ClusterSpec: spec, name: name}
clusterNames[spec.OutputFile] = name
clusterNames[spec.Variant] = name
entries[i] = lifecycle.ClusterEntry{
Variant: spec.Variant,
Name: name,
InfraID: name,
Namespace: cfg.namespace,
}
log.Printf("Cluster %s: name=%s", spec.Variant, name)
}

manifest := &lifecycle.ClusterManifest{
Clusters: entries,
}
if err := lifecycle.WriteManifest(cfg.sharedDir, manifest); err != nil {
return fmt.Errorf("writing cluster manifest: %w", err)
}
log.Printf("Wrote cluster manifest to %s/%s", cfg.sharedDir, lifecycle.ManifestFileName)

// Phase 0: Platform-specific pre-create hooks (e.g., deploy OIDC providers).
log.Println("Phase 0: Running platform pre-create hooks")
// Phase 1: Some platforms need prerequisites deployed before clusters
// exist (e.g., OIDC providers that the HC spec references).
log.Println("Phase 1: Running platform pre-create hooks")
mgmtClientPre, err := newMgmtClient()
if err != nil {
return fmt.Errorf("creating management cluster client for pre-create: %w", err)
Expand All @@ -151,8 +171,9 @@ func run(ctx context.Context, cfg envConfig) error {
return fmt.Errorf("platform pre-create hook: %w", err)
}

// Phase 1: Create all clusters in parallel.
log.Printf("Phase 1: Creating %d clusters in parallel", len(named))
// Phase 2: Clusters are independent; creating them in parallel cuts
// wall-clock time proportionally.
log.Printf("Phase 2: Creating %d clusters in parallel", len(named))
createErrors := createClustersParallel(ctx, cfg, named)
for _, ns := range named {
if err := createErrors[ns.Variant]; err != nil {
Expand All @@ -167,8 +188,9 @@ func run(ctx context.Context, cfg envConfig) error {
}
}

// Phase 2: Platform-specific post-create hooks.
log.Println("Phase 2: Running platform post-create hooks")
// Phase 3: Some platforms need configuration applied after the HC CR
// exists but before it becomes Available (e.g., patching OperatorConfiguration).
log.Println("Phase 3: Running platform post-create hooks")
mgmtClient, err := newMgmtClient()
if err != nil {
return fmt.Errorf("creating management cluster client: %w", err)
Expand All @@ -177,9 +199,9 @@ func run(ctx context.Context, cfg envConfig) error {
return fmt.Errorf("platform post-create hook: %w", err)
}

// Phase 3: Watch for Available condition on all clusters.
log.Println("Phase 3: Waiting for all clusters to become Available")
// Use cfg.waitTimeout (45m) to match the version rollout timeout at line 352.
// Phase 4: Control plane components must exist before post-available
// hooks can run; Available guarantees that.
log.Println("Phase 4: Waiting for all clusters to become Available")
availableErrors := waitForClustersAvailable(ctx, mgmtClient, cfg.namespace, named, cfg.waitTimeout)
for _, ns := range named {
if err := availableErrors[ns.Variant]; err != nil {
Expand All @@ -194,15 +216,16 @@ func run(ctx context.Context, cfg envConfig) error {
}
}

// Phase 4: Platform-specific post-available hooks (e.g., waiting for
// day-2 config transitions now that control plane components exist).
log.Println("Phase 4: Running platform post-available hooks")
// Phase 5: Some platforms need to wait for day-2 config transitions
// that depend on control plane components existing.
log.Println("Phase 5: Running platform post-available hooks")
if err := cfg.platform.PostAvailable(ctx, mgmtClient, cfg.namespace, clusterNames); err != nil {
return fmt.Errorf("platform post-available hook: %w", err)
}

// Phase 5: Watch for version rollout completion on all clusters.
log.Println("Phase 5: Waiting for version rollout completion on all clusters")
// Phase 6: Tests assume rollout is complete; block until all version
// history entries reach CompletedUpdate.
log.Println("Phase 6: Waiting for version rollout completion on all clusters")
rolloutErrors := waitForVersionRollout(ctx, mgmtClient, cfg, named)
anyRolloutFailed := false
for _, ns := range named {
Expand All @@ -216,23 +239,13 @@ func run(ctx context.Context, cfg envConfig) error {
}
}

// Phase 6: Day-2 operations that disrupt ClusterOperators (e.g., External OIDC).
// These run after VersionState=Completed so the initial rollout isn't blocked.
log.Println("Phase 6: Running platform post-version-rollout hooks (day-2 operations)")
// Phase 7: Day-2 operations that disrupt ClusterOperators (e.g., External OIDC)
// run after rollout so they don't block the initial version completion.
log.Println("Phase 7: Running platform post-version-rollout hooks (day-2 operations)")
if err := cfg.platform.PostVersionRollout(ctx, mgmtClient, cfg.namespace, clusterNames); err != nil {
return fmt.Errorf("platform post-version-rollout hook: %w", err)
}

// Phase 7: Write cluster names to SHARED_DIR.
log.Println("Phase 7: Writing cluster names to SHARED_DIR")
for _, ns := range named {
outputPath := filepath.Join(cfg.sharedDir, ns.OutputFile)
if err := os.WriteFile(outputPath, []byte(ns.name), 0600); err != nil {
return fmt.Errorf("writing cluster name to %s: %w", outputPath, err)
}
log.Printf("Wrote cluster name %q to %s", ns.name, outputPath)
}

if anyRolloutFailed {
return fmt.Errorf("one or more cluster version rollouts failed")
}
Expand All @@ -242,16 +255,17 @@ func run(ctx context.Context, cfg envConfig) error {
}

// buildCreateArgs returns CLI arguments for creating a cluster.
func buildCreateArgs(cfg envConfig, name string, spec lifecycle.ClusterSpec) []string {
func buildCreateArgs(cfg envConfig, ns namedSpec) []string {
releaseImage := cfg.releaseImage
if spec.ReleaseImage != "" {
releaseImage = spec.ReleaseImage
if ns.ReleaseImage != "" {
releaseImage = ns.ReleaseImage
}

args := []string{
"create", "cluster", cfg.platform.Name(),
"--name=" + name,
"--name=" + ns.name,
"--namespace=" + cfg.namespace,
"--infra-id=" + ns.name,
"--node-pool-replicas=" + strconv.Itoa(cfg.nodeCount),
"--base-domain=" + cfg.baseDomain,
"--pull-secret=" + cfg.pullSecret,
Expand All @@ -267,7 +281,7 @@ func buildCreateArgs(cfg envConfig, name string, spec lifecycle.ClusterSpec) []s
}

args = append(args, cfg.platform.CreateArgs()...)
args = append(args, spec.ExtraArgs...)
args = append(args, ns.ExtraArgs...)

return args
}
Expand All @@ -286,7 +300,7 @@ func createClustersParallel(ctx context.Context, cfg envConfig, specs []namedSpe
wg.Add(1)
go func() {
defer wg.Done()
args := buildCreateArgs(cfg, ns.name, ns.ClusterSpec)
args := buildCreateArgs(cfg, ns)
log.Printf("Creating %s cluster %s", ns.Variant, ns.name)
log.Printf("Running: %s %v", cfg.hypershiftBinary, args)

Expand Down
51 changes: 23 additions & 28 deletions test/e2e/v2/cmd/destroy-guests/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@ limitations under the License.
*/

// destroy-guests destroys all HostedClusters created by the v2 e2e
// lifecycle tests. Cluster names are re-derived from PROW_JOB_ID
// using the same sha256 hash logic as the create step. All clusters
// are destroyed in parallel with best-effort semantics.
// Platform selection is controlled by the HYPERSHIFT_PLATFORM
// environment variable (default: "azure").
// lifecycle tests. Cluster identities are read from the cluster
// manifest written by create-guests to SHARED_DIR. Platform-specific
// destroy flags come from PlatformConfig.DestroyArgs().
// All clusters are destroyed in parallel with best-effort semantics.
package main

import (
Expand All @@ -35,12 +34,15 @@ import (
const clusterGracePeriod = "40m"

func main() {
prowJobID := os.Getenv("PROW_JOB_ID")
if prowJobID == "" {
log.Fatal("PROW_JOB_ID is required")
sharedDir := os.Getenv("SHARED_DIR")
if sharedDir == "" {
log.Fatal("SHARED_DIR is required")
}

sharedDir := os.Getenv("SHARED_DIR")
manifest, err := lifecycle.ReadManifest(sharedDir)
if err != nil {
log.Fatalf("Failed to read cluster manifest: %v", err)

@csrwng csrwng Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@ironcladlou ironcladlou Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

log.Fatalf exits after logging

}

platform, err := lifecycle.NewPlatformConfig(os.Getenv("HYPERSHIFT_PLATFORM"), sharedDir)
if err != nil {
Expand All @@ -52,29 +54,21 @@ func main() {
hypershiftBin = "hypershift"
}

namespace := os.Getenv("HYPERSHIFT_NAMESPACE")
if namespace == "" {
namespace = "clusters"
}

specs := platform.ClusterSpecs("", "")

log.Printf("Destroying %d clusters derived from PROW_JOB_ID=%s", len(specs), prowJobID)
log.Printf("Destroying %d clusters from manifest", len(manifest.Clusters))

var (
mu sync.Mutex
failed bool
wg sync.WaitGroup
)

for _, spec := range specs {
clusterName := lifecycle.DeriveClusterName(prowJobID, spec.Variant)
for _, entry := range manifest.Clusters {
wg.Add(1)
go func() {
defer wg.Done()
if err := destroyCluster(hypershiftBin, clusterName, namespace, spec.Variant, platform); err != nil {
log.Printf("WARNING: Failed to destroy cluster %s (%s): %v", clusterName, spec.Variant, err)
log.Printf("ACTION REQUIRED: cloud resources for cluster %s may be orphaned and need manual cleanup (resource group, DNS records, etc.)", clusterName)
if err := destroyCluster(hypershiftBin, entry, platform); err != nil {
log.Printf("WARNING: Failed to destroy cluster %s (%s): %v", entry.Name, entry.Variant, err)
log.Printf("ACTION REQUIRED: cloud resources for cluster %s (infraID=%s) may be orphaned and need manual cleanup", entry.Name, entry.InfraID)
mu.Lock()
failed = true
mu.Unlock()
Expand All @@ -90,13 +84,14 @@ func main() {
log.Printf("All clusters destroyed successfully")
}

func destroyCluster(hypershiftBin, name, namespace, variant string, platform lifecycle.PlatformConfig) error {
log.Printf("Destroying cluster %s (%s)", name, variant)
func destroyCluster(hypershiftBin string, entry lifecycle.ClusterEntry, platform lifecycle.PlatformConfig) error {
log.Printf("Destroying cluster %s (%s, infraID=%s)", entry.Name, entry.Variant, entry.InfraID)

args := []string{
"destroy", "cluster", platform.Name(),
"--name=" + name,
"--namespace=" + namespace,
"--name=" + entry.Name,
"--namespace=" + entry.Namespace,
"--infra-id=" + entry.InfraID,
"--cluster-grace-period=" + clusterGracePeriod,
}
args = append(args, platform.DestroyArgs()...)
Expand All @@ -107,9 +102,9 @@ func destroyCluster(hypershiftBin, name, namespace, variant string, platform lif
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
return fmt.Errorf("hypershift destroy cluster %s failed for %s: %w", platform.Name(), name, err)
return fmt.Errorf("hypershift destroy cluster %s failed for %s: %w", platform.Name(), entry.Name, err)
}

log.Printf("Finished destroying cluster: %s", name)
log.Printf("Finished destroying cluster: %s", entry.Name)
return nil
}
32 changes: 11 additions & 21 deletions test/e2e/v2/cmd/dump-guests/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@ limitations under the License.
*/

// dump-guests collects diagnostic artifacts from all v2 e2e
// HostedClusters in parallel. It shells out to the hypershift CLI
// for each cluster and always exits 0 so that dump failures never
// block teardown.
// Platform selection is controlled by the HYPERSHIFT_PLATFORM
// environment variable (default: "azure").
// HostedClusters in parallel. Cluster identities are read from
// the cluster manifest written by create-guests to SHARED_DIR.
// It always exits 0 so that dump failures never block teardown.
package main

import (
Expand All @@ -37,36 +35,28 @@ func main() {
hypershiftBinary := flag.String("hypershift-binary", "hypershift", "Path to the hypershift CLI binary")
flag.Parse()

prowJobID := os.Getenv("PROW_JOB_ID")
if prowJobID == "" {
log.Fatal("PROW_JOB_ID environment variable is required")
sharedDir := os.Getenv("SHARED_DIR")
if sharedDir == "" {
log.Fatal("SHARED_DIR environment variable is required")
}
artifactDir := os.Getenv("ARTIFACT_DIR")
if artifactDir == "" {
log.Fatal("ARTIFACT_DIR environment variable is required")
}

sharedDir := os.Getenv("SHARED_DIR")
platform, err := lifecycle.NewPlatformConfig(os.Getenv("HYPERSHIFT_PLATFORM"), sharedDir)
manifest, err := lifecycle.ReadManifest(sharedDir)
if err != nil {
log.Fatalf("Failed to initialize platform config: %v", err)
}

namespace := os.Getenv("HYPERSHIFT_NAMESPACE")
if namespace == "" {
namespace = "clusters"
log.Fatalf("Failed to read cluster manifest: %v", err)

@csrwng csrwng Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as with destroy guests. If you can't read the manifest, just log the error and exit.

}

specs := platform.ClusterSpecs("", "")
log.Printf("Dumping %d clusters derived from PROW_JOB_ID=%s", len(specs), prowJobID)
log.Printf("Dumping %d clusters from manifest", len(manifest.Clusters))

var wg sync.WaitGroup
for _, spec := range specs {
clusterName := lifecycle.DeriveClusterName(prowJobID, spec.Variant)
for _, entry := range manifest.Clusters {
wg.Add(1)
go func() {
defer wg.Done()
dumpCluster(*hypershiftBinary, artifactDir, clusterName, namespace)
dumpCluster(*hypershiftBinary, artifactDir, entry.Name, entry.Namespace)
}()
}
wg.Wait()
Expand Down
Loading