Skip to content

IBMCloud: BYON Enablement #6183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Aug 17, 2022
Merged

IBMCloud: BYON Enablement #6183

openshift-merge-robot merged 1 commit into from
Aug 17, 2022

Conversation

@cjschaef
Copy link
Member

@cjschaef cjschaef commented Jul 28, 2022

Setup enablement for Bring Your Own Network support on IBM Cloud
using IPI.

@openshift-ci openshift-ci bot requested review from AnnaZivkovic and sadasu July 28, 2022 18:34
@cjschaef cjschaef force-pushed the ibmcloud_byon_enablement branch from 3f3c05f to f40e80e Compare July 28, 2022 20:08
@cjschaef
Copy link
Member Author

cjschaef commented Jul 28, 2022

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2022
@cjschaef cjschaef force-pushed the ibmcloud_byon_enablement branch 3 times, most recently from 8a965fe to ef14209 Compare August 2, 2022 15:39
@cjschaef
Copy link
Member Author

cjschaef commented Aug 2, 2022

/test e2e-ibmcloud

@cjschaef
Copy link
Member Author

cjschaef commented Aug 8, 2022

MAPI patch merged.

Retesting e2e-ibmcloud to verify install works (Conformance tests expected to fail).
/retest

@cjschaef
Copy link
Member Author

cjschaef commented Aug 8, 2022 •
edited
Loading

API and CI appears to be a bit flaky

 level=info msg=Consuming Worker Machines from target directory
level=fatal msg=failed to fetch Terraform Variables: failed to fetch dependency of "Terraform Variables": failed to generate asset "Platform Provisioning Check": baseDomain: Internal error: failed to get cis instance: Get "https://resource-controller.cloud.ibm.com/v2/resource_instances?resource_id=75874a60-cb12-11e7-948e-37ac098eb1b9": dial tcp: lookup resource-controller.cloud.ibm.com on 172.30.0.10:53: server misbehaving

Hoping a retrigger helps.

/retest

@cjschaef
Copy link
Member Author

cjschaef commented Aug 8, 2022

e2e-ibmcloud failure is due to Conformance tests (which is expected at this time).

So far, I don't think the other failures (libvirt/openstack) are related to changes. So I think this is good for review now.

@cjschaef
Copy link
Member Author

cjschaef commented Aug 8, 2022

/unhold

@openshift-ci 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 8, 2022
rvanderp3
rvanderp3 reviewed Aug 9, 2022
View reviewed changes
pkg/asset/installconfig/ibmcloud/validation_test.go Outdated
Copy link
Contributor

@rvanderp3 rvanderp3 Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is any additional unit test coverage possible with the addition of these new parameters?

Copy link
Member Author

@cjschaef cjschaef Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me take a look quick

Copy link
Member Author

@cjschaef cjschaef Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have some testing coverage for the subnets (these added parameters) as part of TestValidate.

And since neither of those parameters (control plane subnets and compute subnets) are used in the ValidatePreExitingPublicDNS code, I think it's okay to just default them to nil and basically ignore them.

I will see about adding a metadata_test.go suite in a future PR however, so we have more fine grained testing of the metadata.go functions. Perhaps something I can get done next week or so. If that's cool.

rvanderp3
rvanderp3 reviewed Aug 10, 2022
View reviewed changes
pkg/asset/installconfig/ibmcloud/metadata.go Outdated
Comment on lines 133 to 139
Copy link
Contributor

@rvanderp3 rvanderp3 Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does access to m.client need to be wrapped in a mutex here as done in ControlPlaneSubnets and ComputeSubnets?

Copy link
Member Author

@cjschaef cjschaef Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a bad idea, I can add that.

Copy link
Member Author

@cjschaef cjschaef Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

rvanderp3
rvanderp3 reviewed Aug 10, 2022
View reviewed changes
pkg/asset/installconfig/ibmcloud/subnet.go Outdated
Copy link
Contributor

@rvanderp3 rvanderp3 Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the impact of results.ID being nil? does this imply an error or does this imply that a given subnet name wasnt found?

Copy link
Member Author

@cjschaef cjschaef Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's probably better raising an error like the other checks. I'll update.

Copy link
Member Author

@cjschaef cjschaef Aug 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

rvanderp3
rvanderp3 reviewed Aug 10, 2022
View reviewed changes
Copy link
Contributor

@rvanderp3 rvanderp3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM, i left a couple of comments/questions

@cjschaef
IBMCloud: BYON Enablement
486b5e5 
Setup enablement for Bring Your Own Network support on IBM Cloud
using IPI.
@cjschaef cjschaef force-pushed the ibmcloud_byon_enablement branch from ef14209 to 486b5e5 Compare August 11, 2022 20:33
@rvanderp3
Copy link
Contributor

rvanderp3 commented Aug 11, 2022

/lgtm
/assign @patrickdillon

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2022
@jeffnowicki
Copy link
Contributor

jeffnowicki commented Aug 12, 2022

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 12, 2022

@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/e2e-libvirt 486b5e5 link false /test e2e-libvirt
ci/prow/e2e-ibmcloud 486b5e5 link false /test e2e-ibmcloud
ci/prow/e2e-openstack 486b5e5 link false /test e2e-openstack

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.

@jstuever
Copy link
Contributor

jstuever commented Aug 17, 2022

/assign

@jstuever
Copy link
Contributor

jstuever commented Aug 17, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jstuever

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 17, 2022
@openshift-merge-robot openshift-merge-robot merged commit a8482db into openshift:master Aug 17, 2022
@cjschaef cjschaef deleted the ibmcloud_byon_enablement branch August 18, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AnnaZivkovic AnnaZivkovic Awaiting requested review from AnnaZivkovic

@sadasu sadasu Awaiting requested review from sadasu

1 more reviewer

@rvanderp3 rvanderp3 rvanderp3 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@patrickdillon patrickdillon

@jstuever jstuever

@rvanderp3 rvanderp3

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cjschaef @rvanderp3 @jeffnowicki @jstuever @patrickdillon @openshift-merge-robot