CORS-2271: IBMCloud: Add DNS Service support - installconfig #6255

cjschaef · 2022-08-23T13:45:39Z

Add support for DNS Service on IBM Cloud in IPI. This allows lookup
of resources based on PublishStrategy, CIS (External) vs. DNS
(Internal). Handles service and zone lookup, and configuring DNS
and Infrastructure.

openshift-ci · 2022-08-23T13:46:16Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2022-08-25T13:29:40Z

@cjschaef: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test aro-unit
/test e2e-aws
/test e2e-aws-upi
/test e2e-azure
/test e2e-azure-upi
/test e2e-gcp
/test e2e-gcp-upi
/test e2e-metal-ipi-ovn-ipv6
/test e2e-vsphere
/test e2e-vsphere-upi
/test gofmt
/test golint
/test govet
/test images
/test okd-images
/test okd-unit
/test okd-verify-codegen
/test openstack-manifests
/test shellcheck
/test tf-lint
/test unit
/test verify-codegen
/test verify-vendor
/test yaml-lint

The following commands are available to trigger optional jobs:

/test e2e-alibaba
/test e2e-aws-disruptive
/test e2e-aws-fips
/test e2e-aws-imdsv2
/test e2e-aws-proxy
/test e2e-aws-shared-vpc
/test e2e-aws-single-node
/test e2e-aws-upgrade
/test e2e-aws-upi-proxy
/test e2e-aws-workers-rhel8
/test e2e-azure-resourcegroup
/test e2e-azure-shared-vpc
/test e2e-azurestack
/test e2e-azurestack-upi
/test e2e-crc
/test e2e-gcp-shared-vpc
/test e2e-gcp-upgrade
/test e2e-gcp-upi-xpn
/test e2e-ibmcloud
/test e2e-libvirt
/test e2e-metal-assisted
/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-dualstack
/test e2e-metal-ipi-swapped-hosts
/test e2e-metal-ipi-virtualmedia
/test e2e-metal-single-node-live-iso
/test e2e-nutanix
/test e2e-openstack
/test e2e-openstack-kuryr
/test e2e-openstack-parallel
/test e2e-openstack-proxy
/test e2e-openstack-upi
/test e2e-ovirt
/test e2e-vsphere-zones
/test okd-e2e-aws-ovn
/test okd-e2e-aws-upgrade
/test okd-e2e-gcp
/test okd-e2e-gcp-upgrade
/test okd-e2e-vsphere
/test tf-fmt

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-installer-master-aro-unit
pull-ci-openshift-installer-master-e2e-aws
pull-ci-openshift-installer-master-e2e-azure
pull-ci-openshift-installer-master-e2e-gcp
pull-ci-openshift-installer-master-e2e-ibmcloud
pull-ci-openshift-installer-master-e2e-libvirt
pull-ci-openshift-installer-master-e2e-metal-assisted
pull-ci-openshift-installer-master-e2e-metal-ipi
pull-ci-openshift-installer-master-e2e-openstack
pull-ci-openshift-installer-master-e2e-ovirt
pull-ci-openshift-installer-master-e2e-vsphere
pull-ci-openshift-installer-master-gofmt
pull-ci-openshift-installer-master-golint
pull-ci-openshift-installer-master-govet
pull-ci-openshift-installer-master-images
pull-ci-openshift-installer-master-okd-e2e-aws-ovn
pull-ci-openshift-installer-master-okd-images
pull-ci-openshift-installer-master-okd-unit
pull-ci-openshift-installer-master-okd-verify-codegen
pull-ci-openshift-installer-master-shellcheck
pull-ci-openshift-installer-master-tf-fmt
pull-ci-openshift-installer-master-tf-lint
pull-ci-openshift-installer-master-unit
pull-ci-openshift-installer-master-verify-codegen
pull-ci-openshift-installer-master-verify-vendor
pull-ci-openshift-installer-master-yaml-lint

Details

In response to this:

