[WIP] Add Alibaba Cloud platform

[bd233]

No description provided.

[openshift-ci]

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.

[jcpowermac]

@bd233 can you rebase to fix the conflict

[jcpowermac]

/ok-to-test

[staebler]

I would recommend adding the ImageID back to the platform and obeying the environment variable override to enable testing.

I disagree. The image should be determined at the machine-pool level and not the platform level.

[kwoodson]

@staebler

I disagree. The image should be determined at the machine-pool level and not the platform level.

I trust your opinion as this is your realm of expertise. I was basing my recommendations on the other providers who store the image on the platform itself (vsphere, AWS, baremetal, ovirt, kubevirt, openstack).

The recent changes have broken the installer by not obeying the OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE environment variable. As the PR sits currently, the instlalation fails because the Alibaba images have not been published.

If I attempt to add a check for the existing image in the Alibaba section (in pkg/asset/rhcos/image.go) like the following produces a nil pointer dereference:

if config.Platform.AlibabaCloud.DefaultMachinePlatform.ImageID != "" {
      return config.Platform.AlibabaCloud.DefaultMachinePlatform.ImageID, nil
}

How do you recommend we solve the issue with obeying the environment variable override?

[patrickdillon]

The recent changes have broken the installer by not obeying the OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE environment variable. As the PR sits currently, the instlalation fails because the Alibaba images have not been published.

I don't think it's a bug in the recent changes but rather the the bootstrap image asset. To me it seems like the bootstrap image should have a dependency on the rhcos image asset, rather than calling osImage again here: https://github.com/openshift/installer/blob/master/pkg/asset/rhcos/bootstrap_image.go#L76

How do you recommend we solve the issue with obeying the environment variable override?

I'll take a look at whether we have a bug with this. What is the status with publishing the rhcos images?

[kwoodson]

@patrickdillon I spoke with @miabbott and he mentioned we are waiting on getting some accounts worked out and a bucket to store the images. We are or have been close on this for a while. Hopefully the trt team can make progress.

[staebler]

@staebler

I was basing my recommendations on the other providers who store the image on the platform itself (vsphere, AWS, baremetal, ovirt, kubevirt, openstack).

The other platforms--with the exception of AWS--define the image once at the platform level for the entire cluster and not for each machine pool. AWS is the only plaform where the user can set the OS image in either the platform or the machine pool. This was done in #3308 with little context for why the option of setting it in the machine pool was added.

My contention is that it is incorrect to provide the option to set it at the platform level and at the machine-pool level. It just makes the code more complicated. If the user wants to set it once for the entire cluster, then the user should use the default machine pool.

It looks to me like there is actually a bug in the Bootstrap Image asset. The only platform that actually uses that asset is the baremetal platform. The other platforms should either (1) generate nothing for the bootstrap image or (2) generate the actual image that is going to be used for bootstrapping, even though the asset is not used. If it were the latter, then the asset should respect the environment variable override.

[kwoodson]

My contention is that it is incorrect to provide the option to set it at the platform level and at the machine-pool level. It just makes the code more complicated. If the user wants to set it once for the entire cluster, then the user should use the default machine pool.

I completely agree with you. I don't think it is good to store it on the platform level and storing the image in the machinepool makes much more sense.

It looks to me like there is actually a bug in the Bootstrap Image asset. The only platform that actually uses that asset is the baremetal platform. The other platforms should either (1) generate nothing for the bootstrap image or (2) generate the actual image that is going to be used for bootstrapping, even though the asset is not used. If it were the latter, then the asset should respect the environment variable override.

The following code calls osImage and bypasses the Generate in image.go which checks for the environment variable.
https://github.com/openshift/installer/blob/master/pkg/asset/rhcos/bootstrap_image.go#L76

