Awscapi #342

huali9 · 2024-11-21T06:19:43Z

Auto 3 capi case:
OCP-51071 - [CAPI] Create machineset with CAPI on aws
OCP-75395 - [CAPI] AWS Placement group support
OCP-75396 - [CAPI] Creating machines using KMS keys from AWS

liuhuali@Lius-MacBook-Pro cluster-api-actuator-pkg % ./hack/ci-integration.sh -focus "Cluster API AWS MachineSet" -v
Running Suite: Machine Suite - /Users/liuhuali/project/cluster-api-actuator-pkg/pkg
===================================================================================
Random Seed: 1732174559

Will run 3 of 45 specs
------------------------------
[BeforeSuite] 
/Users/liuhuali/project/cluster-api-actuator-pkg/pkg/e2e_test.go:63
[BeforeSuite] PASSED [1.856 seconds]
------------------------------
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS
------------------------------
Cluster API AWS MachineSet should be able to run a machine with a default provider spec [capi, disruptive]
/Users/liuhuali/project/cluster-api-actuator-pkg/pkg/capi/aws.go:103
  STEP: Creating core cluster @ 11/21/24 15:36:29.579
  STEP: Creating AWS machine template @ 11/21/24 15:36:30.591
  STEP: Creating MachineSet "aws-machineset-51071" @ 11/21/24 15:36:31.27
  STEP: Waiting for MachineSet machines "aws-machineset-51071" to enter Running phase @ 11/21/24 15:36:31.616
  STEP: Deleting MachineSet "aws-machineset-51071" @ 11/21/24 15:40:17.064
  STEP: Waiting for MachineSet "aws-machineset-51071" to be deleted @ 11/21/24 15:40:17.834
  STEP: Deleting /aws-machine-template @ 11/21/24 15:42:22.615
• [356.869 seconds]
------------------------------
Cluster API AWS MachineSet should be able to run a machine with cluster placement group [capi, disruptive]
/Users/liuhuali/project/cluster-api-actuator-pkg/pkg/capi/aws.go:115
  STEP: Creating AWS machine template @ 11/21/24 15:42:27.294I1121 15:42:27.294175   86632 aws_client.go:72] The created placementGroupID is pg-0ff9c0b32fae36690

  STEP: Creating MachineSet "aws-machineset-75395" @ 11/21/24 15:42:27.685
  STEP: Waiting for MachineSet machines "aws-machineset-75395" to enter Running phase @ 11/21/24 15:42:28.063
  STEP: Deleting MachineSet "aws-machineset-75395" @ 11/21/24 15:45:36.624
  STEP: Waiting for MachineSet "aws-machineset-75395" to be deleted @ 11/21/24 15:45:37.133
  STEP: Deleting /aws-machine-template @ 11/21/24 15:47:42.003
• [321.381 seconds]
------------------------------
Cluster API AWS MachineSet should be able to run a machine using KMS keys [capi, disruptive]
/Users/liuhuali/project/cluster-api-actuator-pkg/pkg/capi/aws.go:142
  STEP: Creating AWS machine template @ 11/21/24 15:47:44.44
  STEP: Creating MachineSet "aws-machineset-75396" @ 11/21/24 15:47:45.128
  STEP: Waiting for MachineSet machines "aws-machineset-75396" to enter Running phase @ 11/21/24 15:47:45.488
  STEP: Deleting MachineSet "aws-machineset-75396" @ 11/21/24 15:50:10.189
  STEP: Waiting for MachineSet "aws-machineset-75396" to be deleted @ 11/21/24 15:50:10.532
  STEP: Deleting /aws-machine-template @ 11/21/24 15:51:15.803
• [212.339 seconds]
------------------------------
SSSSSSSSSS
------------------------------
[ReportAfterSuite] Autogenerated ReportAfterSuite for --junit-report
autogenerated by Ginkgo
[ReportAfterSuite] PASSED [0.002 seconds]
------------------------------

Ran 3 of 45 Specs in 892.446 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 42 Skipped
PASS

