Bug 2037209: refactor AlibabaResourceReference #1096
Conversation
This change updates the AlibabaResourceReference struct to follow a discriminated union pattern. It now contains a `type` field which will indicate whether the resource reference is an ID or a list of tags. The `ID` and `Tags` fields have been converted to pointers in following with that pattern, and some constant values have been added for the various types.
This change converts the previous ResourceGroupID field into ResourceGroup. This change is being made to convert that field from a string into the AlibabaResourceReference type so that we can support multiple types of lookup for the resource group. There are some cases, notably during installation, where the ID may not be known, and in these cases the Tags will be used to find the resource group. This change to the structure gives us a method to detect this option when receiving the API object in the machine api actuator.
openshift-ci
bot
added
bugzilla/severity-high
|
@elmiko: This pull request references Bugzilla bug 2037209, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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/test-infra repository. |
|
/hold until we have a patch ready for the alibaba mapi actuator |
openshift-ci
bot
added
the
do-not-merge/hold
|
@elmiko: This pull request references Bugzilla bug 2037209, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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/test-infra repository. |
|
i suppose we don't necessarily need the hold, but i just want to make sure we don't break the alibaba actuator. |
|
This matches my expectations based on our conversation earlier |
|
I'd like the comments on the usage of the resource reference clarified and then this change lgtm. |
This change updates the usages of the AlibabaResourceReference type to include detailed descriptions of what happens when a tag search can return multiple results.
|
@deads2k updated to include more information about the cases when multiple results could be returned from a tag-based search. i also added a note to the resource struct itself, but it felt a little clunky. let me know what you think. |
|
this change is required by openshift/cluster-api-provider-alibaba#26 |
|
removing the hold as we have a patch ready to go |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/hold while we discuss some implementation details |
openshift-ci
bot
added
the
do-not-merge/hold
|
it sounds like adding the name field will help with the resource group issue we are tracking |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@elmiko: This pull request references Bugzilla bug 2037209, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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/test-infra repository. |
if there are zero matches for security group or vswitch, that will produce an error when creating the Machine. @JoelSpeed and i are still working through what should happen if there are zero resource groups found. i had thought it should also cause an error, but Joel is pointing out that perhaps some alibaba api queries are acceptable without a resource group. if that is the case, then a zero result on a resource group search wouldn't, by itself, indicate a failure. |
|
I believe for resource groups, if there are no matches, we would send no resource group ID within the request, in which case, the Alibaba API puts the resource in the default resource group, which may or may not be acceptable. I think if a user specifies tags/name to match a resource group, and we can't find it, that should be an error. I would only allow this fallback to the default resource group when no resource group configuration is given explicitly |
this would be in a situation where the user has omitted the that makes me wonder if we should make it a pointer? |
Correct. We should make sure that it is in fact optional, and if it is, make the field a pointer IMO |
|
i'm putting a hold here just to make sure we don't merge before we figure out this resource group issue |
openshift-ci
bot
added
the
do-not-merge/hold
This change makes the `VpcID`, `SecurityGroups`, `VSwitch`, and `ResourceGroups` fields required byu removing the `+optional` and `omitempty` annotations. This change is being made to reflect the best practices when deploying OpenShift on Alibaba Cloud. When deploying on OpenShift, only the VPC mode is allowed. This means that the user must specify a VPC ID or the instance creation will fail. When creating instances in VPC mode, the user must specify a vSwitch or the operation will fail.
|
@elmiko: This pull request references Bugzilla bug 2037209, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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/test-infra repository. |
|
i think we've resolved all the issue around these fields. i have updated the description, and my last change is essentially the following: This change makes the When deploying on OpenShift, only the VPC mode is allowed. This means i would like to get @JoelSpeed to review this again and i think we should consult Alibaba as well |
openshift-ci
bot
removed
the
do-not-merge/hold
| // A reference holds either the resource group ID, the resource name, or the required tags to search. | ||
| // When more than one resource group are returned for a search, an error will be produced and the Machine will not be created. | ||
| // Resource Groups do not support searching by tags. | ||
| ResourceGroup AlibabaResourceReference `json:"resourceGroup"` |
There was a problem hiding this comment.
Based on some of the conversations I've seen on slack, I think this should be optional
There was a problem hiding this comment.
is this still true?
the latest conversations seem to indicate that we can require this field, even for the default resource group.
|
Anyone could help checking why "build #1096,openshift/cluster-api-provider-alibaba#26" cannot succeed? |
|
@jianli-wei i think those errors are unrelated to this pr.
sorry, i see you are having it build with both PRs in place. it seems like maybe there is a misconfiguration, but i'm not sure why. |
|
@jianli-wei it looks like you might have launched a cluster from the ci-bot with those 2 PRs ? assuming that is the case, i think we need to update the installer as well before this will fully work. i am creating a PR to make sure that the installer is updated with these api changes. |
As alibaba provider spec has been added to the machine v1, we need a deepcopy so that we can utilize it in the installer. This change also adds the deepcopy file for machine v1.
|
@elmiko: all tests passed! 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/test-infra repository. I understand the commands that are listed here. |
|
this is also needed by openshift/installer#5562 |
|
/lgtm I believe this is ready to go now, @elmiko feel free to hold cancel if you're not aware of any further changes required here |
openshift-ci
bot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, elmiko, JoelSpeed 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 |
|
thanks @JoelSpeed and @deads2k , i am removing the hold as we have the follow up patches ready to go. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@elmiko: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged: 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 bug 2037209 has not been moved to the MODIFIED state. 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/test-infra repository. |
5 participants
This change updates the AlibabaResourceReference struct to follow a
discriminated union pattern. It now contains a
Typefield which willindicate whether the resource reference is an ID, a name, or a list of tags. The
IDandTagsfields have been converted to pointers in following withthat pattern, and some constant values have been added for the various
types.
Additionally, it also converts the previous ResourceGroupID field into
ResourceGroup. This change is being made to convert that field from a
string into the AlibabaResourceReference type so that we can support
multiple types of lookup for the resource group. There are some cases,
notably during installation, where the ID may not be known, and in these
cases the Tags will be used to find the resource group. This change to
the structure gives us a method to detect this option when receiving the
API object in the machine api actuator.
This change makes the
VpcID,SecurityGroups,VSwitch, andResourceGroupsfields required byu removing the+optionalandomitemptyannotations. This change is being made to reflect the bestpractices when deploying OpenShift on Alibaba Cloud.
When deploying on OpenShift, only the VPC mode is allowed. This means
that the user must specify a VPC ID or the instance creation will fail.
When creating instances in VPC mode, the user must specify a vSwitch or
the operation will fail.