Bug 2037209: refactor for AlibabaResourceReference changes by elmiko · Pull Request #26 · openshift/cluster-api-provider-alibaba

elmiko · 2022-01-13T22:47:30Z

This change updates the logic around security group IDs, vswitch IDs,
and resource group IDs to match the union changes in the resource
reference API. AlibabaResourceReferences now contain a Type variable
which indicates whether it is an ID or a list of Tags.

The behavior has not been changed for security groups, vswitches, and
resource groups. This change is being added to help allow for scenarios
where the ResourceGroupID is not known and must be searched. Now that
the AlibabaResourceReference contains a discriminating union of ID and
Tags, we can properly address scenarios with limited information.

openshift-ci · 2022-01-13T22:47:36Z

@elmiko: This pull request references Bugzilla bug 2037209, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianli-wei

Details

In response to this:

Bug 2037209: refactor for AlibabaResourceReference changes

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/test-infra repository.

elmiko · 2022-01-13T22:48:21Z

this is most likely going to fail until we merge the API changes in openshift/api#1096

pkg/actuators/machine/instances.go

JoelSpeed · 2022-01-14T10:59:47Z

pkg/actuators/machine/instances.go

+// An error will be returned if no group id can be found, or if multiple groups are
+// found from the search tags.
+func getResourceGroupId(machineProviderConfig *machinev1.AlibabaCloudMachineProviderConfig) (string, error) {
+	switch machineProviderConfig.ResourceGroup.Type {


What happens when resource group is nil? Is this allowed? IIRC someone said resource group was an optional field?

it seems like we fail if the resource group is nil currently, the way the code is structured i'm not sure how it's supposed to find the security groups without the group id. but maybe that's ok?

i asked around a little bit, and we should not allow creation of resources in the default group. the user should always specify the resource group.

I saw a comment about most customers using the default group, are we sure this is correct?

sorry, i worded that poorly. we definitely should allow creation in the default group, but we should force the user to always specify the resource group so that there are no surprises when creating new machines.

It looks to me that resource group will never be nil in a provider config https://github.com/openshift/api/blob/master/machine/v1/types_alibabaprovider.go#L140 it's not a pointer

+1, we agreed that the user should always provide this information, even in the case where they want to use the default resource group. this will help ensure that the user isn't creating resources in the wrong group by accident.

pkg/actuators/machine/instances.go

elmiko · 2022-01-14T20:12:43Z

updated to match the API changes and to reflect our requirements

jianli-wei · 2022-01-17T13:46:48Z

"build openshift/api#1096,#26" failed, see https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-gcp-modern/1483070108938014720

elmiko · 2022-01-17T14:19:04Z

@jianli-wei it looks like it is still unable to build this, until we get the api change in place.

elmiko · 2022-01-21T18:21:07Z

/hold this will need to revendor the api before we can merge it

elmiko · 2022-01-21T21:06:06Z

here is the installer patch that should work this as well, openshift/installer#5562

elmiko · 2022-01-21T21:18:48Z

i have added a second commit to vendor from my branch of the api, this is only for testing and should be replaced before we merge.

This change updates the logic around security group IDs, vswitch IDs, and resource group IDs to match the union changes in the resource reference API. AlibabaResourceReferences now contain a `Type` variable which indicates whether it is an ID or a list of Tags. The behavior has not been changed for security groups, vswitches, and resource groups. This change is being added to help allow for scenarios where the ResourceGroupID is not known and must be searched. Now that the AlibabaResourceReference contains a discriminating union of ID and Tags, we can properly address scenarios with limited information.

This is required to support the resource reference changes.

elmiko · 2022-01-24T14:44:05Z

now that the API changes have merged, i have revendored this repository. we need one final round of manual testing before releasing the hold here.

alexander-demicev

/lgtm

alexander-demicev · 2022-01-24T15:58:39Z

pkg/actuators/machine/instances.go

+// An error will be returned if no group id can be found, or if multiple groups are
+// found from the search tags.
+func getResourceGroupId(machineProviderConfig *machinev1.AlibabaCloudMachineProviderConfig) (string, error) {
+	switch machineProviderConfig.ResourceGroup.Type {


It looks to me that resource group will never be nil in a provider config https://github.com/openshift/api/blob/master/machine/v1/types_alibabaprovider.go#L140 it's not a pointer

openshift-ci · 2022-01-24T15:59:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demichev, elmiko

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~~ [alexander-demichev,elmiko]

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

openshift-ci · 2022-01-24T16:18:45Z

@elmiko: all tests passed!

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

kwoodson · 2022-01-24T22:20:02Z

Test with this code today:

        resourceGroup:
          id: rg-xxxxxx
          type: ID

and

test-rx6ff-worker-us-east-1a-vdr6m   Ready    worker   14m   v1.23.0+06791f6
test-rx6ff-worker-us-east-1b-8l777   Ready    worker   14m   v1.23.0+06791f6
test-rx6ff-worker-us-east-1b-plcst   Ready    worker   16m   v1.23.0+06791f6

openshift-machine-api   test-rx6ff-worker-us-east-1a-vdr6m   Running   ecs.g6.large    us-east-1   us-east-1a   33m
openshift-machine-api   test-rx6ff-worker-us-east-1b-8l777   Running   ecs.g6.large    us-east-1   us-east-1b   33m
openshift-machine-api   test-rx6ff-worker-us-east-1b-plcst   Running   ecs.g6.large    us-east-1   us-east-1b   33m

/lgtm

elmiko · 2022-01-24T22:20:32Z

removing the hold here as we have a good test signal. thanks @kwoodson !
/hold cancel

openshift-ci · 2022-01-24T22:22:50Z

@elmiko: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/installer#5562 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

Bugzilla bug 2037209 has not been moved to the MODIFIED state.

Details

In response to this:

Bug 2037209: refactor for AlibabaResourceReference changes

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/test-infra repository.

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 13, 2022

openshift-ci bot requested review from JoelSpeed, alexander-demicev and jianli-wei January 13, 2022 22:47

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2022

elmiko mentioned this pull request Jan 13, 2022

Bug 2037209: refactor AlibabaResourceReference openshift/api#1096

Merged

elmiko force-pushed the refactor-resource-types branch from 313325b to eb21db0 Compare January 13, 2022 23:09

JoelSpeed reviewed Jan 14, 2022

View reviewed changes

elmiko force-pushed the refactor-resource-types branch 2 times, most recently from 3f585d6 to b7f49e3 Compare January 14, 2022 20:12

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2022

elmiko mentioned this pull request Jan 21, 2022

Bug 2037209: update alibaba for provider spec api changes openshift/installer#5562

Merged

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2022

elmiko force-pushed the refactor-resource-types branch from b0f5174 to bf65da9 Compare January 21, 2022 21:22

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2022

elmiko force-pushed the refactor-resource-types branch from bf65da9 to 1bf50a9 Compare January 21, 2022 21:37

update vendor for latest openshift/api changes

d773426

This is required to support the resource reference changes.

elmiko force-pushed the refactor-resource-types branch from 1bf50a9 to d773426 Compare January 24, 2022 14:43

alexander-demicev approved these changes Jan 24, 2022

View reviewed changes

openshift-ci bot assigned alexander-demicev Jan 24, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2022

openshift-ci bot mentioned this pull request Jan 24, 2022

Bug 2037209: vendor: update openshift/api to latest in preparation for alibaba machine provider changes openshift/installer#5563

Merged

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2022

openshift-ci bot assigned kwoodson Jan 24, 2022

openshift-merge-robot merged commit 00f93af into openshift:main Jan 24, 2022

openshift-ci bot mentioned this pull request Jan 25, 2022

Bug 2037209: [Alibaba] refactor for getResourceGroup ID from name #29

Merged

Conversation

elmiko commented Jan 13, 2022

Uh oh!

openshift-ci bot commented Jan 13, 2022

Uh oh!

elmiko commented Jan 13, 2022

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

elmiko commented Jan 14, 2022

Uh oh!

jianli-wei commented Jan 17, 2022

Uh oh!

elmiko commented Jan 17, 2022

Uh oh!

elmiko commented Jan 21, 2022

Uh oh!

elmiko commented Jan 21, 2022

Uh oh!

elmiko commented Jan 21, 2022

Uh oh!

elmiko commented Jan 24, 2022

Uh oh!

alexander-demicev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jan 24, 2022

Uh oh!

openshift-ci bot commented Jan 24, 2022

Uh oh!

kwoodson commented Jan 24, 2022

Uh oh!

elmiko commented Jan 24, 2022

Uh oh!

openshift-ci bot commented Jan 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments