Skip to content

CNTRLPLANE-3646: make lifecycle binaries configurable for non-CI environments #9021

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

Closed
ironcladlou wants to merge 1 commit into from
Closed

CNTRLPLANE-3646: make lifecycle binaries configurable for non-CI environments #9021

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
16 changes: 12 additions & 4 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 @@ -62,8 +62,6 @@ func init() {
utilruntime.Must(routev1.AddToScheme(scheme))
}

const defaultNamespace = "clusters"

// envConfig captures the common environment configuration.
type envConfig struct {
prowJobID string
Expand All @@ -79,6 +77,9 @@ type envConfig struct {
etcdSC string
pullSecret string

variants string
infraIDFromName bool

platform lifecycle.PlatformConfig
hypershiftBinary string
waitTimeout time.Duration
Expand Down Expand Up @@ -112,11 +113,14 @@ func loadEnvConfig() envConfig {

baseDomain: envOrDefault("HYPERSHIFT_BASE_DOMAIN", platform.DefaultBaseDomain()),
nodeCount: envOrDefaultInt("HYPERSHIFT_NODE_COUNT", 3),
namespace: envOrDefault("HYPERSHIFT_NAMESPACE", defaultNamespace),
namespace: envOrDefault("HYPERSHIFT_NAMESPACE", lifecycle.DefaultNamespace),
externalDNS: os.Getenv("HYPERSHIFT_EXTERNAL_DNS_DOMAIN"),
etcdSC: os.Getenv("HYPERSHIFT_ETCD_STORAGE_CLASS"),
pullSecret: envOrDefault("PULL_SECRET", "/etc/ci-pull-credentials/.dockerconfigjson"),

variants: os.Getenv("HYPERSHIFT_VARIANTS"),
infraIDFromName: os.Getenv("HYPERSHIFT_INFRA_ID_FROM_NAME") == "true",

Comment thread
coderabbitai[bot] marked this conversation as resolved.
platform: platform,
hypershiftBinary: envOrDefault("HYPERSHIFT_BINARY", "hypershift"),
waitTimeout: 45 * time.Minute,
Expand All @@ -130,7 +134,7 @@ func loadEnvConfig() envConfig {
}

func run(ctx context.Context, cfg envConfig) error {
specs := cfg.platform.ClusterSpecs(cfg.releaseImage, cfg.n1Image)
specs := lifecycle.FilterClusterSpecs(cfg.platform.ClusterSpecs(cfg.releaseImage, cfg.n1Image), cfg.variants)

// Derive cluster names and build the name map.
named := make([]namedSpec, len(specs))
Expand Down Expand Up @@ -251,13 +255,17 @@ func buildCreateArgs(cfg envConfig, name string, spec lifecycle.ClusterSpec) []s
args := []string{
"create", "cluster", cfg.platform.Name(),
"--name=" + name,
"--namespace=" + cfg.namespace,
"--node-pool-replicas=" + strconv.Itoa(cfg.nodeCount),
"--base-domain=" + cfg.baseDomain,
"--pull-secret=" + cfg.pullSecret,
"--release-image=" + releaseImage,
"--generate-ssh",
}

if cfg.infraIDFromName {
args = append(args, "--infra-id="+name)
}
if cfg.externalDNS != "" {
args = append(args, "--external-dns-domain="+cfg.externalDNS)
}
Expand Down
23 changes: 19 additions & 4 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 @@ -52,7 +52,15 @@ func main() {
hypershiftBin = "hypershift"
}

specs := platform.ClusterSpecs("", "")
namespace := os.Getenv("HYPERSHIFT_NAMESPACE")
if namespace == "" {
namespace = lifecycle.DefaultNamespace
}

baseDomain := os.Getenv("HYPERSHIFT_BASE_DOMAIN")
infraIDFromName := os.Getenv("HYPERSHIFT_INFRA_ID_FROM_NAME") == "true"

specs := lifecycle.FilterClusterSpecs(platform.ClusterSpecs("", ""), os.Getenv("HYPERSHIFT_VARIANTS"))
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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

Expand All @@ -67,7 +75,7 @@ func main() {
wg.Add(1)
go func() {
defer wg.Done()
if err := destroyCluster(hypershiftBin, clusterName, spec.Variant, platform); err != nil {
if err := destroyCluster(hypershiftBin, clusterName, namespace, baseDomain, infraIDFromName, 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)
mu.Lock()
Expand All @@ -85,14 +93,21 @@ func main() {
log.Printf("All clusters destroyed successfully")
}

func destroyCluster(hypershiftBin, name, variant string, platform lifecycle.PlatformConfig) error {
log.Printf("Destroying cluster %s (%s)", name, variant)
func destroyCluster(hypershiftBin, name, namespace, baseDomain string, infraIDFromName bool, platform lifecycle.PlatformConfig) error {

@bryan-cox bryan-cox Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: destroyCluster now takes 6 positional params. Three of them (namespace, baseDomain, infraIDFromName) are the same env-derived config that create-guests already bundles into its envConfig struct. A shared config type (or adding these to PlatformConfig) would reduce the parameter list and keep the two binaries in sync.

Not blocking — just noting the data-clump smell before it grows.

@ironcladlou ironcladlou Jul 22, 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.

Seems worth a followup refactor

log.Printf("Destroying cluster %s", name)

args := []string{
"destroy", "cluster", platform.Name(),
"--name=" + name,
"--namespace=" + namespace,
"--cluster-grace-period=" + clusterGracePeriod,
}
if infraIDFromName {
args = append(args, "--infra-id="+name)
}
if baseDomain != "" {
args = append(args, "--base-domain="+baseDomain)
}
args = append(args, platform.DestroyArgs()...)

log.Printf("Running: %s %v", hypershiftBin, args)
Expand Down
9 changes: 7 additions & 2 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 @@ -34,7 +34,11 @@ import (
)

func main() {
hypershiftBinary := flag.String("hypershift-binary", "hypershift", "Path to the hypershift CLI binary")
defaultBinary := "hypershift"
if v := os.Getenv("HYPERSHIFT_BINARY"); v != "" {
defaultBinary = v
}
hypershiftBinary := flag.String("hypershift-binary", defaultBinary, "Path to the hypershift CLI binary")
flag.Parse()

prowJobID := os.Getenv("PROW_JOB_ID")
Expand All @@ -52,7 +56,8 @@ func main() {
log.Fatalf("Failed to initialize platform config: %v", err)
}

specs := platform.ClusterSpecs("", "")
variants := os.Getenv("HYPERSHIFT_VARIANTS")

@bryan-cox bryan-cox Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: The pattern os.Getenv("HYPERSHIFT_VARIANTS") + lifecycle.FilterClusterSpecs(platform.ClusterSpecs(...), variants) is repeated identically in all four cmd/ binaries. Similarly, os.Getenv("HYPERSHIFT_INFRA_ID_FROM_NAME") == "true" appears in both create-guests and destroy-guests.

Fine for now, but if more env vars get added across all four binaries, a shared config loader in lifecycle/ would help.

@ironcladlou ironcladlou Jul 22, 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.

Agree all this stuff needs cleaned up into a structure with uniform env handling applied and passed around

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

var wg sync.WaitGroup
Expand Down
36 changes: 29 additions & 7 deletions test/e2e/v2/cmd/run-tests/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ import (
)

const (
testBinary = "bin/test-e2e-v2"
clusterNS = "clusters"
defaultVerbose = "false"
defaultVerbose = "false"
defaultGinkgoTimeout = "3h"
)

Expand All @@ -37,6 +35,11 @@ func main() {
artifactDir := requireEnv("ARTIFACT_DIR")
releaseImage := os.Getenv("RELEASE_IMAGE_LATEST")

testBinary := "bin/test-e2e-v2"
if binDir := os.Getenv("E2EV2_BIN_DIR"); binDir != "" {
testBinary = filepath.Join(binDir, "test-e2e-v2")
}

eventuallyVerbose := os.Getenv("EVENTUALLY_VERBOSE")
if eventuallyVerbose == "" {
eventuallyVerbose = defaultVerbose
Expand All @@ -51,7 +54,22 @@ func main() {
// Let the platform set up any env vars it needs for tests.
platform.SetupTestEnv(sharedDir)

matrix := platform.TestMatrix(releaseImage)
variants := os.Getenv("HYPERSHIFT_VARIANTS")
specs := lifecycle.FilterClusterSpecs(platform.ClusterSpecs(releaseImage, os.Getenv("OCP_IMAGE_N1")), variants)

@bryan-cox bryan-cox Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

observation (not blocking): specs is reconstructed here via platform.ClusterSpecs() independently of what create-guests used at cluster creation time. If env state differs between the two invocations (e.g., OCP_IMAGE_N1 set during create but absent during test), FilterTestMatrix could silently drop valid test groups.

Might be worth a brief comment here noting the assumption that env vars must be consistent across all four binaries.

@ironcladlou ironcladlou Jul 22, 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.

Wouldn't this general consistency issue be applicable to every environment variable shared amongst the binaries? Not sure it's worth calling out this single instance. What may be lacking is a stable non-environment input passed through to all of the binaries if there's some concern of environment drift during the same workflow, or something. Seems like a systemic thing to follow up on?

matrix := lifecycle.FilterTestMatrix(platform.TestMatrix(releaseImage), specs)

// Allow overriding the label filter for all test groups.
if override := os.Getenv("GINKGO_LABEL_FILTER"); override != "" {
log.Printf("Overriding label filters with GINKGO_LABEL_FILTER=%s", override)
for i := range matrix.Parallel {
matrix.Parallel[i].LabelFilter = override
}
for i := range matrix.Sequential {
for j := range matrix.Sequential[i].Steps {
matrix.Sequential[i].Steps[j].LabelFilter = override
}
}
}

var (
mu sync.Mutex
Expand All @@ -67,7 +85,7 @@ func main() {
defer wg.Done()
clusterName := readClusterName(sharedDir, g.ClusterFile)
log.Printf("Running %s tests against %s...", g.Name, clusterName)
err := runTestBinary(clusterName, g.LabelFilter, g.Skip,
err := runTestBinary(testBinary, clusterName, g.LabelFilter, g.Skip,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
filepath.Join(artifactDir, g.JUnitFile), g.ExtraEnv)
mu.Lock()
results = append(results, testResult{name: g.Name, err: err})
Expand All @@ -90,7 +108,7 @@ func main() {
for i, step := range sg.Steps {
clusterName := readClusterName(sharedDir, step.ClusterFile)
log.Printf("Running %s tests against %s...", step.Name, clusterName)
err := runTestBinary(clusterName, step.LabelFilter, step.Skip,
err := runTestBinary(testBinary, clusterName, step.LabelFilter, step.Skip,
filepath.Join(artifactDir, step.JUnitFile), step.ExtraEnv)
mu.Lock()
results = append(results, testResult{name: step.Name, err: err})
Expand Down Expand Up @@ -126,7 +144,7 @@ func main() {
log.Println("All test groups passed")
}

func runTestBinary(clusterName, labelFilter, skip, junitPath string, extraEnv []string) error {
func runTestBinary(testBinary, clusterName, labelFilter, skip, junitPath string, extraEnv []string) error {
ginkgoTimeout := os.Getenv("GINKGO_TIMEOUT")
if ginkgoTimeout == "" {
ginkgoTimeout = defaultGinkgoTimeout
Expand All @@ -146,6 +164,10 @@ func runTestBinary(clusterName, labelFilter, skip, junitPath string, extraEnv []
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr

clusterNS := os.Getenv("HYPERSHIFT_NAMESPACE")
if clusterNS == "" {
clusterNS = lifecycle.DefaultNamespace
}
cmd.Env = append(os.Environ(),
fmt.Sprintf("E2E_HOSTED_CLUSTER_NAME=%s", clusterName),
fmt.Sprintf("E2E_HOSTED_CLUSTER_NAMESPACE=%s", clusterNS),
Expand Down
54 changes: 52 additions & 2 deletions test/e2e/v2/lifecycle/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@ import (
"context"
"crypto/sha256"
"fmt"
"strings"

crclient "sigs.k8s.io/controller-runtime/pkg/client"
)

// DefaultNamespace is the default namespace for hosted clusters.
const DefaultNamespace = "clusters"

// ClusterSpec describes a single cluster to create for lifecycle tests.
type ClusterSpec struct {
Variant string
Expand Down Expand Up @@ -95,7 +99,6 @@ type PlatformConfig interface {
// DestroyArgs returns platform-specific args for
// "hypershift destroy cluster <platform>".
DestroyArgs() []string

}

// NewPlatformConfig creates a PlatformConfig for the given platform
Expand All @@ -110,6 +113,54 @@ func NewPlatformConfig(platform, sharedDir string) (PlatformConfig, error) {
}
}

// FilterClusterSpecs returns only the specs whose Variant is in the
// comma-separated variants string. If variants is empty, all specs
// are returned.
func FilterClusterSpecs(specs []ClusterSpec, variants string) []ClusterSpec {
if variants == "" {
return specs
}
allowed := make(map[string]bool)
for _, v := range strings.Split(variants, ",") {
allowed[strings.TrimSpace(v)] = true
}
var filtered []ClusterSpec
for _, s := range specs {
if allowed[s.Variant] {
filtered = append(filtered, s)
}
}
return filtered
}

// FilterTestMatrix removes test groups that reference cluster files
// not present in the given specs.
func FilterTestMatrix(matrix TestMatrix, specs []ClusterSpec) TestMatrix {
clusterFiles := make(map[string]bool)
for _, s := range specs {
clusterFiles[s.OutputFile] = true
}
var parallel []TestGroup
for _, g := range matrix.Parallel {
if clusterFiles[g.ClusterFile] {
parallel = append(parallel, g)
}
}
var sequential []SequentialGroup
for _, sg := range matrix.Sequential {
var steps []TestGroup
for _, step := range sg.Steps {
if clusterFiles[step.ClusterFile] {
steps = append(steps, step)
}
}
if len(steps) > 0 {
sequential = append(sequential, SequentialGroup{Name: sg.Name, Steps: steps})
}
}
return TestMatrix{Parallel: parallel, Sequential: sequential}
}

// DeriveClusterName builds a human-readable, deterministic cluster name
// from the prow job ID and cluster variant. The format is
// "{variant}-{hash10}" where hash10 is the first 10 hex characters of
Expand All @@ -119,4 +170,3 @@ func DeriveClusterName(prowJobID, variant string) string {
hash := sha256.Sum256([]byte(prowJobID))
return variant + "-" + fmt.Sprintf("%x", hash)[:10]
}

Loading