CNTRLPLANE-3646: wire up AWS v2 e2e lifecycle test coverage - #9174
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ironcladlou: This pull request references CNTRLPLANE-3646 which is a valid jira issue. 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. |
📝 WalkthroughWalkthroughThe e2e v2 lifecycle framework now supports AWS platform configuration, including AWS-specific cluster creation arguments, test selection, lifecycle hooks, and teardown settings. Platform selection reads AWS region and zone environment variables. Guest creation, destruction, and dump commands pass a configurable hosted-cluster namespace. The test runner resolves the test binary and namespace at runtime and forwards them to test processes. Sequence Diagram(s)sequenceDiagram
participant LifecycleRunner
participant NewPlatformConfig
participant AWSPlatformConfig
participant HypershiftCLI
LifecycleRunner->>NewPlatformConfig: select aws platform
NewPlatformConfig->>AWSPlatformConfig: construct AWS configuration
LifecycleRunner->>AWSPlatformConfig: request lifecycle arguments
AWSPlatformConfig->>HypershiftCLI: provide AWS create and destroy arguments
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ 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
Manual e2e validation
Steps:
Results: passed — 553 passed, 0 failed, 411 skipped (25s)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9174 +/- ##
==========================================
+ Coverage 44.70% 44.77% +0.07%
==========================================
Files 775 775
Lines 97116 97210 +94
==========================================
+ Hits 43415 43528 +113
+ Misses 50707 50681 -26
- Partials 2994 3001 +7 see 9 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/v2/cmd/run-tests/main.go (1)
33-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for the new resolution paths.
resolveTestBinaryand theHYPERSHIFT_NAMESPACEfallback add behavior without corresponding tests. Add table-driven cases for configured/unconfigured values; extracting namespace resolution into a small helper would make it directly testable. As per coding guidelines, Go changes must be unit tested, and table-driven tests should be used where possible.Also applies to: 49-52
🤖 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/run-tests/main.go` around lines 33 - 38, The new resolution logic lacks unit coverage for configured and fallback paths. Add table-driven tests for resolveTestBinary, covering E2EV2_BIN_DIR set and unset, and extract HYPERSHIFT_NAMESPACE fallback resolution into a small testable helper before adding equivalent configured/unconfigured cases; keep existing defaults and behavior unchanged.Source: Coding guidelines
🤖 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/cmd/create-guests/main.go`:
- Line 271: Ensure the create-guest argument construction around the namespace
arguments keeps cfg.namespace authoritative by appending the framework namespace
after user-provided extras or rejecting namespace overrides. Preserve consistent
namespace usage for post-create and cleanup operations, and add a
buildCreateArgs regression test covering an extra --namespace override.
In `@test/e2e/v2/lifecycle/aws.go`:
- Around line 29-43: Add focused unit tests for the AWS lifecycle implementation
beyond ParseExtraArgs: verify create and destroy argument generation, and
validate PostCreate HostedCluster patch behavior. Include a test covering the
failure path when the required public output is missing, using the existing AWS
lifecycle symbols and test conventions.
- Around line 84-89: Update PostCreate’s handling of clusterNames so a missing
“cluster-name-public” entry returns a contextual error instead of silently
succeeding. Preserve the existing postCreatePublic call and error propagation
when the required output is present.
---
Nitpick comments:
In `@test/e2e/v2/cmd/run-tests/main.go`:
- Around line 33-38: The new resolution logic lacks unit coverage for configured
and fallback paths. Add table-driven tests for resolveTestBinary, covering
E2EV2_BIN_DIR set and unset, and extract HYPERSHIFT_NAMESPACE fallback
resolution into a small testable helper before adding equivalent
configured/unconfigured cases; keep existing defaults and behavior unchanged.
🪄 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: ca92c2e0-da74-42c7-b696-d8f06890c7f7
📒 Files selected for processing (7)
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/platform.gotest/e2e/v2/lifecycle/platform_test.go
|
/pipeline required |
|
Scheduling tests matching the |
|
/verified by manual e2e testing and existing e2e test regression analysis Manual e2e validation
Steps:
Results: passed — 553 passed, 0 failed, 411 skipped (25s)
|
Test Resultse2e-aws
e2e-aks
|
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. |
| cfg.zones = strings.Split(opts.Zones, ",") | ||
|
|
||
| cfg.additionalTags = []string{ | ||
| fmt.Sprintf("expirationDate=%s", time.Now().Add(4*time.Hour).UTC().Format(time.RFC3339)), |
There was a problem hiding this comment.
Is this necessary? AFAIK, the CI jobs can be configured to run longer as part of CI config. What is the purpose of this? Thanks
There was a problem hiding this comment.
behavioral carryover from the v1 aws flow: https://github.com/openshift/hypershift/blob/main/test/e2e/util/options.go#L340
if our AWS tests take >4 hours that would be a big problem. That said, we probably need to audit tag handling generally in the v2 paths (e.g. #8909), we may not need this anymore. Seems benign at the moment
There was a problem hiding this comment.
For reference, the current v2 AWS tests execute in 26 seconds :)
There was a problem hiding this comment.
OK. So let's keep as it is now 👍
There was a problem hiding this comment.
Since I wanted to go ahead and refactor the extra args stuff in line with your suggestion and @georgelipceanu I went ahead and cut these tags out for now
There was a problem hiding this comment.
And now I've discovered there's actually a hidden dependency on this... EnsureInfrastructureResourceTagsTest was basing its assertion on the presence of this tag. I don't think that's a great way to structure the test, but to remain less intrusive for this patch I'll restore the expirationDate tag. I guess that answers what it was really for...
There was a problem hiding this comment.
Restored the tag and added a comment about why it exists and that it should be refactored away
| args = append(args, "--etcd-storage-class="+cfg.etcdSC) | ||
| } | ||
|
|
||
| args = append(args, cfg.extraCreateArgs...) |
There was a problem hiding this comment.
Isn't this already available two lines below? We just need a way to get it to the spec.ExtraArgs. Something like it's done in this fresh PR: #8926 Note, there the flag is also called differently: EXTRA_ARGS. We should align this.
There was a problem hiding this comment.
Yeah, I could have mapped the env onto that struct with the same effect. The intention here is for the extra arguments to be platform agnostic. That said, I think that the way we're handling flags and environment throughout this v2 setup is a mess and needs re-thought and refactored en-masse with a new design of some kind instead of these environment variables. Thinking we need a followup to think through it and discuss holistically, otherwise it'll be more of these isolated and disjointed changes
There was a problem hiding this comment.
Went ahead and refactored to match the approach from #8926 which I agree with you and @georgelipceanu is cleaner and suits my needs
|
@mgencur thanks for the feedback and discussion, I think I want to go ahead and make the changes now before merging. Just have to wait for testing... |
4584b93 to
e3e5bf7
Compare
openshift-ci-robot
removed
the
verified
e3e5bf7 to
77956c3
Compare
|
@ironcladlou: This pull request references CNTRLPLANE-3646 which is a valid jira issue. 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: 2
🤖 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/aws.go`:
- Around line 43-46: Update the environment lookup in the EXTRA_ARGS parsing
block to read HYPERSHIFT_CREATE_EXTRA_ARGS instead of EXTRA_ARGS, preserving the
existing strings.Fields parsing and empty-value behavior.
- Around line 26-33: Update NewAWSPlatformConfig to copy opts.Zones into
cfg.zones alongside the existing region and sharedDir assignments, so CreateArgs
preserves and emits the configured or default availability zones.
🪄 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: a2bb9906-6367-4c67-9a6d-72e4f70a5af3
📒 Files selected for processing (6)
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/platform.go
e7121d3 to
eb91608
Compare
Add AWS platform lifecycle support for v2 e2e tests with a single public cluster variant for the initial implementation. The test matrix uses !lifecycle || hosted-cluster-aws to match existing prow step-registry coverage and add hosted-cluster-aws as lifecycle proof. Backwards compatible with existing step-registry workflow. EXTRA_ARGS provides a generic flag passthrough for cluster creation arguments used for local development.
eb91608 to
2d435b1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/e2e/v2/lifecycle/aws.go (2)
49-54: 🎯 Functional Correctness | 🟠 MajorRead
HYPERSHIFT_CREATE_EXTRA_ARGS.The PR contract defines
HYPERSHIFT_CREATE_EXTRA_ARGS, but this code readsEXTRA_ARGS; the documented variable is silently ignored.Proposed fix
- if envArgs := os.Getenv("EXTRA_ARGS"); envArgs != "" { + if envArgs := os.Getenv("HYPERSHIFT_CREATE_EXTRA_ARGS"); envArgs != "" {🤖 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/aws.go` around lines 49 - 54, Update AWSPlatformConfig.ClusterSpecs to read HYPERSHIFT_CREATE_EXTRA_ARGS instead of EXTRA_ARGS when parsing environment-provided arguments, while preserving the existing strings.Fields behavior and empty-value handling.
28-40: 🎯 Functional Correctness | 🟠 MajorPreserve the configured AWS zones.
NewPlatformConfigsuppliesAWSPlatformOptions.Zones, but this constructor never copies it intocfg.zones.CreateArgs()therefore emits--zones=and ignoresHYPERSHIFT_AWS_ZONES.Proposed fix
func NewAWSPlatformConfig(opts AWSPlatformOptions, sharedDir string) *AWSPlatformConfig { + zones := make([]string, 0, 1) + for _, zone := range strings.Split(opts.Zones, ",") { + if zone = strings.TrimSpace(zone); zone != "" { + zones = append(zones, zone) + } + } cfg := &AWSPlatformConfig{ region: opts.Region, + zones: zones, sharedDir: sharedDir,🤖 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/aws.go` around lines 28 - 40, Update NewAWSPlatformConfig to copy opts.Zones into cfg.zones when constructing AWSPlatformConfig. Preserve the configured zones so CreateArgs emits them and HYPERSHIFT_AWS_ZONES is honored.
🧹 Nitpick comments (1)
test/e2e/v2/lifecycle/aws.go (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported AWS lifecycle API.
Add comments describing the actual behavior of the exported AWS types, constructor, and methods. In particular, document that the lifecycle hook methods currently perform no work and return
nil.As per path instructions, exported functions in
test/e2e/v2must describe actual behavior, including panic conditions.Also applies to: 43-118
🤖 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/aws.go` around lines 16 - 28, Document the exported AWS lifecycle API around AWSPlatformOptions, NewAWSPlatformConfig, AWSPlatformConfig, and its lifecycle hook methods, using Go-style comments that state their actual behavior. Explicitly note that the lifecycle hooks perform no work and return nil, and include any panic conditions for exported functions as required by the path guidance.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.
Duplicate comments:
In `@test/e2e/v2/lifecycle/aws.go`:
- Around line 49-54: Update AWSPlatformConfig.ClusterSpecs to read
HYPERSHIFT_CREATE_EXTRA_ARGS instead of EXTRA_ARGS when parsing
environment-provided arguments, while preserving the existing strings.Fields
behavior and empty-value handling.
- Around line 28-40: Update NewAWSPlatformConfig to copy opts.Zones into
cfg.zones when constructing AWSPlatformConfig. Preserve the configured zones so
CreateArgs emits them and HYPERSHIFT_AWS_ZONES is honored.
---
Nitpick comments:
In `@test/e2e/v2/lifecycle/aws.go`:
- Around line 16-28: Document the exported AWS lifecycle API around
AWSPlatformOptions, NewAWSPlatformConfig, AWSPlatformConfig, and its lifecycle
hook methods, using Go-style comments that state their actual behavior.
Explicitly note that the lifecycle hooks perform no work and return nil, and
include any panic conditions for exported functions as required by the path
guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 143ef6e8-ab4b-42df-b505-2047ce075167
📒 Files selected for processing (6)
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/platform.go
|
/verified by manual e2e testing and CI regression analysis Manual e2e validation
Steps:
Results: passed — 552 passed, 0 failed, 416 skipped (22s)
|
|
@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. |
openshift-ci-robot
added
the
verified
|
/pipeline required |
|
Scheduling tests matching the |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devguyio, 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 |
openshift-ci
Bot
added
the
approved
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@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. |
5 participants
Add AWS platform lifecycle support for v2 e2e tests with a single public cluster variant for the initial implementation. The test matrix uses !lifecycle || hosted-cluster-aws to match existing prow step-registry coverage and add hosted-cluster-aws as lifecycle proof. Backwards compatible with existing step-registry workflow.
EXTRA_ARGS provides a generic flag passthrough for cluster creation arguments used for local development.
Summary by CodeRabbit
E2EV2_BIN_DIR) and hosted-cluster namespace (viaHYPERSHIFT_NAMESPACE).clusterswhen unset.