AlibabaCloud: fix for bootstrap teardown removing slb backends #5682

kwoodson · 2022-03-04T18:24:11Z

AlibabaCloud provider has an issue with the recent terraform provider updates. When the bootstrap is torn down the SLB server backends are removed. This seems to be an issue in the terraform provider.

This code removes the teardown of the SLB for the bootstrap. This adds an additional stage to add all of the control plane nodes including the bootstrap. When the bootstrap is torn down the instance is removed automatically from the SLB (service load balancer).

kwoodson · 2022-03-04T18:27:51Z

DEBUG Time elapsed per stage:                      
DEBUG                 cluster: 2m9s                
DEBUG               bootstrap: 1m2s                
DEBUG slb_attach_controlplane: 7s                  
DEBUG      Bootstrap Complete: 14m29s              
DEBUG                     API: 4m51s               
DEBUG       Bootstrap Destroy: 55s                 
DEBUG       Cluster Operators: 12m18s              
INFO Time elapsed: 31m6s

kwoodson · 2022-03-04T18:27:58Z

/test e2e-alibaba

kwoodson · 2022-03-04T18:51:58Z

/test e2e-alibaba

kwoodson · 2022-03-04T20:13:17Z

@jsafrane The e2e-alibaba runs and is able to install with this PR.

staebler

The bootstrap VM is not being removed from the load balancers' backends.

data/data/alibabacloud/bootstrap/outputs.tf

bd233 · 2022-03-10T10:54:36Z

data/data/alibabacloud/slb_attach_controlplane/main.tf

Maybe a virtual server group alicloud_slb_server_group can be used, of course, this is not required

bd233 · 2022-03-10T11:01:55Z

This seems to be an issue in the terraform provider.

@kwoodson Have you located the cause of the issue? can you briefly describe it, thanks. I checked the resourceAliyunSlbBackendServersDelete, but found no problem

kwoodson · 2022-03-14T13:51:46Z

@bd233 The issue is when the teardown step of the bootstrap happens, all of the servers are removed from the SLB. This causes the cluster installation to fail as the oc commands no longer succeed. I'm able to manually add all control plane nodes back to the cluster and the cluster is functional but that is not something we can do during the install.

By adding this extra step to the installation then all nodes successfully get added and the bootstrap will be removed once the instance is released.

data/data/alibabacloud/bootstrap/main.tf

staebler · 2022-03-14T14:06:44Z

pkg/terraform/stages/alibabacloud/stages.go

This may have been overlooked from the message on my first review, but there is no code run for this stage during bootstrap destroy to remove the bootstrap VM from the backend of the slb.

@staebler I'm sorry for the late response. Just returning from PTO.

When I tested this code previously, the bootstrap release automatically removes the instance from the SLB list of backends. I'll test this morning and re-verify that behavior.

Is this handled implicitly by the bootstrap VM being deleted?

Do you think there is any value in explicitly removing the bootstrap VM from the load balancer? If we are going to rely on implicit behavior, then we at least need some good comments explaining why the bootstrap VM does not need to be explicitly removed from the load balancer. A year from now, I am going to look at this code and not remember why we are not removing from the load balancer: I am going to think it was an oversight.

So the effective difference is that the master backends are created after the bootstrap backends, right? If that works, I think that is a cleaner approach. I would like to see some comments on the stage explaining why the master backends need to be added after the bootstrap backends.

I also think that this will be a bit easier to revert once the bug is fixed in the terraform provider.

Running into the same issue now. Wondering if there is a race condition upon running the next stage. I've had a successful cluster install and a failure. The bootstrap destroy ran but the slb_attachment stage never ran.

DEBUG Plan: 0 to add, 0 to change, 11 to destroy. ... DEBUG Destroy complete! Resources: 11 destroyed. <-- loadbalancer backends were removed here --> DEBUG Loading Install Config... DEBUG Loading SSH Key... DEBUG Loading Base Domain... DEBUG Loading Platform... DEBUG Loading Cluster Name... DEBUG Loading Base Domain... DEBUG Loading Platform... DEBUG Loading Networking... DEBUG Loading Platform... DEBUG Loading Pull Secret... DEBUG Loading Platform... DEBUG Using Install Config loaded from state file INFO Waiting up to 40m0s (until 3:20PM) for the cluster at https://api.test.alicloud-dev.devcluster.openshift.com:6443 to initialize... W0316 14:40:59.738715 1143968 reflector.go:324] k8s.io/client-go/tools/watch/informerwatcher.go:146: failed to list *v1.ClusterVersion: Get "https://api.test.alicloud-dev.devcluster.openshift.com:6443/apis/config.openshift.io/v1/clusterversions?fieldSelector=metadata.name%3Dversion&limit=500&resourceVersion=0": dial tcp 47.253.132.94:6443: connect: connection refused

@staebler Any ideas why the next stage did not proceed?

