IBM Cloud Provider Scaffolding #4923

BobbyRadford · 2021-05-10T17:26:46Z

Initial scaffolding for the IBM Cloud provider

With this PR, a user should be able to:

openshift-install create install-config and follow CLI prompts to generate a bare bones install-config.yaml
openshift-install create install-config --dir=./existing-config where ./existing-config has a customized install-config.yaml with IBM Cloud platform and machine pool customizations.

This connects with the following enhancement doc PR: openshift/enhancements#773 Details about the supported install config customizations can be found there.

CI has not yet been implemented for the IBM Cloud platform, so this PR should not be merged until that is complete. The scaffolding code proposed here is fairly comprehensive, already containing a suite of unit tests. However, it is not fully done. Some validation on customizations is not yet implemented, specifically around byo VPC and subnets. This byo VPC and subnets may not end up in an MVP for the IBM Cloud platform anyway.

With this PR, I hope that we can get some clarity on direction and verification that we are doing this the "right way." Hopefully that can happen prior to CI being fully implemented.

openshift-ci · 2021-05-10T17:27:06Z

Hi @BobbyRadford. 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.

srcarrier · 2021-05-14T13:25:52Z

/assign @staebler

staebler

This look generally good. Most of the comments are minor. By biggest concern is with how we are dealing with the OS image.

pkg/asset/installconfig/ibmcloud/client.go

pkg/asset/installconfig/ibmcloud/validation.go

pkg/asset/installconfig/ssh.go

pkg/types/ibmcloud/ibmcloud.go

pkg/asset/installconfig/ibmcloud/client.go

pkg/types/ibmcloud/validation/platform.go

BobbyRadford · 2021-05-17T14:51:00Z

I'm going to slide in a commit to use some newer IBM Go SDKs. It will affect some things in the ibmcloud client. I think it's important to get these in the initial PR rather than later. From there, I will work through all of the review comments. Thanks @staebler

openshift-ci

@BobbyRadford: 2 invalid OWNERS files

In response to this:

Initial scaffolding for the IBM Cloud provider

With this PR, a user should be able to:

openshift-install create install-config and follow CLI prompts to generate a bare bones install-config.yaml

openshift-install create install-config --dir=./existing-config where ./existing-config has a customized install-config.yaml with IBM Cloud platform and machine pool customizations.

This connects with the following enhancement doc PR: openshift/enhancements#773 Details about the supported install config customizations can be found there.

CI has not yet been implemented for the IBM Cloud platform, so this PR should not be merged until that is complete. The scaffolding code proposed here is fairly comprehensive, already containing a suite of unit tests. However, it is not fully done. Some validation on customizations is not yet implemented, specifically around byo VPC and subnets. This byo VPC and subnets may not end up in an MVP for the IBM Cloud platform anyway.

With this PR, I hope that we can get some clarity on direction and verification that we are doing this the "right way." Hopefully that can happen prior to CI being fully implemented.

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.

pkg/asset/installconfig/ibmcloud/OWNERS

pkg/types/ibmcloud/OWNERS

openshift-ci

@BobbyRadford: 2 invalid OWNERS files

In response to this:

Initial scaffolding for the IBM Cloud provider

With this PR, a user should be able to:

openshift-install create install-config and follow CLI prompts to generate a bare bones install-config.yaml

openshift-install create install-config --dir=./existing-config where ./existing-config has a customized install-config.yaml with IBM Cloud platform and machine pool customizations.

This connects with the following enhancement doc PR: openshift/enhancements#773 Details about the supported install config customizations can be found there.

CI has not yet been implemented for the IBM Cloud platform, so this PR should not be merged until that is complete. The scaffolding code proposed here is fairly comprehensive, already containing a suite of unit tests. However, it is not fully done. Some validation on customizations is not yet implemented, specifically around byo VPC and subnets. This byo VPC and subnets may not end up in an MVP for the IBM Cloud platform anyway.

With this PR, I hope that we can get some clarity on direction and verification that we are doing this the "right way." Hopefully that can happen prior to CI being fully implemented.

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.

pkg/asset/installconfig/ibmcloud/OWNERS

pkg/types/ibmcloud/OWNERS

pkg/asset/installconfig/ibmcloud/client.go

BobbyRadford · 2021-05-27T14:17:36Z

I am going to drop in another commit here with some refactoring around resource groups. I don't want to just keep putting more and more into this PR, but I think this aligns with the mission of scaffolding.

In the commit...

I added another resource group field to the install config that will be needed for future work. This will allow for a distinct resource group for VPC resources.
I also added a "Name" suffix to the resource group fields to align with other providers.
Finally, an empty resource group name is handled. Eventually, through Terraform, we will create a new resource group if it is empty.

