OCPCLOUD-2992: add migration e2e test for scaling/updating/deleting machineset #337
Conversation
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 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 "4.20.0" version, but no target version was set. In 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
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
Thanks a lot Zhaohua!!
The structure of the tests looks good overall.
I left many comments but they are many occurrences of a few suggestions I have to make this more robust (with retries) or clearer in logs.
|
Thinking about onward maintainability: The current machineset_migration_test.go could be split into multiple focused files:
WDYT? |
openshift-merge-robot
added
the
needs-rebase
|
@sunzhaohua2 we should not have |
@coderabbitai can you confirm what cases we will need it to be present in e2e/go.mod , if the version we need doesn't have to be different than the parent go.mod ? |
openshift-merge-robot
added
the
needs-rebase
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@miyadav: You raise an excellent question about Go workspace dependency management. Let me examine the workspace setup to provide a definitive answer. 🏁 Script executed: Length of output: 25111 @miyadav: Great question! You're absolutely right to question this. Let me explain the specific cases where the dependency would or wouldn't be needed: When workspace resolution should handle it:With a proper
Why it's currently needed:Looking at the imports, I found the issue. The e2e module is importing:
The second import is the key - the e2e module is importing code from the parent module's The correct approach:Instead of having a versioned dependency, the e2e
Recommendation:The explicit versioned dependency
The workspace should handle the module resolution automatically since both modules are in the same workspace. |
openshift-merge-robot
removed
the
needs-rebase
|
@sunzhaohua2: This pull request references OCPCLOUD-2992 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 "4.21.0" version, but no target version was set. In 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
e2e/machineset_migration_helpers.go (1)
176-179: Field path is correctly maintained.The field path
Status.V1Beta2.Conditionsaccurately matches the CAPI MachineSet status structure, as confirmed by @sunzhaohua2's clarification that conditions are nested understatus.v1beta2.conditions.e2e/machine_migration_helpers.go (1)
276-276: Replace direct client call with polling wrapperThe client.Create call should be wrapped in a closure for proper retry behavior.
- Eventually(cl.Create(ctx, newMachine)).Should(Succeed(), "Failed to create new machine") + Eventually(func() error { + return cl.Create(ctx, newMachine) + }, capiframework.WaitShort, capiframework.RetryShort).Should(Succeed(), "Failed to create new machine")
🧹 Nitpick comments (6)
e2e/machineset_migration_helpers.go (1)
26-53: Consider extracting magic strings as constants.The test uses inline string literals for condition types and reasons. Consider defining these as constants in
e2e/migration_common.gofor better maintainability and reusability across tests.+// In e2e/migration_common.go +const ( + SynchronizationSuccessReason = "ResourceSynchronized" + AuthoritativeAPIMachineAPIReason = "AuthoritativeAPIMachineAPI" + AuthoritativeAPINotMachineAPIReason = "AuthoritativeAPINotMachineAPI" + PausedReasonNotPaused = "NotPaused" + PausedReasonPaused = "Paused" +) // In this file - HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Reason", Equal(SynchronizationSuccessReason)),e2e/machine_migration_capi_auth_test.go (2)
18-19: Fix grammatical error in skip message.There's a grammatical error in the skip message that should be corrected.
- Skip(fmt.Sprintf("Skipping tests on %s, this only support on aws", platform)) + Skip(fmt.Sprintf("Skipping tests on %s, this is only supported on AWS", platform))
52-55: Consider tracking OCPBUGS-54703 resolution.The pending tests (PIt) for verifying Synchronized condition are correctly marked due to bug OCPBUGS-54703. Ensure these are re-enabled once the bug is resolved.
Would you like me to create a tracking issue or reminder to re-enable these tests once OCPBUGS-54703 is resolved?
Also applies to: 86-89
e2e/aws_helpers.go (3)
103-128: Consider adding retry logic for AWS client creation.The AWS client creation might fail due to transient network issues or secret propagation delays. Consider wrapping the secret retrieval in an
Eventuallyblock for better resilience.func createAWSClient(region string) *ec2.EC2 { var secret corev1.Secret - Expect(cl.Get(context.Background(), client.ObjectKey{ - Namespace: framework.CAPINamespace, - Name: "capa-manager-bootstrap-credentials", - }, &secret)).To(Succeed()) + Eventually(func() error { + return cl.Get(context.Background(), client.ObjectKey{ + Namespace: framework.CAPINamespace, + Name: "capa-manager-bootstrap-credentials", + }, &secret) + }, framework.WaitShort, framework.RetryShort).Should(Succeed(), + "Failed to get AWS bootstrap credentials secret")
193-236: Consider enhancing instance comparison with tag validation.The comment at line 200 correctly notes that tags need manual comparison. Since tags are crucial for resource identification and management, consider adding a separate validation step.
Would you like me to generate a helper function that performs semantic tag comparison, focusing on important tags while ignoring dynamic ones like timestamps?
233-235: Consider using Gomega's Expect for assertion consistency.Instead of printing to GinkgoWriter and continuing, consider using Gomega's
Expectwith a custom matcher for better test failure reporting.- if !cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...) { - GinkgoWriter.Print("Instances created by MAPI and CAPI are not equal\n" + cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...)) - } + Expect(cmp.Equal(mapiEC2Instance, capiEC2Instance, cmpOpts...)).To(BeTrue(), + "Instances created by MAPI and CAPI should be equal\n%s", + cmp.Diff(mapiEC2Instance, capiEC2Instance, cmpOpts...))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (17)
e2e/aws_helpers.go(1 hunks)e2e/aws_test.go(2 hunks)e2e/e2e_common.go(1 hunks)e2e/e2e_test.go(1 hunks)e2e/framework/machine.go(3 hunks)e2e/framework/machineset.go(5 hunks)e2e/framework/machinetemplate.go(4 hunks)e2e/machine_migration_capi_auth_test.go(1 hunks)e2e/machine_migration_helpers.go(1 hunks)e2e/machine_migration_mapi_auth_test.go(1 hunks)e2e/machine_migration_test.go(0 hunks)e2e/machineset_migration_capi_auth_test.go(1 hunks)e2e/machineset_migration_helpers.go(1 hunks)e2e/machineset_migration_mapi_auth_test.go(1 hunks)e2e/machineset_migration_test.go(0 hunks)e2e/mapimachines_test.go(0 hunks)e2e/migration_common.go(1 hunks)
💤 Files with no reviewable changes (3)
- e2e/mapimachines_test.go
- e2e/machineset_migration_test.go
- e2e/machine_migration_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/migration_common.go
- e2e/e2e_common.go
- e2e/framework/machinetemplate.go
- e2e/e2e_test.go
- e2e/machineset_migration_mapi_auth_test.go
- e2e/machine_migration_mapi_auth_test.go
🔇 Additional comments (21)
e2e/framework/machine.go (3)
16-17: LGTM! Documentation consistency maintained.The comment correctly explains that the function gets machines from the default CAPI namespace, accurately reflecting the implementation that uses
CAPINamespace.
17-27: Good transition to assertion-based API.The migration from error-returning functions to assertion-based patterns using Gomega's
ExpectandEventuallyaligns well with the test framework's style, making test failures more informative and reducing boilerplate error handling.Also applies to: 33-42
59-60: Namespace usage verified — no action required.GetAWSMachine accepts a namespace parameter and the found call site passes capiframework.CAPINamespace (e2e/machine_migration_helpers.go:54).
e2e/machineset_migration_helpers.go (2)
333-334: Namespace corrected for AWSMachineTemplate verification.Good fix - using
CAPINamespaceinstead ofMAPINamespacefor AWSMachineTemplate deletion verification is correct, as these resources are created in the CAPI namespace throughout the test flow.
338-364: Comprehensive cleanup routine.The cleanup function properly handles all resource types (CAPI MachineSets, MAPI MachineSets, and AWS Machine Templates) with nil checks and proper deletion verification. This ensures test isolation.
e2e/aws_test.go (3)
8-9: LGTM! Consistent migration to v1beta1 MAPI types.The update from
mapiv1tomapiv1beta1aligns with the broader PR's adoption of v1beta1 types across all MAPI resources.
29-30: Context parameter added for consistency.The addition of the
ctxparameter togetDefaultAWSMAPIProviderSpecmaintains consistency with the broader refactoring that makes context passing explicit throughout the test suite.
65-65: Client parameter added to compareInstances.The addition of the
clparameter enables the comparison function to retrieve machine resources directly, removing the dependency on global state.e2e/machine_migration_capi_auth_test.go (1)
27-230: Well-structured test coverage for CAPI-authoritative migration scenarios.The test comprehensively covers:
- Machine creation with CAPI authority (both with and without pre-existing CAPI machines)
- Deletion scenarios for both authoritative and non-authoritative machines
- Proper verification of synchronization status, paused conditions, and resource cleanup
The use of DeferCleanup ensures resources are properly cleaned up even on test failures.
e2e/machine_migration_helpers.go (2)
311-312: LGTM!The call properly uses the capiframework alias after the duplicate import issue was resolved.
135-135: LGTM! Proper nil-safe handling of MAPI Machine phaseThe use of
ptr.Derefwith an empty string default properly handles the case whereStatus.Phaseis nil, preventing potential panics.e2e/machineset_migration_capi_auth_test.go (4)
119-126: LGTM! Proper empty slice checks before accessThe code correctly validates that both
mapiMachinesandcapiMachinesslices are non-empty before accessing index 0, preventing potential panics.
221-227: LGTM! Consistent empty slice validationThe implementation properly checks both slices for emptiness before accessing their first elements, maintaining consistency with the earlier pattern.
268-297: Confirm OCPBUGS-56897 status and either re-enable the verification or track itgh search returned no results for OCPBUGS-56897 in openshift/cluster-capi-operator, so we cannot confirm the bug is resolved.
- If the bug is resolved: uncomment and fix the verification block in e2e/machineset_migration_capi_auth_test.go (lines 268–297) and add a link to the resolving bug/PR in the test comment.
- If the bug is still open: convert the test to Pending (PIt) with a one-line rationale referencing OCPBUGS-56897 and a TODO linking the bug.
- Otherwise: open a follow-up issue to track re-enabling this test and reference the file/lines above.
144-145: Track pending MachineSet replica verification — PR #329 still open
- PR #329 (OCPCLOUD-2994) is OPEN (mergedAt: null).
- TODO comments referencing the PR were found at: e2e/machineset_migration_mapi_auth_test.go:191, 232, 284, 332.
- Ensure all commented verifyMAPI/MachineSet replica checks (including e2e/machineset_migration_capi_auth_test.go:144–145 and any other occurrences) have a TODO with the PR/URL so they can be easily re-enabled once PR #329 is merged.
e2e/framework/machineset.go (6)
103-106: LGTM! Clean refactor to non-error signatureThe change from error-returning to direct slice return aligns well with the test-oriented nature of this framework code, where failures should cause immediate test failures rather than error propagation.
108-111: Good error handling with local variableProperly scoped the error variable for the
cl.Getcall after removing error returns from other functions.
176-187: Consistent non-error pattern with assertionThe refactor to return
*clusterv1.MachineSetdirectly with internalEventuallyassertion maintains consistency with the broader framework changes.
190-201: Clean signature change for GetMachinesFromMachineSetThe removal of error return aligns with the pattern of immediate test failure on errors.
204-206: Good defensive check for empty machinesThe addition of
Expect(machines).ToNot(BeEmpty())properly validates the precondition before attempting to find the newest machine.
224-235: Clean refactor of ScaleMachineSetThe change to void return with internal assertion via
komega.Updateis consistent with the framework's error handling pattern.
|
/test remaining-required Scheduling tests matching the |
1 similar comment
|
/test remaining-required Scheduling tests matching the |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@CodeRabbit configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
poem: true
labeling_instructions: []
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
path_filters:
- '!**/vendor/**'
- '!vendor/**'
|
|
/test e2e-aws-capi-techpreview |
|
Hi @theobarberbany @damdo do you have any comments on this PR? If it is close to merge, I think there is no need to split it into multiple PRs. Otherwise, let me split it into several PRs to submit. |
If we could split this it would be great - its still a huge PR and I'll find it much easier to review if it's smaller and easier to fit into my head! :D ⏺ The total diff for PR #337 is:
- +1,993 additions
- -1,417 deletions
- 576 net lines added
- 18 files changed |
|
@sunzhaohua2: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Close this, will split it into several PRs to submit. |
9 participants
Summary by CodeRabbit
Tests
Refactor
Chores