/retest aro-unit
/retest e2e-ibmcloud
/retest golint
/retest govet
/retest okd-unit
/retest shellcheck
/retest tf-fmt
/retest unit
/retest verify-vendor
/retest yaml-lint

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.

cjschaef · 2022-08-25T13:30:18Z

/test aro-unit
/test e2e-ibmcloud
/test golint
/test govet
/test okd-unit
/test shellcheck
/test tf-fmt
/test unit
/test verify-vendor
/test yaml-lint

cjschaef · 2022-08-25T14:59:14Z

/test golint
/test govet
/test okd-unit
/test tf-fmt
/test e2e-ibmcloud

cjschaef · 2022-08-25T19:03:23Z

/test gofmt
/test golint
/test images
/test okd-unit
/test okd-verify-codegen
/test tf-fmt
/test unit
/test verify-codegen

cjschaef · 2022-08-25T19:32:44Z

/retitle CORS-2255: IBMCloud: Add DNS Service support - installconfig

jeffnowicki · 2022-08-25T20:15:05Z

/retitle CORS-2271: IBMCloud: Add DNS Service support - installconfig

jeffnowicki · 2022-08-25T20:16:41Z

/retitle CORS-2271: IBMCloud: Add DNS Service support - installconfig

cjschaef · 2022-08-25T21:15:01Z

/test images
/test verify-codegen

cjschaef · 2022-08-25T22:33:18Z

The core linting tests all finally got kicked off properly and have passed.

Will see about getting the remaining e2e tests to at least run now.

/retest

patrickdillon · 2022-08-26T14:10:26Z

/approve

openshift-ci · 2022-08-26T14:16:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

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

rvanderp3

I left a few questions, overall looks good.

rvanderp3 · 2022-08-26T14:41:48Z

pkg/asset/installconfig/ibmcloud/client.go

Suggested change

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

"github.com/pkg/errors"

"github.com/pkg/errors"

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

rvanderp3 · 2022-08-26T14:54:47Z

pkg/asset/installconfig/ibmcloud/client.go

could we make a new type here for instanceType and create constants for "DNS" and "CIS"?

Sure, I can do that.

rvanderp3 · 2022-08-26T15:01:13Z

pkg/asset/installconfig/ibmcloud/dns.go

Suggested change

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

"github.com/pkg/errors"

"github.com/pkg/errors"

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

rvanderp3 · 2022-08-26T15:02:02Z

pkg/asset/installconfig/ibmcloud/metadata.go

Suggested change

"github.com/IBM/go-sdk-core/v5/core"

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

"github.com/IBM/go-sdk-core/v5/core"

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

rvanderp3 · 2022-08-26T15:05:18Z

pkg/asset/installconfig/ibmcloud/metadata.go

Do we want to allow access of this variable outside the protection of the mutex?

I think that is acceptable, since if the value isn't set, it get locked by the mutex after this check, setting it, or waiting for another thread to set it and then rechecking if it is set.

I thought this method was potentially best given the last time I ran into this when fixing the deadlock bug, per this review comment
#6241 (review)

Not opposed to making a change, but just wasn't sure which path is safer/best.

rvanderp3 · 2022-08-26T15:08:38Z

pkg/asset/installconfig/ibmcloud/metadata.go

is the purpose of the mutex in DNSInstance to guard against concurrent modification of m.dnsInstance? if so, could this public function possibly negate that protection?

That's a good point, although the public function allows access to test the metadata functions, perhaps I can work out another method for performing the testing of the functions in metadata without having to have public "set" functions.

I removed the SetDNSInstance function for now, will handle necessary changes in upcoming unit test PR for metadata.

rvanderp3 · 2022-08-26T18:43:25Z

/retest

openshift-ci-robot · 2022-08-26T19:18:51Z

/retest-required

Remaining retests: 2 against base HEAD 82dfb36 and 8 for PR HEAD f52ee7c73da4f8c19d4b89ddd82332338afa77c7 in total