BobbyRadford · 2021-06-04T13:11:39Z

@staebler We were able to implement an automated way to upload and create an RHCOS image. Due to it depending on Terraform, which will come in a subsequent PR, is it OK for that automation piece to come later as well?

BobbyRadford · 2021-06-10T15:01:29Z

My last commit adds a build tag to conditionally show ibmcloud in the platform survey. By default IBM Cloud will now be a hidden platform. I have tested this out locally with several different scenarios and all seems to be good.

jeffnowicki · 2021-06-11T12:46:57Z

/retest

staebler

This looks good. My comments are almost all nits, except for the question about whether the CISInstanceCRN field should be optional.

Please squash down the commits into one or a few commits.

pkg/asset/installconfig/basedomain.go

pkg/asset/installconfig/ibmcloud/client.go

pkg/asset/installconfig/ibmcloud/validation.go

pkg/types/ibmcloud/ibmcloud.go

pkg/types/ibmcloud/platform.go

droslean · 2021-06-14T13:09:35Z

/test e2e-aws-fips

staebler · 2021-06-15T15:14:01Z

go generate ./pkg/types

diff --git a/data/data/install.openshift.io_installconfigs.yaml b/data/data/install.openshift.io_installconfigs.yaml
index 148e357..88dcb4d 100644
--- a/data/data/install.openshift.io_installconfigs.yaml
+++ b/data/data/install.openshift.io_installconfigs.yaml
@@ -1532,10 +1532,6 @@ spec:
                 description: IBMCloud is the configuration used when installing on
                   IBM Cloud.
                 properties:
-                  cisInstanceCRN:
-                    description: CISInstanceCRN is the Cloud Internet Services CRN
-                      of the base domain DNS zone.
-                    type: string
                   clusterOSImage:
                     description: ClusterOSImage is the name of the custom RHCOS image.
                     type: string
@@ -1597,7 +1593,6 @@ spec:
                       defined.
                     type: string
                 required:
-                - cisInstanceCRN
                 - clusterOSImage
                 - region
                 type: object

ibmcloud: add ibm cloud types support the new ibm cloud platform by adding required types ibmcloud: add initial assets Add ibmcloud assets to support the new ibmcloud platform. These changes are functional, but additional functionality will be built out over time ibmcloud: resolve linting issues ibmcloud: obtain the cisInstanceCRN for install-config The cisInstanceCRN field is derived from the user-provided baseDomain. It is needed for all DNS configuration. types: fix ibmcloud machinepool file name ibmcloud: rename platform ResourceGroupID field Rename the field `ResourceGroupID` in the Platform type to `ResourceGroup` ibmcloud: add initial metadata ibmcloud: add ClusterOSImage customization The ClusterOSImage field will allow the user to specify the custom RHCOS image to use for their cluster VSIs ibmcloud: add fields to Platform type Add the DefaultMachinePlatform, VPC, VPCResourceGroup, and Subnets fields to the Platform type. These are needed to fully define a cluster. ibmcloud: improve platform validation and tests Add in additional validation to the ibmcloud Platform. Also, add unit tests around that new validation. This is just a start and more validation and unit tests are required. ibmcloud: fix linting issues These issues were discovered using golangci-lint ibmcloud: use resource group name in install config Use the resource group name instead of ID in the install config. This will be more human friendly. The ID will also still be valid, but name will be preferred. ibmcloud: improve default resource group check Check for the default resource group based off of the 'default' field in the resource group struct. ibmcloud: fix typo ibmcloud: remove vpcResourceGroup and use vpc ID vpcResourceGroup is no longer needed if the vpc field holds the ID of the VPC instead of the name. ibmcloud: enforce clusterOSImage region The clusterOSImage refers to a custom image in a user's VPC. That image is regionally scoped and the region should be honored. Users should not be allowed to pick a custom image from a region that differes from the value of the region field in the install config. ibmcloud: use resourcev2 API The resourcev2 API should be used in place of v1. This is the most up-to-date and well supported version. ibmcloud: fix log message Co-Authored-By: Hidematsu Sueki <Hidematsu.Sueki@ibm.com> ibmcloud: add machinepool type and validation Add the MachinePool type for the IBM Cloud platform. Also include initial validation on the fields. ibmcloud: update survey version ibmcloud: use ibm go sdks instead of bluemix-go The ibm-go-sdk and corresponding service SDKs in the IBM GH org are more up-to-date and routinely supported. The old bluemix-go SDKs should no longer be used. update go mod update vendor fix: make validateVPCConfig a private function Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: improve log message for resource group not found Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: simplify subnet return statement Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: improve log message for vpc not found Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: images slice declaration Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: improve images range loop Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: typo in baseDomain help string Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: clarify resource group help message fix: use platformPath as variable name Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: ibmcloud platform reference Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: remove unnecessary conditionals in validation fix: check encryptionKey exists before validation fix: improve zone validation message Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: keep errors out of happy path fix: add index to subnets validation fix: create VPCResourceNotFoundError fix: use sets.String for contains fix: ibmcloud platform type comments fix: improve vpc config validation messages fix: add omitempty for encryptionKey Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: improve BootVolume copying Co-authored-by: Matthew Staebler <staebler@redhat.com> fix: rename MachinePool.Type to InstanceType fix: remove validateRegion fix: remove unnecessary context from client fix: remove context timeout in ic Platform fix: remove superfluous edit fix: update unit tests fix: move types used by cilent to same package fix: update OWNERS and OWNERS_ALIASES fix: improve client call to load services fix: improve GetZoneIDByName range loop fix: whitepsace in OWNERS fix: populate ibmcloud OWNERS_ALIASES fix: make cisServiceID a const Co-authored-by: Matthew Staebler <staebler@redhat.com> ibmcloud: refactor resource groups Allow users to have VPC resources in a different resource group from the cluster creation. This will enable CI and E2E testing along with making it easier to destroy clusters. This commit also adds a "Name" suffix to resource groups to align with other platforms naming convention. fix: error message format fix: update unit test ibmcloud: move ibmcloud to hidden platforms The IBMCloud platform will be in a developer preview for 4.9. As a result we will move it to the list of hidden platforms. This commit does that by default, but allows for it to show up in the survey via a build flag: 'ibmcloud'. ibmcloud: remove cisInstanceCRN field The cisInstanceCRN platform field is not needed. Though it is possible to manage a single DNS zone with multiple CIS instances, only one zone can be in the "Active" state at a time. As a result, we know which CIS instance to use based off of its managed zone's state. fix: address pr comments update codegen

