Alibaba: support existing VPC, VSwitchs and PrivateZone #5379

bd233 · 2021-11-12T05:06:41Z

This PR is based on #5378
If the user wants the Installer to install the cluster in an existing network, it is now supported.

openshift-ci · 2021-11-12T05:07:19Z

Hi @bd233. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

staebler

In the future, please refrain from adding so many separate parts in one commit.
For example, it would have been better to have one commit each for the existing VPC, the existing VSwitches, and the existing private zones. They could also be done as separate PRs, which would make it much faster to get approval.

data/data/alibabacloud/cluster/outputs.tf

pkg/asset/installconfig/alibabacloud/alibabacloud.go

pkg/types/alibabacloud/platform.go

data/data/alibabacloud/cluster/outputs.tf

data/data/alibabacloud/cluster/dns/variables.tf

staebler · 2021-11-16T03:34:18Z

pkg/types/alibabacloud/platform.go

@@ -12,6 +12,27 @@ type Platform struct {
 	// +optional
 	ResourceGroupID string `json:"resourceGroupID"`

+	// VpcID is the ID of an already existing VPC where the cluster should be installed.
+	// If empty, a new VPC will created for the cluster.
+	// Destroying the cluster using installer will delete this VPC.


Why would the cluster destroy delete the VPC? Is that because the VPC must be in the existing resource group? If so, then we are missing some validation. If not, then I don't think that the cluster destroy should delete the existing VPC.

I think there is no clear description here. If VpcID is empty, a new VPC will be created, and released when the cluster is destroyed.

My suggestion is to remove the language around deleting the VPC.

// VpcID is the ID of the VPC into which the cluster's machines will be created. // Leave the VPC ID unset to have the installer create a VPC on your behalf.

pkg/types/alibabacloud/platform.go

staebler · 2021-11-16T03:37:03Z

pkg/asset/installconfig/alibabacloud/validation.go

+func validateVSwitchs(client *Client, ic *types.InstallConfig, path *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if ic.AlibabaCloud.VpcID == "" {


This should be checked via static validation in pkg/types/alibabacloud/validation/platform.go.

staebler · 2021-11-16T03:39:23Z

pkg/types/alibabacloud/platform.go

+	// +optional
+	VpcID string `json:"vpcID,omitempty"`
+
+	// VSwitchIDs is the ID list of already existing VSwitchs where the master should be created.


Globally, replace VSwitchs with VSwitches.

Suggested change

// VSwitchIDs is the ID list of already existing VSwitchs where the master should be created.

// VSwitchIDs is the ID list of already existing VSwitches where the masters should be created.

staebler · 2021-11-16T03:40:25Z

pkg/types/alibabacloud/platform.go

+	// +optional
+	VpcID string `json:"vpcID,omitempty"`
+
+	// VSwitchIDs is the ID list of already existing VSwitchs where the master should be created.


The vswitches are not just for the masters.

bd233 · 2021-11-16T14:00:43Z

@staebler I have updated, please continue to review

jstuever · 2021-11-16T22:03:52Z

/uncc

patrickdillon · 2021-11-22T20:31:45Z

/ok-to-test

data/data/alibabacloud/cluster/dns/privatezone.tf

staebler · 2021-11-22T20:48:56Z

data/data/alibabacloud/cluster/master/variables.tf

  type        = list(string)
-  description = "The VSwitch IDs of the master ECS. Example: [vsw-xxx1, vsw-xxx2, vsw-xxx3]"
+  description = "The availability zones in which to create the masters. The length of this list must match master_count."


Suggested change

description = "The availability zones in which to create the masters. The length of this list must match master_count."

description = "The availability zones in which to create the masters. The length of this list must match instance_count."

@bd233 Please update, thanks.

data/data/alibabacloud/cluster/dns/privatezone.tf

staebler · 2021-11-22T21:00:02Z

data/data/alibabacloud/cluster/vpc/vpc.tf

-resource "alicloud_vswitch" "vswitchs" {
-  count = length(var.zone_ids)
+resource "alicloud_vswitch" "vswitches" {
+  count = length(var.vswitch_ids) == 0 ? length(var.zone_ids) : 0


This count is not correct when the installer is creating the vSwitches. Using length(var.zone_ids) assumes that the zones in var.zone_ids are unique. If the master replica count is greater than the number of availability zones used, then this will create multiple vSwitches in some of the zones, and those extras will be omitted from the mapping from zone ID to vSwitch ID.

I can solve this by length(toset(var.zone_ids)), but I think this may not be the most suitable solution. I should ensure that the zones in var_zone_ids is unique, at the same time, the length of ali_zone_ids need not be equal master_count. WDYT?

Yes, var.zone_ids should be a distinct list of availability zones. It should also include zones for the workers, which appears to be missing currently.

Yes, thank you for your reminder, the worker zones is missing currently, I will create a new PR to update.

Yes, var.zone_ids should be a distinct list of availability zones. It should also include zones for the workers, which appears to be missing currently.

@staebler #5438

staebler · 2021-11-22T21:04:34Z

pkg/asset/installconfig/alibabacloud/validation.go

+	if ic.AlibabaCloud.VpcID != "" {
+		allErrs = append(allErrs, validateVpc(client, ic, path)...)
+	}
+	if ic.AlibabaCloud.VSwitchIDs != nil {


Suggested change

if ic.AlibabaCloud.VSwitchIDs != nil {

if len(ic.AlibabaCloud.VSwitchIDs) != 0 {

staebler · 2021-11-22T21:35:24Z

pkg/types/alibabacloud/platform.go

+
+	// VSwitchIDs is the ID list of already existing VSwitches where cluster resources will be created.
+	// If empty, the new VSwitches will created for the cluster,
+	// and destroying the cluster using installer will delete these VSwitches.


See above for the comment regarding deleting the VPC.

staebler · 2021-11-22T21:41:14Z

pkg/types/alibabacloud/platform.go

+	// +optional
+	VpcID string `json:"vpcID,omitempty"`
+
+	// VSwitchIDs is the ID list of already existing VSwitches where cluster resources will be created.


I assume that since the vSwitch is part of the VPC, that the vSwitches can only be user-provided if the VPC is also user-provided. If so, then there should (1) be text to that affect in this description, (2) be validation that the VPC ID is specified when the vSwitch IDs are specified, and (3) be validation that the vSwitches are part of the VPC.

Regarding (2) and (3), relevant validation have already been made, and I will add text in this description

that the vSwitches can only be user-provided if the VPC is also user-provided.

I disagree with this point. I think it’s okay for the user to only provide the VPC ID, and the installer will create the VSwitches

Sorry, I thought that I had deleted parts (2) and (3) after re-checking that the validation was indeed already there.

that the vSwitches can only be user-provided if the VPC is also user-provided.

I disagree with this point. I think it’s okay for the user to only provide the VPC ID, and the installer will create the VSwitches

Yes, I agree that it is OK to use a user-provided VPC and installer-generated VSwitches. My point is the converse. If the user provides the VSwitches, then they must all provide the VPC. I would like to call that out specifically in the description for the field so that the user does not assume that the installer will deduce the VPC to use from the VSwitches (which the installer could presumably do).

staebler · 2021-11-22T21:50:08Z

data/data/alibabacloud/cluster/vpc/vpc.tf

@@ -4,9 +4,17 @@ locals {
  prefix            = var.cluster_id
  newbits           = tonumber(split("/", var.vpc_cidr_block)[1]) < 16 ? 20 - tonumber(split("/", var.vpc_cidr_block)[1]) : 4


nit not related to this PR: This would be a lot easier to read if it used the max function.

Suggested change

newbits = tonumber(split("/", var.vpc_cidr_block)[1]) < 16 ? 20 - tonumber(split("/", var.vpc_cidr_block)[1]) : 4

newbits = max(4, 20 - tonumber(split("/", var.vpc_cidr_block)[1]))

Okey, thanks

staebler · 2021-11-22T21:50:57Z

pkg/types/alibabacloud/validation/platform.go

+		allErrs = append(allErrs, field.Required(fldPath.Child("vpcID"), "when using existing VSwitches, an existing VPC must be used"))
+	}
+
+	vswitchIDs := []string{}


Use a map instead of a list.

staebler · 2021-11-22T21:51:31Z

pkg/types/alibabacloud/validation/platform.go

+			}
+		}
+		if isRepeated {
+			allErrs = append(allErrs, field.Duplicate(fldPath.Child("vswitchIDs"), vswitchID))


Include the index to the duplicate in the field path.

Still need to include the index in the field path.

Suggested change

allErrs = append(allErrs, field.Duplicate(fldPath.Child("vswitchIDs"), vswitchID))

allErrs = append(allErrs, field.Duplicate(fldPath.Child("vswitchIDs").Index(i), vswitchID))

staebler · 2021-11-22T22:00:48Z

pkg/types/alibabacloud/platform.go

@@ -7,11 +7,32 @@ type Platform struct {

 	// ResourceGroupID is the ID of an already existing resource group where the cluster should be installed.
 	// This resource group must be empty with no other resources when trying to use it for creating a cluster.


Is this strictly true? Or could the user-supplied VPC be in the resource group?

In fact, it has no effect if it is not empty. we will not query and destroy resources through resource group. So this description is incorrect, I will delete it.

Will the destroyer delete the resource group if it is user-supplied?

staebler · 2021-11-22T22:03:04Z

pkg/types/alibabacloud/metadata.go

+	VpcID           string   `json:"vpcID"`
+	VSwitchIDs      []string `json:"vswitchIDs"`
+	PrivateZoneID   string   `json:"privateZoneID"`


In what way are these values going to be used while destroying the cluster?

I have to admit that these are not used, I will delete it.

bd233 · 2021-11-29T10:08:48Z

@staebler I should have updated everything, please continue to review

openshift-bot · 2021-12-14T15:24:49Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T16:13:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T16:25:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T18:26:05Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T19:49:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T21:01:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T23:13:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-14T23:38:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T00:02:50Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T00:14:58Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

kwoodson · 2021-12-15T00:42:17Z

@staebler Looks like an error in the AWS terraform generation. Any ideas on this one?

level=error
level=error msg=Error: Provider produced inconsistent result after apply
level=error
level=error msg=When applying changes to module.vpc.aws_vpc_dhcp_options_association.main[0],
level=error msg=provider "registry.terraform.io/-/aws" produced an unexpected new value for
level=error msg=was present, but now absent.
level=error
level=error msg=This is a bug in the provider, which should be reported in the provider's own
level=error msg=issue tracker.
level=error
level=fatal msg=failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply Terraform: failed to complete the change

mtulio · 2021-12-15T01:08:39Z

@kwoodson this looks like a possible flake similar to what was reported in hashicorp/terraform-provider-aws#16142
/retest-required

openshift-bot · 2021-12-15T01:26:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T02:29:51Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

mtulio · 2021-12-15T02:35:54Z

/test e2e-aws

openshift-bot · 2021-12-15T02:45:00Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T03:08:50Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T03:20:52Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T03:46:15Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T03:59:08Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T04:12:12Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-12-15T04:13:09Z

@bd233: 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-libvirt	`4be9a0b`	link	false	`/test e2e-libvirt`
ci/prow/e2e-aws-single-node	`4be9a0b`	link	false	`/test e2e-aws-single-node`
ci/prow/e2e-aws-workers-rhel7	`4be9a0b`	link	false	`/test e2e-aws-workers-rhel7`
ci/prow/okd-e2e-aws	`4be9a0b`	link	false	`/test okd-e2e-aws`
ci/prow/e2e-aws-fips	`4be9a0b`	link	false	`/test e2e-aws-fips`
ci/prow/e2e-crc	`4be9a0b`	link	false	`/test e2e-crc`

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

openshift-bot · 2021-12-15T04:25:10Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T04:37:10Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T04:49:11Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T05:01:11Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T05:37:14Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T06:01:09Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-15T06:25:14Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci bot requested review from jstuever and patrickdillon November 12, 2021 05:07

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 12, 2021

bd233 mentioned this pull request Nov 12, 2021

alibaba: implement cluster destroy #5348

Merged

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

bd233 force-pushed the existing-network branch from 2565ab6 to d2f9488 Compare November 16, 2021 02:47

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

staebler reviewed Nov 16, 2021

View reviewed changes

bd233 force-pushed the existing-network branch from d2f9488 to 8be8890 Compare November 16, 2021 13:59

openshift-ci bot removed the request for review from jstuever November 16, 2021 22:03

bd233 force-pushed the existing-network branch from 8be8890 to 642ce8d Compare November 17, 2021 10:21

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

bd233 force-pushed the existing-network branch from 642ce8d to 7fbdd01 Compare November 22, 2021 08:17

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

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 22, 2021

staebler reviewed Nov 22, 2021

View reviewed changes

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

bd233 force-pushed the existing-network branch from 7fbdd01 to c14bbf4 Compare November 24, 2021 11:41

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 24, 2021

bd233 force-pushed the existing-network branch from c14bbf4 to 291cdf3 Compare November 26, 2021 07:41

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

openshift-merge-robot merged commit 1ecfddc into openshift:master Dec 15, 2021

	// VSwitchIDs is the ID list of already existing VSwitchs where the master should be created.
	// VSwitchIDs is the ID list of already existing VSwitches where the masters should be created.

	description = "The availability zones in which to create the masters. The length of this list must match master_count."
	description = "The availability zones in which to create the masters. The length of this list must match instance_count."

	if ic.AlibabaCloud.VSwitchIDs != nil {
	if len(ic.AlibabaCloud.VSwitchIDs) != 0 {

		@@ -4,9 +4,17 @@ locals {
		prefix = var.cluster_id
		newbits = tonumber(split("/", var.vpc_cidr_block)[1]) < 16 ? 20 - tonumber(split("/", var.vpc_cidr_block)[1]) : 4

	newbits = tonumber(split("/", var.vpc_cidr_block)[1]) < 16 ? 20 - tonumber(split("/", var.vpc_cidr_block)[1]) : 4
	newbits = max(4, 20 - tonumber(split("/", var.vpc_cidr_block)[1]))

	allErrs = append(allErrs, field.Duplicate(fldPath.Child("vswitchIDs"), vswitchID))
	allErrs = append(allErrs, field.Duplicate(fldPath.Child("vswitchIDs").Index(i), vswitchID))

		@@ -7,11 +7,32 @@ type Platform struct {

		// ResourceGroupID is the ID of an already existing resource group where the cluster should be installed.
		// This resource group must be empty with no other resources when trying to use it for creating a cluster.

Alibaba: support existing VPC, VSwitchs and PrivateZone #5379

Alibaba: support existing VPC, VSwitchs and PrivateZone #5379

Conversation

bd233 commented Nov 12, 2021

openshift-ci bot commented Nov 12, 2021

staebler left a 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

bd233 commented Nov 16, 2021

jstuever commented Nov 16, 2021

patrickdillon commented Nov 22, 2021

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

bd233 commented Nov 29, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 14, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

kwoodson commented Dec 15, 2021

mtulio commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

mtulio commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-ci bot commented Dec 15, 2021 • edited Loading

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-bot commented Dec 15, 2021

openshift-ci bot commented Dec 15, 2021 •

edited

Loading