rvanderp3 · 2022-08-26T20:48:04Z

/retest

openshift-ci-robot · 2022-08-26T20:57:24Z

/retest-required

Remaining retests: 1 against base HEAD 82dfb36 and 7 for PR HEAD f52ee7c73da4f8c19d4b89ddd82332338afa77c7 in total

openshift-ci-robot · 2022-08-26T21:08:29Z

/retest-required

Remaining retests: 0 against base HEAD 82dfb36 and 6 for PR HEAD f52ee7c73da4f8c19d4b89ddd82332338afa77c7 in total

openshift-ci-robot · 2022-08-27T02:08:42Z

/retest-required

Remaining retests: 2 against base HEAD abf77d9 and 5 for PR HEAD f52ee7c73da4f8c19d4b89ddd82332338afa77c7 in total

openshift-ci-robot · 2022-08-27T03:05:59Z

/retest-required

Remaining retests: 1 against base HEAD abf77d9 and 4 for PR HEAD f52ee7c73da4f8c19d4b89ddd82332338afa77c7 in total

openshift-ci-robot · 2022-08-27T03:29:23Z

/retest-required

Remaining retests: 0 against base HEAD abf77d9 and 3 for PR HEAD f52ee7c73da4f8c19d4b89ddd82332338afa77c7 in total

jeffnowicki · 2022-08-28T15:25:41Z

/retest

jeffnowicki · 2022-08-28T20:26:15Z

/test golint

jeffnowicki · 2022-08-29T12:16:04Z

/test golint

cjschaef · 2022-08-29T13:27:54Z

Pushed up fix to addressing missing comments for consts in golint

 INFO[2022-08-29T12:24:10Z] /go/src/github.com/openshift/installer/pkg/asset/installconfig/ibmcloud/client.go:61:2: exported const CISInstanceType should have comment (or a comment on this block) or be unexported
Found 1 lint suggestions; failing.

rvanderp3 · 2022-08-29T14:03:37Z

/retest

cjschaef · 2022-08-29T17:31:33Z

/hold
I need to rebase since a conflict has popped up with #6157 merging.

I will have to rebase to resolve the conflict when I can.

Add support for DNS Service on IBM Cloud in IPI. This allows lookup of resources based on PublishStrategy, CIS (External) vs. DNS (Internal). Handles service and zone lookup, and configuring DNS and Infrastructure. Partial: https://issues.redhat.com/browse/SPLAT-633

Update vendor packages for added IBM Cloud DNS Service support.

cjschaef · 2022-08-29T22:04:31Z

/unhold
Rebase completed

rvanderp3 · 2022-08-30T12:36:04Z

/lgtm

openshift-ci · 2022-08-30T16:08:01Z

@cjschaef: 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/okd-e2e-aws-ovn	`13a20a7`	link	false	`/test okd-e2e-aws-ovn`
ci/prow/e2e-openstack	`13a20a7`	link	false	`/test e2e-openstack`
ci/prow/e2e-libvirt	`13a20a7`	link	false	`/test e2e-libvirt`
ci/prow/e2e-metal-ipi	`13a20a7`	link	false	`/test e2e-metal-ipi`
ci/prow/e2e-ibmcloud	`13a20a7`	link	false	`/test e2e-ibmcloud`

Full PR test history. Your PR dashboard.

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

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2022

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2022

cjschaef force-pushed the ibmcloud_private_installconfig branch from 56fdc1b to 2114650 Compare August 24, 2022 20:36

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2022

cjschaef force-pushed the ibmcloud_private_installconfig branch from 2114650 to 5c600eb Compare August 25, 2022 13:02

cjschaef marked this pull request as ready for review August 25, 2022 13:02

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2022

openshift-ci bot requested review from rna-afk and rvanderp3 August 25, 2022 13:08

cjschaef force-pushed the ibmcloud_private_installconfig branch from 5c600eb to c0dad3f Compare August 25, 2022 18:18

