vSwitch and SecurityGroup query refactor by kwoodson · Pull Request #12 · openshift/cluster-api-provider-alibaba

kwoodson · 2021-11-09T20:03:04Z

With the work going into #11, we wanted to tidy up the querying methods to be similar to other providers as well as prepare the API to be moved to openshift/api.

This pull request addresses the feedback given in #11 and I will work on a corresponding PR to the installer.

kwoodson · 2021-11-09T21:38:45Z

@JoelSpeed I have some questions about testing this. Due to our renaming in the go.mod to module github.com/openshift/... the installer repository does not like the replace directive as we are renaming. I can get around this but I have to refactor code to make the libraries match which backtracks a bit of the work in this PR. If you have a few minutes to spare tomorrow that would be great.

kwoodson · 2021-11-12T03:15:52Z

@JoelSpeed @elmiko This PR addresses the feedback left for #11. Please review and let me know if there is anything you would like me to fix.

I was able to test this code and the machinesets successfully created the nodes.

NAME                                 STATUS   ROLES    AGE     VERSION
test-6m9rh-master-0                  Ready	master   137m    v1.22.1+f773b8b
test-6m9rh-master-1                  Ready	master   136m    v1.22.1+f773b8b
test-6m9rh-master-2                  Ready	master   138m    v1.22.1+f773b8b
test-6m9rh-worker-us-east-1a-v2wn5   Ready	worker   28m     v1.22.1+f773b8b
test-6m9rh-worker-us-east-1b-8dlqd   Ready    worker   81s     v1.22.1+f773b8b
test-6m9rh-worker-us-east-1b-xwzhd   Ready    worker   3m40s   v1.22.1+f773b8b
NAME                                 PHASE     TYPE            REGION	   ZONE         AGE
test-6m9rh-master-0                  Running   ecs.g6.xlarge   us-east-1   us-east-1b   139m
test-6m9rh-master-1                  Running   ecs.g6.xlarge   us-east-1   us-east-1a   139m
test-6m9rh-master-2                  Running   ecs.g6.xlarge   us-east-1   us-east-1b   139m
test-6m9rh-worker-us-east-1a-v2wn5   Running   ecs.g6.large    us-east-1   us-east-1a   32m
test-6m9rh-worker-us-east-1b-8dlqd   Running   ecs.g6.large    us-east-1   us-east-1b   6m
test-6m9rh-worker-us-east-1b-xwzhd   Running   ecs.g6.large    us-east-1   us-east-1b   7m40s

JoelSpeed · 2021-11-12T13:00:23Z

pkg/apis/alibabacloudprovider/v1beta1/alibabacloudmachineproviderconfig_types.go

+	// Filters is a set of metadata based upon ECS object tags used to identify a resource
 	Tags []Tag `json:"tags,omitempty"`


This comment doesn't match the field name, did we intend to rename the filed?

I was going to go filters but Tags are actually used and the structure for AWS didn't align here. AWS can have a single key and multiple values. Where as here, it is 1:1 for key:value. I think Tags is appropriate.

Will rename.

JoelSpeed · 2021-11-12T13:02:15Z

pkg/actuators/machine/instances.go

+	if machineProviderConfig.SecurityGroups == nil {
+		return nil, errors.New("no security configuration provided")
+	}


There's a different between nil and an empty slice that would make this not work. Best to use len which handles both cases

Suggested change

if machineProviderConfig.SecurityGroups == nil {

return nil, errors.New("no security configuration provided")

}

if len(machineProviderConfig.SecurityGroups) == 0 {

return nil, errors.New("no security configuration provided")

}

This makes sense since we create it on the previous line. I will update.

JoelSpeed · 2021-11-12T13:03:34Z

pkg/actuators/machine/instances.go

+	if machineProviderConfig.VpcID != "" {
+		request.VpcId = machineProviderConfig.VpcID
+	}
+	if machineProviderConfig.ResourceGroupID != "" {
+		request.ResourceGroupId = machineProviderConfig.ResourceGroupID
+	}


Do these need to be behind if statements?

I honestly do not know. I was reusing the code that was already there. Although it appears in the DescribeSecurityGroups API call that when making the request the VpcId and the ResourceGroupID looks like this:

VpcId: (string) "" ResourceGroupId: (string) (len=18) "rg-acfnxrgr6qtm3mq",