@patrickdillon @staebler
Would the proper solution be to check for the environment variable right there and return the variable if exists? Or should we refactor the check for the environment variable into its own function which both locations can call? I'd be fine with either approach and probably lean towards the function in an attempt to not repeat ourselves. I've been using the override to make progress with the MCO, the CCCMO, as well as the cluster-api-provider-alibaba. I can continue to hack this code but having an official path to override would be preferred.

[patrickdillon]

The following code calls osImage and bypasses the Generate in image.go which checks for the environment variable. https://github.com/openshift/installer/blob/master/pkg/asset/rhcos/bootstrap_image.go#L76

@patrickdillon @staebler Would the proper solution be to check for the environment variable right there and return the variable if exists? Or should we refactor the check for the environment variable into its own function which both locations can call? I'd be fine with either approach and probably lean towards the function in an attempt to not repeat ourselves. I've been using the override to make progress with the MCO, the CCCMO, as well as the cluster-api-provider-alibaba. I can continue to hack this code but having an official path to override would be preferred.

@kwoodson would you take a look at #5267 and see if that fixes the problem for you? I believe it should and is one way of fixing the issue.

[kwoodson]

@dongchen126 @bd233

I discovered today that the instance type for the master does not meet the required resources. This will need to be updated. I was able to achieve this with the following code. Please update the code so that masters default to have the xlarge instance type.

diff --git a/pkg/asset/machines/master.go b/pkg/asset/machines/master.go
index 03a96d59d..0bcd133a9 100644
--- a/pkg/asset/machines/master.go
+++ b/pkg/asset/machines/master.go
@@ -134,6 +134,13 @@ func (m *Master) Dependencies() []asset.Asset {
                &machine.Master{},
        }
 }
