Alibabacloud recommitted and fixed #5333

bd233 · 2021-10-27T04:21:31Z

This PR is based on #5018, and applies the modification of #5291, and fixes the issues raised in #5291.

openshift-ci · 2021-10-27T04:22:01Z

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.

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.

kwoodson · 2021-10-27T20:12:15Z

When I attempted to run this I received the following error:

ERROR Error: Invalid value for input variable      
ERROR                                              
ERROR   on /tmp/openshift-install-cluster-063195056/terraform.alibabacloud.auto.tfvars.json line 17: 
ERROR   17:   "ali_extra_tags": [                  
ERROR   18:     {                                  
ERROR   19:       "Value": "owned",                
ERROR   20:       "Key": "kubernetes.io/cluster/test-6w5nd" 
ERROR   21:     },                                 
ERROR   22:     {                                  
ERROR   23:       "Value": "ISV Integration",      
ERROR   24:       "Key": "OCP"                     
ERROR   25:     }                                  
ERROR   26:   ],                                   
ERROR                                              
ERROR The given value is not valid for variable "ali_extra_tags": map of string 
ERROR required.

I believe this is due to the mismatch in types. The pkg/tfvars/alibabacloud/alibabacloud.go has this:

ExtraTags             []alibabacloudprovider.Tag `json:"ali_extra_tags,omitempty"`

The terraform definition of ali_extra_tags is a map(string) in data/data/alibabacloud/variables-alibabacloud.tf.

The list of alibabacloudprovider.Tag mismatched on the terraform variable type map(string). This causes the merge that happens at the top of the terraform files to fail (bootstrap/main.tf, cluster/main.tf).

patrickdillon · 2021-10-27T21:37:06Z

/ok-to-test

patrickdillon · 2021-10-28T00:57:47Z

pkg/asset/manifests/template.go

The gofmt test is failing after removing this line. See: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_installer/5333/pull-ci-openshift-installer-master-gofmt/1453476031779311616

Can you run gofmt on this file or fix the formatting?

Yes, I can.

patrickdillon · 2021-10-28T00:58:27Z

Thanks for posting this. I am still working on reviewing these most recent changes.

bd233 · 2021-10-28T12:55:34Z

When I attempted to run this I received the following error:
ERROR Error: Invalid value for input variable      
ERROR                                              
ERROR   on /tmp/openshift-install-cluster-063195056/terraform.alibabacloud.auto.tfvars.json line 17: 
ERROR   17:   "ali_extra_tags": [                  
ERROR   18:     {                                  
ERROR   19:       "Value": "owned",                
ERROR   20:       "Key": "kubernetes.io/cluster/test-6w5nd" 
ERROR   21:     },                                 
ERROR   22:     {                                  
ERROR   23:       "Value": "ISV Integration",      
ERROR   24:       "Key": "OCP"                     
ERROR   25:     }                                  
ERROR   26:   ],                                   
ERROR                                              
ERROR The given value is not valid for variable "ali_extra_tags": map of string 
ERROR required.                                    
I believe this is due to the mismatch in types. The pkg/tfvars/alibabacloud/alibabacloud.go has this:
ExtraTags             []alibabacloudprovider.Tag `json:"ali_extra_tags,omitempty"`
The terraform definition of ali_extra_tags is a map(string) in data/data/alibabacloud/variables-alibabacloud.tf.

The list of alibabacloudprovider.Tag mismatched on the terraform variable type map(string). This causes the merge that happens at the top of the terraform files to fail (bootstrap/main.tf, cluster/main.tf).

Okay, I also found this problem, and I have submitted a commit to fix it.
In addition, I found some CI tests that failed, which I am dealing with them.

bd233 · 2021-10-28T13:09:50Z

/retest-required

kwoodson · 2021-10-28T23:09:36Z

@patrickdillon @bd233 The latest pull request has fixed the tagging issue previously noted. I was able to install using this PR.

patrickdillon · 2021-10-29T00:26:56Z

/retest

patrickdillon · 2021-10-29T00:58:24Z

All of the tests are failing/failing to build because you need a rebase. fmt was removed from pkg/tfvars/aws/aws.go in 6fc25a6 so this line is causing an error: 464c8ae#diff-9c4cee7cc715858abb38823e9409c61a65d8fdabedc104d5d69bf62d38e4bd1bR148

Feel free to add fmt back to that file after you rebase.

patrickdillon · 2021-10-29T01:35:01Z

It is not clear to me that the feedback regarding the destroy code has been completely address, in particular:
#5291 (comment) and #5291 (comment)

If there is going to be further work needed for the destroy code, I wonder if it might be a good idea to separate the destroy code into a separate PR so that we don't block the rest of the code in this PR.

patrickdillon · 2021-10-29T01:47:34Z

pkg/asset/cluster/tfvars.go