Ginkgo ran 1 suite in 15m17.211460531s
Test Suite Passed

@sunzhaohua2 @miyadav @shellyyang1989 PTAL, thanks!

huali9 · 2024-11-21T23:41:54Z

/retest-required

huali9 · 2024-11-22T01:29:08Z

@huali9: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azure-operator 26311ca link true /test e2e-azure-operator
ci/prow/e2e-azure-capi-techpreview 26311ca link true /test e2e-azure-capi-techpreview
Full PR test history. Your PR dashboard.

I checked the azure failure, it should be flake, and this pr will not affect azure, so we can ignore that.
I think the pr is ready to review. @sunzhaohua2 @miyadav @shellyyang1989 PTAL, thanks!

sunzhaohua2 · 2024-11-22T02:20:40Z

pkg/framework/aws_client.go

@@ -45,3 +45,41 @@ func (a *AwsClient) CancelCapacityReservation(capacityReservationID string) (boo

 	return ptr.Deref(result.Return, false), err
 }
+
+// CreateCapacityReservation Create CapacityReservation.


should be CreatePlacementGroup

updated, thank you!

sunzhaohua2 · 2024-11-22T02:26:11Z

pkg/capi/aws.go

+
+func getDefaultAWSMAPIProviderSpec(cl client.Client) (*mapiv1.MachineSet, *mapiv1.AWSMachineProviderConfig) {
+	machineSetList := &mapiv1.MachineSetList{}
+	Expect(cl.List(ctx, machineSetList, client.InNamespace(framework.MachineAPINamespace))).To(Succeed())


Dam suggested use Evenutally on all cl. (client) actions instead of Expect, so we allow for retry on brief API/network hiccups. see here #337 (comment)

updated, thank you!

sunzhaohua2 · 2024-11-22T02:30:31Z

pkg/capi/aws.go

+	It("should be able to run a machine with a default provider spec", func() {
+		awsMachineTemplate = newAWSMachineTemplate(mapiDefaultProviderSpec)
+		if err = cl.Create(ctx, awsMachineTemplate); err != nil {
+			Expect(err).ToNot(HaveOccurred())


With all the .Should() and .To() (both with the Eventuallys and the Expects) could we add the optional description
#337 (comment)

all updated, thank you!

sunzhaohua2 · 2024-11-22T02:34:21Z

pkg/capi/aws.go

+		case "us-east-1":
+			key = "arn:aws:kms:us-east-1:301721915996:key/c471ec83-cfaf-41a2-9241-d9e99c4da344"
+		case "us-east-2":
+			key = "arn:aws:kms:us-east-2:301721915996:key/c228ef83-df2c-4151-84c4-d9f39f39a972"


can this be ran in dev's account?

I checked the job result, and yes, you are right, this cannot be ran in dev's account, I just comment this case out for now and think of how to deal with it, will update in a following pr. Thank you @sunzhaohua2 PTAL again, thanks!

Updated this case to skip it in dev's account.

sunzhaohua2 · 2024-11-22T07:36:15Z

pkg/capi/aws.go

+	awsMachineSpec := awsv1.AWSMachineSpec{
+		UncompressedUserData: &uncompressedUserData,
+		IAMInstanceProfile:   *mapiProviderSpec.IAMInstanceProfile.ID,
+		InstanceType:         mapiProviderSpec.InstanceType,
+		AMI: awsv1.AMIReference{
+			ID: mapiProviderSpec.AMI.ID,
+		},
+		Ignition: &awsv1.Ignition{
+			Version:     "3.4",
+			StorageType: awsv1.IgnitionStorageTypeOptionUnencryptedUserData,
+		},
+		Subnet: &awsv1.AWSResourceReference{
+			Filters: []awsv1.Filter{
+				{
+					Name:   "tag:Name",
+					Values: mapiProviderSpec.Subnet.Filters[0].Values,
+				},
+			},
+		},
+		AdditionalSecurityGroups: []awsv1.AWSResourceReference{
+			{
+				Filters: []awsv1.Filter{
+					{
+						Name:   "tag:Name",
+						Values: mapiProviderSpec.SecurityGroups[0].Filters[0].Values,
+					},
+				},
+			},
+		},
+	}
+
+	awsMachineTemplate := &awsv1.AWSMachineTemplate{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      awsMachineTemplateName,
+			Namespace: framework.ClusterAPINamespace,
+		},
+		Spec: awsv1.AWSMachineTemplateSpec{
+			Template: awsv1.AWSMachineTemplateResource{
+				Spec: awsMachineSpec,
+			},
+		},


here can use resourcebuilder https://github.com/openshift/cluster-api-actuator-pkg/tree/master/testutils/resourcebuilder

Updated, thank you @sunzhaohua2 PTAL again, it passed in local.

liuhuali@Lius-MacBook-Pro cluster-api-actuator-pkg % ./hack/ci-integration.sh -focus "Cluster API AWS MachineSet" -v Running Suite: Machine Suite - /Users/liuhuali/project/cluster-api-actuator-pkg/pkg =================================================================================== Random Seed: 1732506965 Will run 2 of 44 specs ------------------------------ [BeforeSuite] /Users/liuhuali/project/cluster-api-actuator-pkg/pkg/e2e_test.go:63 [BeforeSuite] PASSED [1.360 seconds] ------------------------------ SSSSSSSSSSSSSSSSSSSSSSSSSS ------------------------------ Cluster API AWS MachineSet should be able to run a machine with a default provider spec [capi, disruptive] /Users/liuhuali/project/cluster-api-actuator-pkg/pkg/capi/aws.go:100 STEP: Creating core cluster @ 11/25/24 11:56:29.931 STEP: Creating AWS machine template @ 11/25/24 11:56:30.766 STEP: Creating MachineSet "aws-machineset-51071" @ 11/25/24 11:56:31.32 STEP: Waiting for MachineSet machines "aws-machineset-51071" to enter Running phase @ 11/25/24 11:56:31.602 STEP: Deleting MachineSet "aws-machineset-51071" @ 11/25/24 12:00:17.049 STEP: Waiting for MachineSet "aws-machineset-51071" to be deleted @ 11/25/24 12:00:17.327 STEP: Deleting /aws-machine-template @ 11/25/24 12:01:21.212 • [294.166 seconds] ------------------------------ Cluster API AWS MachineSet should be able to run a machine with cluster placement group [capi, disruptive] /Users/liuhuali/project/cluster-api-actuator-pkg/pkg/capi/aws.go:112 I1125 12:01:24.951211 28912 aws_client.go:72] The created placementGroupID is pg-0b86fc1ccb6aa9742 STEP: Creating AWS machine template @ 11/25/24 12:01:24.951 STEP: Creating MachineSet "aws-machineset-75395" @ 11/25/24 12:01:25.238 STEP: Waiting for MachineSet machines "aws-machineset-75395" to enter Running phase @ 11/25/24 12:01:25.521 STEP: Deleting MachineSet "aws-machineset-75395" @ 11/25/24 12:05:51.76 STEP: Waiting for MachineSet "aws-machineset-75395" to be deleted @ 11/25/24 12:05:52.055 STEP: Deleting /aws-machine-template @ 11/25/24 12:06:55.93 • [336.224 seconds] ------------------------------ SSSSSSSSSSSSSSSS ------------------------------ [ReportAfterSuite] Autogenerated ReportAfterSuite for --junit-report autogenerated by Ginkgo [ReportAfterSuite] PASSED [0.003 seconds] ------------------------------ Ran 2 of 44 Specs in 631.752 seconds SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 42 Skipped PASS Ginkgo ran 1 suite in 10m52.059989621s Test Suite Passed

huali9 · 2024-11-25T06:59:26Z

@huali9: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azure-operator 2b5eb5c link true /test e2e-azure-operator
Full PR test history. Your PR dashboard.

The azure job failed should be flake, and this pr will not affect azure, so we can ignore that.

sunzhaohua2 · 2024-11-26T01:43:31Z

Thank you! @huali9
LGTM

huali9 · 2024-11-26T03:13:44Z

/assign @JoelSpeed @damdo

openshift-ci · 2024-11-27T10:43:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from damdo. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shellyyang1989 · 2024-11-28T10:57:11Z

cc @RadekManak for review. Thank you!

JoelSpeed · 2024-11-28T15:31:16Z

pkg/framework/aws_client.go

+	placementGroupID := ptr.Deref(result.PlacementGroup.GroupId, "")
+	klog.Infof("The created placementGroupID is %s", placementGroupID)
+
+	return placementGroupID, err


err will always be nil here, better to return nil

Suggested change

return placementGroupID, err

return placementGroupID, nil

updated, thank you!

JoelSpeed · 2024-11-28T15:32:17Z

pkg/framework/aws_client.go

+	result, err := a.Svc.DeletePlacementGroup(input)
+
+	return result.String(), err


Prefer to check the error explicitly

Suggested change

result, err := a.Svc.DeletePlacementGroup(input)

return result.String(), err

result, err := a.Svc.DeletePlacementGroup(input)

if err != nil {

return "", fmt.Errorf("could not delete placement group: %w", err)

}

return result.String(), nil

updated, thank you!

JoelSpeed · 2024-11-28T15:33:17Z

pkg/capi/aws.go

+var (
+	cl          client.Client
+	ctx         = context.Background()
+	platform    configv1.PlatformType
+	clusterName string
+	oc          *gatherer.CLI
+)


Need these be exposed at this scope, or could they be within the Describe on L38?

moved to within the Describe, thank you!

JoelSpeed · 2024-11-28T15:34:21Z

pkg/capi/aws.go

+		if platform != configv1.AWSPlatformType {
+			Skip("Skipping AWS E2E tests")
+		}
+		oc, _ = framework.NewCLI()


What is the second return variable here and why are we ignoring it?

the second return variable is error, updated, thank you!

JoelSpeed · 2024-11-28T15:35:31Z

pkg/capi/aws.go

+		if platform != configv1.AWSPlatformType {
+			// Because AfterEach always runs, even when tests are skipped, we have to
+			// explicitly skip it here for other platforms.
+			Skip("Skipping AWS E2E tests")
+		}


Would they not have skipped in the BeforeAll?

You are right, they should have skipped in the BeforeAll, deleted here, thank you!

JoelSpeed · 2024-11-28T15:36:22Z

pkg/capi/aws.go

+			Skip("Skipping AWS E2E tests")
+		}
+		if !deleted {
+			framework.DeleteCAPIMachineSets(ctx, cl, machineSet)


If this framework tolerated the object not existing, you wouldn't need to track whether the test expected to delete the machineset or not.

Are there cases where you want to be sure it did delete?

I think we should delete the objects we created in the test, right?

JoelSpeed · 2024-11-28T15:37:34Z

pkg/capi/aws.go

+		placementGroupID, err := awsClient.CreatePlacementGroup(placementGroupName, "cluster")
+		Expect(err).ToNot(HaveOccurred(), "Failed to create placementgroup")
+		Expect(placementGroupID).ToNot(Equal(""), "expected the placementGroupID to not be empty string")
+		defer func() {


I think if you were to leverage DeferCleanup you might be able to avoid some complexity here

yeah, great, updated, thank you!

JoelSpeed · 2024-11-28T15:38:20Z

pkg/capi/aws.go

+			framework.DeleteCAPIMachineSets(ctx, cl, machineSet)
+			framework.WaitForCAPIMachineSetsDeleted(ctx, cl, machineSet)
+			framework.DeleteObjects(ctx, cl, awsMachineTemplate)


Is it possible for these to cause the defer to exit prior to deleting the placement group?

You are right, that may occur, updated to use DeferCleanup, thank you!

JoelSpeed · 2024-11-28T15:39:47Z

pkg/capi/aws.go

+		if err != nil {
+			Skip(fmt.Sprintf("Skip because cannot get the key %v", err))
+		}


Skipping because of errors is likely to mean this test gets skipped when we don't want it to. Is there a better way to skip tests when we are in the dev account, vs QE account? Perhaps using labels?

skipped for errors is a temporary solution, using labels should be better way, but we don't have that labels at present, and that's what we are discussing on slack https://redhat-internal.slack.com/archives/GE2HQ9QP4/p1732789329032099, we need identify which cases run in DEV CI, which cases run in QE CI, then add labels for them, then update all the DEV jobs to only run the cases chosen for DEV CI. Currently all the cases in this repo are run in DEV CI.

Thank you @JoelSpeed I added a new label qe-account-only and updated Makefile to exclude use cases with such labels. And all other comments also updated, PLAT again, thanks!

Does this new label align with the plan @sunzhaohua2 has proposed?

Yes, the meaning is the same, just the name is different, Zhaohua named it qe-only and I named it qe-account-only, I can update the name when we make a decision.

JoelSpeed · 2024-11-28T15:40:30Z

pkg/capi/aws.go

+		if err := cl.Create(ctx, awsMachineTemplate); err != nil {
+			Expect(err).ToNot(HaveOccurred(), "Failed to create awsmachinetemplate")
+		}


What about

Suggested change

if err := cl.Create(ctx, awsMachineTemplate); err != nil {

Expect(err).ToNot(HaveOccurred(), "Failed to create awsmachinetemplate")

}

Expect(cl.Create(ctx, awsMachineTemplate)).To(Succeed(), "Failed to create awsmachinetemplate")

updated, thank you!

openshift-ci · 2024-12-02T06:56:56Z

@huali9: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-azure-capi-techpreview	`49e57a2`	link	true	`/test e2e-azure-capi-techpreview`

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.

openshift-ci bot requested review from JoelSpeed and theobarberbany November 21, 2024 06:20

huali9 mentioned this pull request Nov 21, 2024

[WIP]auto capi cases #339

Closed

huali9 force-pushed the awscapi branch 2 times, most recently from b6329be to 202e9ef Compare November 21, 2024 09:49

sunzhaohua2 reviewed Nov 22, 2024

View reviewed changes

huali9 force-pushed the awscapi branch from 26311ca to 0d1c910 Compare November 22, 2024 03:27

sunzhaohua2 reviewed Nov 22, 2024

View reviewed changes

huali9 force-pushed the awscapi branch from 0d1c910 to 2b5eb5c Compare November 25, 2024 04:12

huali9 force-pushed the awscapi branch from 2b5eb5c to 49e57a2 Compare November 25, 2024 08:50

openshift-ci bot assigned damdo and JoelSpeed Nov 26, 2024

huali9 force-pushed the awscapi branch from 49e57a2 to 91aa86c Compare November 27, 2024 10:39

JoelSpeed reviewed Nov 28, 2024

View reviewed changes

huali9 force-pushed the awscapi branch 4 times, most recently from 4e20177 to d5e1c87 Compare December 2, 2024 03:40

udpate aws capi go mod

e00c559

huali9 force-pushed the awscapi branch from d5e1c87 to 99b6f8d Compare December 2, 2024 04:58

add aws capi cases

7d6a991

huali9 force-pushed the awscapi branch from 99b6f8d to 7d6a991 Compare December 2, 2024 06:56

		result, err := a.Svc.DeletePlacementGroup(input)

		return result.String(), err

Awscapi #342

Are you sure you want to change the base?

Awscapi #342

Conversation

huali9 commented Nov 21, 2024 • edited Loading

huali9 commented Nov 21, 2024

huali9 commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huali9 commented Nov 25, 2024

sunzhaohua2 commented Nov 26, 2024

huali9 commented Nov 26, 2024

openshift-ci bot commented Nov 27, 2024

shellyyang1989 commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 2, 2024 • edited Loading

huali9 commented Nov 21, 2024 •

edited

Loading

openshift-ci bot commented Dec 2, 2024 •

edited

Loading