Do you have a full log that you can show me? I suspect that you are not running the right binary if the "slb_attach_controlplane" stage did not run.

After some troubleshooting in cluster.go and watching the stages, I can see that the stages execute sequentially. The cluster, bootstrap, and then the attachment run successfully. Then the teardown occurs for the bootstrap which removes the backend servers from the SLB.

Is there a way to defer the attachment until the teardown of the bootstrap is complete?

You don't want to defer the attachment. You want the masters attached while bootstrapping is proceeding.

kwoodson · 2022-03-17T19:29:43Z

/test e2e-alibaba

kwoodson · 2022-03-17T19:36:48Z

@staebler I think this was in line with what you were suggesting previously. This mirrors gcp's functionality and is variable driven. This was successful for cluster creation. PTAL

DEBUG Time elapsed per stage:                      
DEBUG                 cluster: 2m12s               
DEBUG               bootstrap: 1m13s               
DEBUG slb_attach_controlplane: 6s                  
DEBUG      Bootstrap Complete: 12m24s              
DEBUG                     API: 6m31s               
DEBUG       Bootstrap Destroy: 55s                 
DEBUG       Cluster Operators: 13m6s               
INFO Time elapsed: 30m2s

staebler

I think this was in line with what you were suggesting previously.

Indeed!

Note that this will cause a merge conflict with #5715.

staebler · 2022-03-17T19:51:50Z

data/data/alibabacloud/slb_attach_controlplane/main.tf

Suggested change

load_balancer_id = "${element(var.slb_ids, count.index)}"

load_balancer_id = var.slb_ids[count.index]

staebler · 2022-03-17T19:54:09Z

data/data/alibabacloud/slb_attach_controlplane/main.tf

You don't need the quotes and dollar sign noise any more.

Suggested change

count = "${length(var.slb_ids)}"

count = length(var.slb_ids)

staebler · 2022-03-17T19:55:17Z

data/data/alibabacloud/slb_attach_controlplane/main.tf

Suggested change

server_id = "${backend_servers.value}"

server_id = backend_servers.value

staebler · 2022-03-17T19:56:12Z

data/data/alibabacloud/slb_attach_controlplane/variables.tf

Put this in data/data/alibabacloud/variables-alibabacloud.tf instead.

staebler · 2022-03-17T19:56:31Z

data/data/alibabacloud/slb_attach_controlplane/variables.tf

Add a newline.

staebler · 2022-03-17T19:57:04Z

pkg/asset/cluster/cluster.go

Remove this superfluous change.

staebler · 2022-03-17T19:57:30Z

pkg/terraform/stages/alibabacloud/stages.go

Suggested change

"github.com/hashicorp/terraform-exec/tfexec"

"github.com/openshift/installer/pkg/terraform"

"github.com/openshift/installer/pkg/terraform/providers"

"github.com/openshift/installer/pkg/terraform/stages"

alitypes "github.com/openshift/installer/pkg/types/alibabacloud"

"github.com/pkg/errors"

"github.com/hashicorp/terraform-exec/tfexec"

"github.com/pkg/errors"

"github.com/openshift/installer/pkg/terraform"

"github.com/openshift/installer/pkg/terraform/providers"

"github.com/openshift/installer/pkg/terraform/stages"

alitypes "github.com/openshift/installer/pkg/types/alibabacloud"

staebler · 2022-03-17T19:59:35Z

pkg/terraform/stages/alibabacloud/stages.go

Suggested change

// all of the controlplane backends to be removed from the SLB. This stage reattaches only the

// master controlplane nodes by calling apply with the ali_boostrap_lb set to false.

// all of the controlplane backends to be removed from the SLB. This stage attaches the

// master VMs and the bootstrap VMs as backend servers to the SLBs on create. Later,

// on bootstrap destroy, this stages removes only the bootstrap VM from the backend servers.

staebler

One small nit. Otherwise it looks good.

staebler · 2022-03-18T14:32:52Z

data/data/alibabacloud/slb_attach_controlplane/variables.tf

This description is not correct.

Suggested change

description = "The load balancer IDs of the bootstrap ECS."

description = "The IDs of the load balancers to which to attach the bootstrap and control plane VMs."

kwoodson · 2022-03-18T14:42:44Z

Latest changes are successful. I'll update one more time to address the nit.

DEBUG Time elapsed per stage:                      
DEBUG                 cluster: 2m18s               
DEBUG               bootstrap: 1m12s               
DEBUG slb_attach_controlplane: 8s                  
DEBUG      Bootstrap Complete: 13m11s              
DEBUG                     API: 6m25s               
DEBUG       Bootstrap Destroy: 57s                 
DEBUG       Cluster Operators: 10m28s              
INFO Time elapsed: 28m21s

staebler

/lgtm
/approve

openshift-ci · 2022-03-18T15:40:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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~~ [staebler]

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

openshift-bot · 2022-03-18T16:49:20Z

/retest-required