staebler

/lgtm

openshift-ci · 2021-06-15T15:45:00Z

[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

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

staebler · 2021-06-15T16:21:38Z

/ok-to-test

openshift-ci · 2021-06-15T19:27:00Z

@BobbyRadford: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-openstack	`49b4da9`	link	`/test e2e-openstack`
ci/prow/e2e-metal-ipi-ovn-ipv6	`49b4da9`	link	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-aws-fips	`49b4da9`	link	`/test e2e-aws-fips`
ci/prow/e2e-crc	`49b4da9`	link	`/test e2e-crc`
ci/prow/e2e-aws-workers-rhel7	`49b4da9`	link	`/test e2e-aws-workers-rhel7`

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.

staebler · 2021-06-16T15:56:59Z

AWS is having issues with API throttling. I am overriding those tests and will use gcp success as the requirement instead.
/override ci/prow/e2e-aws
/override ci/prow/e2e-aws-upgrade
/test e2e-gcp
/hold

openshift-ci · 2021-06-16T15:58:16Z

@staebler: Overrode contexts on behalf of staebler: ci/prow/e2e-aws, ci/prow/e2e-aws-upgrade

In response to this:

AWS is having issues with API throttling. I am overriding those tests and will use gcp success as the requirement instead.
/override ci/prow/e2e-aws
/override ci/prow/e2e-aws-upgrade
/test e2e-gcp
/hold

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 · 2021-06-16T19:52:38Z

The e2e-gcp job passed.
/hold cancel

Miciah · 2021-07-07T18:03:10Z

pkg/asset/installconfig/ibmcloud/client.go

+	_, cancel := context.WithTimeout(ctx, 1*time.Minute)
+	defer cancel()


@BobbyRadford, what's the purpose of creating a child context with a timeout and throwing that context away? I see this _, cancel pattern repeated throughout this file, and I cannot figure out what its purpose is.

I think this is just a remnant of early development work that I intended to come back to. The context can and should be passed on to the different API calls made through the various IBM Cloud SDKs.

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 May 10, 2021

openshift-ci bot requested review from jstuever and rna-afk May 10, 2021 17:27

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

BobbyRadford force-pushed the scaffolding branch from 9b9f06c to b3f8774 Compare May 10, 2021 17:42

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

openshift-ci bot assigned staebler May 14, 2021

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

BobbyRadford force-pushed the scaffolding branch from b3f8774 to 6cc0dc6 Compare May 14, 2021 18:06

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

staebler reviewed May 14, 2021

View reviewed changes

BobbyRadford force-pushed the scaffolding branch 6 times, most recently from 217833e to 804ffe8 Compare May 17, 2021 20:52

BobbyRadford marked this pull request as ready for review May 18, 2021 14:11

openshift-ci bot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label May 18, 2021

openshift-ci bot reviewed May 18, 2021

View reviewed changes

pkg/asset/installconfig/ibmcloud/OWNERS Outdated Show resolved Hide resolved

pkg/types/ibmcloud/OWNERS Outdated Show resolved Hide resolved

BobbyRadford force-pushed the scaffolding branch from ad188a1 to 1397877 Compare May 18, 2021 14:21

openshift-ci bot reviewed May 18, 2021

View reviewed changes

pkg/asset/installconfig/ibmcloud/OWNERS Outdated Show resolved Hide resolved

pkg/types/ibmcloud/OWNERS Outdated Show resolved Hide resolved

openshift-ci bot added do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. and removed do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels May 18, 2021

BobbyRadford commented May 19, 2021

View reviewed changes

pkg/asset/installconfig/ibmcloud/client.go Show resolved Hide resolved

openshift-ci bot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label May 27, 2021

BobbyRadford force-pushed the scaffolding branch from adc6808 to 1db3445 Compare May 27, 2021 14:45

staebler reviewed Jun 12, 2021

View reviewed changes

BobbyRadford force-pushed the scaffolding branch from 3a01f3e to 49b4da9 Compare June 15, 2021 15:31

staebler approved these changes Jun 15, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 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 Jun 15, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2021

openshift-merge-robot merged commit 2a0d2f4 into openshift:master Jun 16, 2021

This was referenced Jun 18, 2021

Add IBMCloud manual mode openshift/cloud-credential-operator#356

Merged

Add CISInstanceCRN to IBMCloudPlatformStatus openshift/api#950

Closed

Add CISInstanceCRN to IBMCloudPlatformStatus openshift/api#951

Merged

Ibm cluster creation #5023

Merged

SzucsAti mentioned this pull request Jun 29, 2021

Add IBMCloud DNS support openshift/cluster-ingress-operator#630

Merged

Miciah reviewed Jul 7, 2021

View reviewed changes

BobbyRadford mentioned this pull request Jul 8, 2021

ibmcloud: Add Platform Provisioning Check #5063

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBM Cloud Provider Scaffolding #4923

IBM Cloud Provider Scaffolding #4923

BobbyRadford commented May 10, 2021

openshift-ci bot commented May 10, 2021

srcarrier commented May 14, 2021

staebler left a comment

BobbyRadford commented May 17, 2021

openshift-ci bot left a comment

openshift-ci bot left a comment

BobbyRadford commented May 27, 2021

BobbyRadford commented Jun 4, 2021

BobbyRadford commented Jun 10, 2021

jeffnowicki commented Jun 11, 2021

staebler left a comment

droslean commented Jun 14, 2021

staebler commented Jun 15, 2021

staebler left a comment

openshift-ci bot commented Jun 15, 2021

staebler commented Jun 15, 2021

openshift-ci bot commented Jun 15, 2021 •

edited

staebler commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

staebler commented Jun 16, 2021

Miciah Jul 7, 2021

BobbyRadford Jul 7, 2021

		_, cancel := context.WithTimeout(ctx, 1*time.Minute)
		defer cancel()

IBM Cloud Provider Scaffolding #4923

IBM Cloud Provider Scaffolding #4923

Conversation

BobbyRadford commented May 10, 2021

Initial scaffolding for the IBM Cloud provider

openshift-ci bot commented May 10, 2021

srcarrier commented May 14, 2021

staebler left a comment

Choose a reason for hiding this comment

BobbyRadford commented May 17, 2021

openshift-ci bot left a comment

Choose a reason for hiding this comment

Initial scaffolding for the IBM Cloud provider

openshift-ci bot left a comment

Choose a reason for hiding this comment

Initial scaffolding for the IBM Cloud provider

BobbyRadford commented May 27, 2021

BobbyRadford commented Jun 4, 2021

BobbyRadford commented Jun 10, 2021

jeffnowicki commented Jun 11, 2021

staebler left a comment

Choose a reason for hiding this comment

droslean commented Jun 14, 2021

staebler commented Jun 15, 2021

staebler left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 15, 2021

staebler commented Jun 15, 2021

openshift-ci bot commented Jun 15, 2021 • edited

staebler commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

staebler commented Jun 16, 2021

Miciah Jul 7, 2021

Choose a reason for hiding this comment

BobbyRadford Jul 7, 2021

Choose a reason for hiding this comment

openshift-ci bot commented Jun 15, 2021 •

edited