so I think you are probably on to something. Setting them to whatever the default is on the machineProviderConfig would probably work since it is defaulting them to empty anyway.

JoelSpeed · 2021-11-12T13:04:39Z

pkg/actuators/machine/instances.go

+	if mpc.VpcID != "" {
+		describeVSwitchesRequest.VpcId = mpc.VpcID
 	}


Does this need to be behind an if statement?

Will remove and test.

JoelSpeed · 2021-11-12T13:05:20Z

pkg/actuators/machine/instances.go

+		klog.Errorf("no vswitches for given tags not found")
+		return "", fmt.Errorf("no vswitches for given tags not found")


These messages are not parsing for me, what are we trying to say here?

This was left over from the original code. I will update. Basically with the provided tags, vpcid, and regionid the query did not return any security groups.

JoelSpeed · 2021-11-12T13:05:36Z

pkg/apis/alibabacloudprovider/v1beta1/alibabacloudmachineproviderconfig_types.go

 	//This parameter is required when you create an instance of the VPC type.
 	//You can call the DescribeVSwitches operation to query the created vSwitches.
-	VSwitch *ResourceTagReference `json:"vSwitch,omitempty"`
+	VSwitch *AlibabaResourceReference `json:"vSwitch,omitempty"`


Does this need to be a pointer?

I saw examples in the AWS where it was so I went with that. I now also see examples where it does not need to be. I'll try and remove the pointer.

kwoodson · 2021-11-12T21:36:19Z

@elmiko @JoelSpeed I have updated this PR from your comments. I have also created the corresponding installer PR here openshift/installer#5381.

I have created 3 clusters using this code and all have been successful creates. This also included removing dependencies and getting updates to cluster-api-provider-{ovirt,openstack} and updates to machine-api-operator-openstack to remove older mao dependencies.

Once this work is finished I will update the API #10 with the changes in this PR and being the migration to openshift/api.

Please TAL

elmiko

this generally looks good to me
/approve

openshift-ci · 2021-11-16T20:47:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko, kwoodson

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [elmiko]

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

JoelSpeed

/lgtm

One nit, but it's not blocking, let's merge this and we can clean it up down the line if needs be

JoelSpeed · 2021-11-17T10:39:54Z

pkg/actuators/machine/instances.go

+		} else {
+			if sg.Tags != nil {


Nit: Looks like this could be an else if and reduce indentation by 1 level

kwoodson requested review from JoelSpeed and elmiko November 9, 2021 20:03

openshift-ci bot requested a review from alexander-demicev November 9, 2021 20:14

kwoodson mentioned this pull request Nov 9, 2021

Alibaba providerfix #11

Merged

kwoodson force-pushed the gh_comments branch from 863f125 to 422c10e Compare November 11, 2021 17:34

Vendor: move machine api to openshift/api

6a910c9

kwoodson force-pushed the gh_comments branch 3 times, most recently from e17d0ed to f3295ab Compare November 12, 2021 03:09

JoelSpeed reviewed Nov 12, 2021

View reviewed changes

update query tags and rename to remove cloud from names

b73af66

kwoodson force-pushed the gh_comments branch from f3295ab to b73af66 Compare November 12, 2021 15:02

elmiko reviewed Nov 16, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2021

JoelSpeed reviewed Nov 17, 2021

View reviewed changes

openshift-ci bot assigned JoelSpeed Nov 17, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

openshift-merge-robot merged commit 3924ffc into openshift:main Nov 17, 2021

This was referenced Nov 17, 2021

Alibaba: Update tag and API types to mirror other providers openshift/installer#5381

Merged

Alibaba provider types update, consolidation, and clean up #10

Merged

Machine API: adding Alibaba cloud provider types. openshift/api#1045

Merged

		// Filters is a set of metadata based upon ECS object tags used to identify a resource
		Tags []Tag `json:"tags,omitempty"`

		klog.Errorf("no vswitches for given tags not found")
		return "", fmt.Errorf("no vswitches for given tags not found")

Conversation

kwoodson commented Nov 9, 2021

Uh oh!

kwoodson commented Nov 9, 2021

Uh oh!

kwoodson commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwoodson Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwoodson commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 16, 2021

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

kwoodson Nov 12, 2021 •

edited

Loading

kwoodson commented Nov 12, 2021 •

edited

Loading