openshift-ci bot changed the title ~~IBMCloud: Add DNS Service support - installconfig~~ CORS-2255: IBMCloud: Add DNS Service support - installconfig Aug 25, 2022

openshift-ci bot changed the title ~~CORS-2255: IBMCloud: Add DNS Service support - installconfig~~ [CORS-2271](https://issues.redhat.com//browse/CORS-2271): IBMCloud: Add DNS Service support - installconfig Aug 25, 2022

openshift-ci bot changed the title ~~[CORS-2271](https://issues.redhat.com//browse/CORS-2271): IBMCloud: Add DNS Service support - installconfig~~ CORS-2271: IBMCloud: Add DNS Service support - installconfig Aug 25, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2022

cjschaef force-pushed the ibmcloud_private_installconfig branch 2 times, most recently from 23471ae to 146ae30 Compare August 26, 2022 15:06

rvanderp3 reviewed Aug 26, 2022

View reviewed changes

cjschaef force-pushed the ibmcloud_private_installconfig branch from 146ae30 to f52ee7c Compare August 26, 2022 16:00

cjschaef force-pushed the ibmcloud_private_installconfig branch from f52ee7c to 6af39b1 Compare August 29, 2022 13:27

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2022

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2022

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

cjschaef added 2 commits August 29, 2022 16:50

IBMCloud: Add DNS Service support - vendor

13a20a7

Update vendor packages for added IBM Cloud DNS Service support.

cjschaef force-pushed the ibmcloud_private_installconfig branch from 6af39b1 to 13a20a7 Compare August 29, 2022 22:04

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2022

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

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2022

openshift-merge-robot merged commit 924ac31 into openshift:master Aug 30, 2022

cjschaef deleted the ibmcloud_private_installconfig branch August 30, 2022 16:20

cjschaef mentioned this pull request Sep 7, 2022

CORS-2280: IBMCloud: Add TF support for private DNS #6282

Merged

-	"github.com/openshift/installer/pkg/types"
-	"github.com/pkg/errors"
+	"github.com/pkg/errors"
+	"github.com/openshift/installer/pkg/types"

CORS-2271: IBMCloud: Add DNS Service support - installconfig #6255

CORS-2271: IBMCloud: Add DNS Service support - installconfig #6255

Uh oh!

Conversation

cjschaef commented Aug 23, 2022

Uh oh!

openshift-ci bot commented Aug 23, 2022

Uh oh!

openshift-ci bot commented Aug 25, 2022

Uh oh!

cjschaef commented Aug 25, 2022

Uh oh!

cjschaef commented Aug 25, 2022

Uh oh!

cjschaef commented Aug 25, 2022

Uh oh!

cjschaef commented Aug 25, 2022 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffnowicki commented Aug 25, 2022

Uh oh!

jeffnowicki commented Aug 25, 2022 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjschaef commented Aug 25, 2022

Uh oh!

cjschaef commented Aug 25, 2022

Uh oh!

patrickdillon commented Aug 26, 2022

Uh oh!

openshift-ci bot commented Aug 26, 2022

Uh oh!

rvanderp3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvanderp3 commented Aug 26, 2022

Uh oh!

openshift-ci-robot commented Aug 26, 2022

Uh oh!

rvanderp3 commented Aug 26, 2022

Uh oh!

openshift-ci-robot commented Aug 26, 2022

Uh oh!

openshift-ci-robot commented Aug 26, 2022

Uh oh!

openshift-ci-robot commented Aug 27, 2022

Uh oh!

openshift-ci-robot commented Aug 27, 2022

Uh oh!

openshift-ci-robot commented Aug 27, 2022

Uh oh!

jeffnowicki commented Aug 28, 2022

cjschaef commented Aug 25, 2022 •

edited by openshift-ci bot

Loading

jeffnowicki commented Aug 25, 2022 •

edited by openshift-ci bot

Loading