OCPCLOUD-2992: migration e2e for creating machine with authoritativeAPI ClusterAPI #320
Conversation
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
huali9
force-pushed
the
machinemig
branch
2 times, most recently
from
4262eea to
d046757
Compare
openshift-merge-robot
added
the
needs-rebase
huali9
force-pushed
the
machinemig
branch
3 times, most recently
from
10a25e4 to
4e18a0c
Compare
openshift-merge-robot
removed
the
needs-rebase
huali9
force-pushed
the
machinemig
branch
2 times, most recently
from
582d6fe to
9474904
Compare
|
creation and deletion cases passed in my local now, update and migration cases are blocked by bug https://issues.redhat.com/browse/OCPBUGS-54703 |
|
@huali9 Could you update the title on the PR? I assume this is e2es, but I'm not sure. |
huali9
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@huali9: 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. 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. |
|
/test ci/prow/unit |
|
@huali9: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
|
/test unit |
|
/retest |
|
@huali9: 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. 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. |
|
/hold |
huali9
force-pushed
the
machinemig
branch
3 times, most recently
from
8c74e79 to
9231414
Compare
huali9
changed the title
|
/test unit |
|
/retest |
|
@damdo Could you please help review this pr? Thanks! |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/test unit |
1 similar comment
|
/test unit |
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
Thanks a lot @huali9 for the PR!
Logic looks good, just a bunch of comments around naming/text for consistency with tests introduced in https://github.com/openshift/cluster-capi-operator/pull/330/files?diff=unified&w=0
e2e/framework/machine.go
Outdated
| if err := cl.Get(context.Background(), key, machine); err != nil { | ||
| return nil, fmt.Errorf("error querying api for awsmachine object: %w", err) | ||
| } |
There was a problem hiding this comment.
Let's wrap this into an eventually like below so we are more robust to transient/intermittent issues
e2e/machine_migration_test.go
Outdated
| } | ||
| }) | ||
|
|
||
| var _ = Describe("Machine Creation", Ordered, func() { |
There was a problem hiding this comment.
| var _ = Describe("Machine Creation", Ordered, func() { | |
| var _ = Describe("Create MAPI Machine", Ordered, func() { |
To stay consistent with https://github.com/openshift/cluster-capi-operator/pull/330/files#diff-5ebecef9b3221104569dfaa6563ae90c684017b1197e77e6327bad5100371f97R41
e2e/machine_migration_test.go
Outdated
| }) | ||
|
|
||
| var _ = Describe("Machine Creation", Ordered, func() { | ||
| var machineNameCAPI = "machine-auth-capi-creation" |
There was a problem hiding this comment.
| var machineNameCAPI = "machine-auth-capi-creation" | |
| var mapiMachineAuthCAPIName = "machine-authoritativeapi-capi" |
To stay consistent with https://github.com/openshift/cluster-capi-operator/pull/330/files#diff-5ebecef9b3221104569dfaa6563ae90c684017b1197e77e6327bad5100371f97R43
e2e/machine_migration_test.go
Outdated
| var newMapiMachine *machinev1beta1.Machine | ||
| var err error | ||
|
|
||
| Context("when existing CAPI Machine with same name should allow creating the MAPI Machine with spec.authoritativeAPI: ClusterAPI", func() { |
There was a problem hiding this comment.
Again for consistency with the MachineSet side of these tests, which already merged, I'd go with something like:
| Context("when existing CAPI Machine with same name should allow creating the MAPI Machine with spec.authoritativeAPI: ClusterAPI", func() { | |
| Context("with spec.authoritativeAPI: CAPI and already existing CAPI Machine with same name", func() { |
e2e/machine_migration_test.go
Outdated
| newMachine.Status = machinev1beta1.MachineStatus{} | ||
| newMachine.Spec.AuthoritativeAPI = authority | ||
| By(fmt.Sprintf("Creating a new %s machine in namespace: %s", authority, newMachine.Namespace)) | ||
| Expect(cl.Create(ctx, newMachine)).To(Succeed()) |
There was a problem hiding this comment.
Let's change this to an Eventually().Should() and add an error message if the Should() fails, to make this more robust to transient issues.
e2e/machine_migration_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| Context("when no existing CAPI Machine with same name should allow creating the MAPI Machine with spec.authoritativeAPI: ClusterAPI", func() { |
There was a problem hiding this comment.
| Context("when no existing CAPI Machine with same name should allow creating the MAPI Machine with spec.authoritativeAPI: ClusterAPI", func() { | |
| Context("with spec.authoritativeAPI: ClusterAPI and no existing CAPI Machine with same name", func() { |
e2e/machine_migration_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| It("should verify CAPI Machine get Running", func() { |
There was a problem hiding this comment.
| It("should verify CAPI Machine get Running", func() { | |
| It("should verify CAPI Machine gets created and becomes Running", func() { |
e2e/machine_migration_test.go
Outdated
| // The test requires at least one existing MAPI machine to act as a template. | ||
| Expect(machineList).NotTo(BeEmpty(), "No MAPI machines found in the openshift-machine-api namespace to use as a template") |
There was a problem hiding this comment.
Template is already a "reserved" word in CAPI language.
We could use something like `to use as a reference for creating a new one".
| // The test requires at least one existing MAPI machine to act as a template. | |
| Expect(machineList).NotTo(BeEmpty(), "No MAPI machines found in the openshift-machine-api namespace to use as a template") | |
| // The test requires at least one existing MAPI machine to act as a template. | |
| Expect(machineList).NotTo(BeEmpty(), "No MAPI machines found in the openshift-machine-api namespace to use as a reference for creating a new one") |
e2e/machine_migration_test.go
Outdated
| // Select the first machine from the list as our template. | ||
| templateMachine := machineList[0] | ||
| By(fmt.Sprintf("Using MAPI machine %s as a template", templateMachine.Name)) | ||
|
|
||
| // Define the new machine based on the template. | ||
| newMachine := &machinev1beta1.Machine{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: machineName, | ||
| Namespace: templateMachine.Namespace, | ||
| }, | ||
| Spec: *templateMachine.Spec.DeepCopy(), | ||
| } |
There was a problem hiding this comment.
Template is already a "reserved" word in CAPI language, let's use something similar to the suggestion in the above comment.
| capiMachineList, err := capiframework.GetMachines(cl) | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to list CAPI machines") | ||
| // The test requires at least one existing CAPI machine to act as a template. | ||
| Expect(capiMachineList).NotTo(BeEmpty(), "No CAPI machines found in the openshift-cluster-api namespace to use as a template") | ||
|
|
||
| // Select the first machine from the list as our template. | ||
| templateCapiMachine := capiMachineList[0] | ||
| By(fmt.Sprintf("Using CAPI machine %s as a template", templateCapiMachine.Name)) | ||
|
|
||
| // Define the new machine based on the template. | ||
| newCapiMachine := &clusterv1.Machine{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: machineName, | ||
| Namespace: templateCapiMachine.Namespace, | ||
| }, | ||
| Spec: *templateCapiMachine.Spec.DeepCopy(), | ||
| } |
There was a problem hiding this comment.
Template is already a "reserved" word in CAPI language, let's use something similar to the suggestion in the above comment.
huali9
force-pushed
the
machinemig
branch
2 times, most recently
from
13c46a1 to
ac422eb
Compare
|
Thanks a lot @damdo for the detailed review. I have updated, PTAL again, thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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
|
/test e2e-gcp-ovn-techpreview |
|
This PR only adds E2Es that don't run in /override ci/prow/e2e-gcp-ovn-techpreview |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-gcp-ovn-techpreview 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. |
1 similar comment
|
This PR only adds E2Es that don't run in [ci/prow/e2e-gcp-ovn-techpreview] /override ci/prow/e2e-gcp-ovn-techpreview |
|
Same for ci/prow/e2e-openstack-ovn-techpreview /override ci/prow/e2e-openstack-ovn-techpreview |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-gcp-ovn-techpreview 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. |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-openstack-ovn-techpreview 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. |
openshift-merge-bot
bot
merged commit e02143b
into
openshift:main
|
@huali9: The following test failed, say
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. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-cluster-capi-operator |
7 participants
This is ready for review @sunzhaohua2 @miyadav @shellyyang1989 @JoelSpeed PTAL, thanks!