+		object := "bootstrap.ign"
+		signURL, err := client.GetOSSObjectSignURL(bucket, object)
+		if err != nil {
+			return err


You need to wrap all of these error messages before returning them.

patrickdillon · 2021-10-29T01:47:41Z

pkg/asset/cluster/tfvars.go

+
+		masters, err := mastersAsset.Machines()
+		if err != nil {
+			return err


You need to wrap all of these error messages before returning them.

patrickdillon · 2021-10-29T01:47:48Z

pkg/asset/cluster/tfvars.go

+		}
+		workers, err := workersAsset.MachineSets()
+		if err != nil {
+			return err


You need to wrap all of these error messages before returning them.

patrickdillon · 2021-10-29T01:47:55Z

pkg/asset/cluster/tfvars.go


+		natGatewayZones, err := client.ListEnhanhcedNatGatewayAvailableZones()
+		if err != nil {
+			return err


You need to wrap all of these error messages before returning them.

OK, I'll fix them. But I have a doubt, under what circumstances should error messages be wrapped? I notice that many of the above parts are returned directly. Understanding this, I can better fix such errors.

Thanks for fixing those. It is always a good idea to wrap errors, but in this case, it is necessary because there is a large function Generate that can return errors at many different points. If you don't wrap the error you will not know which part of the code generated the error if you are are trying to debug/troubleshoot.

bd233 · 2021-10-29T03:33:36Z

It is not clear to me that the feedback regarding the destroy code has been completely address, in particular: #5291 (comment) and #5291 (comment)

If there is going to be further work needed for the destroy code, I wonder if it might be a good idea to separate the destroy code into a separate PR so that we don't block the rest of the code in this PR.

I'm sorry to confuse you. I think I should explain our plan here:
What will be updated in the follow-up PR (we will submit this part next week):

Destroy cluster part, #5291 (comment) and #5291 (comment)
ResourceGroupID is required
Support the existing VPC and PrivateZone
Before deleting a cluster, check and turn off the deletion protection of SLB and ECS

Problems fixed in the current PR (will be submitted today):

use a well-known Tag for VSwitch and Security group in manifests

If there are any problems that block the merging or need us to solve, please let me know. thanks.

patrickdillon · 2021-10-29T16:49:41Z

data/data/manifests/openshift/cloud-creds-secret.yaml.template

You need to switch the next template back from {{- else if -> {{- if

This is causing all of the platform tests to fail (e.g. e2e-aws)

patrickdillon · 2021-10-29T16:49:48Z

data/data/manifests/openshift/cloud-creds-secret.yaml.template

You need to switch the next template back from {{- else if -> {{- if

patrickdillon · 2021-10-29T16:53:07Z

It is not clear to me that the feedback regarding the destroy code has been completely address, in particular: #5291 (comment) and #5291 (comment)
If there is going to be further work needed for the destroy code, I wonder if it might be a good idea to separate the destroy code into a separate PR so that we don't block the rest of the code in this PR.

I'm sorry to confuse you. I think I should explain our plan here: What will be updated in the follow-up PR (we will submit this part next week):

Destroy cluster part, #5291 (comment) and #5291 (comment)

ResourceGroupID is required

Support the existing VPC and PrivateZone

Before deleting a cluster, check and turn off the deletion protection of SLB and ECS

Problems fixed in the current PR (will be submitted today):

use a well-known Tag for VSwitch and Security group in manifests

If there are any problems that block the merging or need us to solve, please let me know. thanks.

This is fine. We want to merge as much of the present PR as we can as quickly as we can. This PR is still not ready to merge, so we can add to it. But If we get the rest of the PR ready to merge but destroy is not ready, we don't want to block this PR on destroy. Instead we could remove destroy and put it in a new PR and merge this one. We can figure that out next week.

kwoodson · 2021-10-29T16:53:42Z

@bd233 Here is an example from the output from the new tags:

...
        providerSpec:
          value:
            apiVersion: alibabacloudmachineproviderconfig.openshift.io/v1beta1
            credentialsSecret:
              name: alibabacloud-credentials
            dedicatedHostId: ""
            imageId: m-0xia2ofzd15xb3jb4ji2
            instanceName: test-6bh8w-worker
            instanceType: ecs.g6.large
            ioOptimized: optimized
            kind: AlibabaCloudMachineProviderConfig
            metadata:
              creationTimestamp: null
            ramRoleName: test-6bh8w-role-worker
            regionId: us-east-1
            resourceGroupId: rg-acfnxrgr6qtm3mq
            securityGroupId: ""
            securityGroups:
            - tags:
              - Key: kubernetes.io/cluster/test-6bh8w
                Value: owned
              - Key: OCP
                Value: ISV Integration
              - Key: Name
                Value: test-6bh8w-sg-worker
            systemDiskCategory: cloud_essd
            systemDiskSize: 120
            tags:
            - Key: kubernetes.io/cluster/test-6bh8w
              Value: owned
            - Key: OCP
              Value: ISV Integration
            userDataSecret:
              name: worker-user-data
            vSwitch:
              tags:
              - Key: kubernetes.io/cluster/test-6bh8w
                Value: owned
              - Key: OCP
                Value: ISV Integration
              - Key: Name
                Value: test-6bh8w-vswitch-us-east-1a
            zoneId: us-east-1a

patrickdillon · 2021-10-29T16:55:43Z

pkg/asset/cluster/tfvars.go

avaliabel -> available

bd233 · 2021-11-01T13:47:41Z

@patrickdillon @kwoodson I have removed the pkg/destroy commit and updated the vendor. Please continue to review the code.

openshift-bot · 2021-11-03T03:26:15Z

/retest-required