+func defaultMasterAlibabaCloudMachinePoolPlatform() alibabacloudtypes.MachinePool {
+       return alibabacloudtypes.MachinePool{
+               InstanceType:       "ecs.g6.xlarge",
+               SystemDiskCategory: alibabacloudtypes.DefaultDiskCategory,
+               SystemDiskSize:     120,
+       }
+}
 
 func awsDefaultMasterMachineTypes(region string, arch types.Architecture) []string {
        classes := awsdefaults.InstanceClasses(region, arch)
@@ -160,7 +167,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
        machines := []machineapi.Machine{}
        switch ic.Platform.Name() {
        case alibabacloudtypes.Name:
-               mpool := defaultAlibabaCloudMachinePoolPlatform()
+               mpool := defaultMasterAlibabaCloudMachinePoolPlatform()
                mpool.ImageID = string(*rhcosImage)
                mpool.Set(ic.Platform.AlibabaCloud.DefaultMachinePlatform)
                mpool.Set(pool.Platform.AlibabaCloud)

I also updated the instance names to match the generated names with an extra -. Please consider the following update:

diff --git a/data/data/alibabacloud/cluster/master/main.tf b/data/data/alibabacloud/cluster/master/main.tf
index f9b07e229..a8c7c15a2 100644
--- a/data/data/alibabacloud/cluster/master/main.tf
+++ b/data/data/alibabacloud/cluster/master/main.tf
@@ -11,8 +11,8 @@ resource "alicloud_instance" "master" {
   count             = length(var.vswitch_ids)
   resource_group_id = var.resource_group_id
 
-  host_name                  = "${local.prefix}-master${count.index}"
-  instance_name              = "${local.prefix}-master${count.index}"
+  host_name                  = "${local.prefix}-master-${count.index}"
+  instance_name              = "${local.prefix}-master-${count.index}"
   instance_type              = var.instance_type
   image_id                   = var.image_id
   internet_max_bandwidth_out = 0
@@ -21,7 +21,7 @@ resource "alicloud_instance" "master" {
   security_groups = [var.sg_id]
   role_name       = var.role_name
 
-  system_disk_name        = "${local.prefix}_sys_disk-master${count.index}"
+  system_disk_name        = "${local.prefix}_sys_disk-master-${count.index}"
   system_disk_description = local.description
   system_disk_category    = var.system_disk_category
   system_disk_size        = var.system_disk_size
@@ -29,7 +29,7 @@ resource "alicloud_instance" "master" {
   user_data = base64encode(var.user_data_ign)
   tags = merge(
     {
-      "Name" = "${local.prefix}-master${count.index}"
+      "Name" = "${local.prefix}-master-${count.index}"
     },
     var.tags,
   )

This change will require an update to the private zone terraform module here https://github.com/openshift/installer/blob/2226d559a2010d8564f3697af8cf741e029d453e/data/data/alibabacloud/cluster/privatezone/privatezone.tf#L65

[kwoodson]

@bd233 @dongchen126
With the latest changes to the naming, we need to fix the DNS names. They show up like the following

:

[kwoodson]

@patrickdillon @staebler
Would you be willing to outline the specifics to what you expect before we can get this merged? I believe we have discussed reformatting this pull request into specifics commits. If we could list out the expectations into a check list that might enable @bd233 and @dongchen126 to get this pull request up to your expectations.

I rebased today and fixed the go.mod dependencies and was able to install a cluster successfully (https://github.com/kwoodson/installer/tree/alibaba_v21). There will be a couple of more items but I want to set expectations for merge.

Thanks!

[kwoodson]

@bd233 @dongchen126 Feel free to rebase and resolve any conflicts. The correct dependencies can be found in the master branch's go.mod file here https://github.com/openshift/installer/blob/master/go.mod or I resolved them in a branch here https://github.com/kwoodson/installer/tree/alibaba_v21. This will simplify the process to build and test. Let me know if you have any questions.

[bd233]

@bd233 @dongchen126 Feel free to rebase and resolve any conflicts. The correct dependencies can be found in the master branch's go.mod file here https://github.com/openshift/installer/blob/master/go.mod or I resolved them in a branch here https://github.com/kwoodson/installer/tree/alibaba_v21. This will simplify the process to build and test. Let me know if you have any questions.

Thanks, I have updated and squash all commits.

[bd233]

@bd233 @dongchen126 With the latest changes to the naming, we need to fix the DNS names. They show up like the following :

Has been fixed.

[patrickdillon]

@patrickdillon @staebler Would you be willing to outline the specifics to what you expect before we can get this merged? I believe we have discussed reformatting this pull request into specifics commits. If we could list out the expectations into a check list that might enable @bd233 and @dongchen126 to get this pull request up to your expectations.

I rebased today and fixed the go.mod dependencies and was able to install a cluster successfully (https://github.com/kwoodson/installer/tree/alibaba_v21). There will be a couple of more items but I want to set expectations for merge.

Thanks!

I have reorganized the commits from this PR in #5291. I outlined a plan in that PR for how to best proceed with the code review of this very large PR. I believe once we reorganize the commits, it will make the PR easier to review, the installer team can then do a review, Alibaba can address immediate concerns and then we can merge.

[openshift-ci]

@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-openstack-provider-network	6809f3ea9a7517be2bd6b96814bd9e8ec21c0e3e	link	/test e2e-openstack-provider-network
ci/prow/e2e-openstack-byon	6809f3ea9a7517be2bd6b96814bd9e8ec21c0e3e	link	/test e2e-openstack-byon
ci/prow/e2e-aws-workers-rhel8	dd4241e	link	false	/test e2e-aws-workers-rhel8
ci/prow/e2e-aws-workers-rhel7	dd4241e	link	false	/test e2e-aws-workers-rhel7
ci/prow/e2e-aws-single-node	dd4241e	link	false	/test e2e-aws-single-node
ci/prow/e2e-crc	dd4241e	link	false	/test e2e-crc
ci/prow/okd-unit	dd4241e	link	true	/test okd-unit

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-ci]

@bd233: PR needs rebase.

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.

[jstuever]

/uncc

[kwoodson]

We have broken this pull request into two subsequent ones:
#5333 and
#5348