Skip to content

Comments

IR-412: IBMCloud: Add support for endpoint overrides#955

Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
cjschaef:ir-412
Dec 12, 2023
Merged

IR-412: IBMCloud: Add support for endpoint overrides#955
openshift-merge-bot[bot] merged 2 commits intoopenshift:masterfrom
cjschaef:ir-412

Conversation

@cjschaef
Copy link
Member

@cjschaef cjschaef commented Nov 9, 2023

Add support to override IBM Cloud Service endpoints, when provided.

Related: https://issues.redhat.com/browse/IR-412

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 9, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 9, 2023

@cjschaef: This pull request references IR-412 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

Add support to override IBM Cloud Service endpoints, when provided.

Related: https://issues.redhat.com/browse/IR-412

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.

@flavianmissi
Copy link
Member

/retest

@flavianmissi
Copy link
Member

Nice work @cjschaef. I have no objections towards merging this. Do you have a plan in regards to px, docs and qe approval?
/test e2e-ibm-ovn

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 10, 2023

@flavianmissi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test e2e-aws-operator
  • /test e2e-aws-ovn
  • /test e2e-aws-ovn-image-registry
  • /test e2e-aws-ovn-upgrade
  • /test e2e-hypershift
  • /test e2e-hypershift-conformance
  • /test e2e-vsphere-operator
  • /test images
  • /test unit
  • /test verify

The following commands are available to trigger optional jobs:

  • /test e2e-azure-manual-oidc
  • /test e2e-azure-operator
  • /test e2e-azure-ovn
  • /test e2e-gcp-operator
  • /test e2e-gcp-ovn
  • /test e2e-openstack
  • /test e2e-vsphere

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

  • pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws-operator
  • pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws-ovn
  • pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws-ovn-image-registry
  • pull-ci-openshift-cluster-image-registry-operator-master-e2e-aws-ovn-upgrade
  • pull-ci-openshift-cluster-image-registry-operator-master-e2e-hypershift
  • pull-ci-openshift-cluster-image-registry-operator-master-e2e-hypershift-conformance
  • pull-ci-openshift-cluster-image-registry-operator-master-images
  • pull-ci-openshift-cluster-image-registry-operator-master-unit
  • pull-ci-openshift-cluster-image-registry-operator-master-verify
Details

In response to this:

Nice work @cjschaef. I have no objections towards merging this. Do you have a plan in regards to px, docs and qe approval?
/test e2e-ibm-ovn

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.

@flavianmissi
Copy link
Member

/test e2e-aws-operator

@cjschaef
Copy link
Member Author

@flavianmissi We will be providing some documentation for IBM Cloud Restricted Network setup and configuration. At this time, these code changes are dependent on the installer, the CCM, and MAPI, to even get to a point to exercise this code (I've best testing in a Disconnected environment). But there are still some items I am working on yet related to the installer.

I will want to work with QE on whether an environment can be configured to test IBM Cloud Disconnected, as I know it requires a good number of user configuration steps and resource setup at this time. If I am able, I'd like to also investigate extending the ibmcos_test.go tests, in case I can add something there. If I can do that before 4.15 FF.

@flavianmissi
Copy link
Member

Thanks for clarifying @cjschaef.
@wewang58 is our QE currently, she might be able to help you out with a cluster.
Maybe you already know this but you can reach the image registry team in Red Hat internal Slack at #forum-ocp-imageregistry if you need faster response.
Also recall that TRT is blocking non-critical merges for the next two weeks due to shift week and US holidays, this might affect your plans for 4.15 (it surely has affected mine).

@cjschaef
Copy link
Member Author

@flavianmissi Okay, thanks for the details.

I have completed testing of these changes in my Disconnected (air-gapped) environment, using all of the changes with installer, CCM, and MAPI, to validate this Operator came up successfully and was using the provided overrides, and was able to communicate with the IBM Cloud Service endpoints (overrides), using IBM Cloud VPC VPE Gateways.

I'll have to guide @wewang58 on the complete setup of this Disconnected environment, and the current dependencies, and limitations (other ClusterOperators require similar changes, etc.), given the vast number of changes across components to test this code, and other components to get a happy IPI cluster (Storage, etc.), if we are going to need that kind of confirmation. It is certainly non-trivial by default, not including the requirement of these other dependent PR's.

Otherwise, not breaking existing functionality, for Public, Private, and BYON IBM Cloud IPI clusters would be good confirmation.

Okay, I believe we will be working on getting approval for these feature changes for the TRT.

@cjschaef
Copy link
Member Author

cjschaef commented Nov 13, 2023

openshift/api#1657 has merged, I will attempt to update the api go dep and update the ServiceName's to use these new enum's.

I pulled in the latest api to use IBMCloudServiceName's.

@cjschaef cjschaef force-pushed the ir-412 branch 5 times, most recently from 41ce655 to 2aa39e4 Compare November 13, 2023 22:33
@cjschaef
Copy link
Member Author

/retest

@flavianmissi
Copy link
Member

Sounds like a plan @cjschaef.
I'll leave it to you to sync with Wen to get QE approval. Do let me know if I can help.

@cjschaef
Copy link
Member Author

cjschaef commented Nov 27, 2023

I'll see about rebasing and go mod changes.

Updated

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023
Add support to override IBM Cloud Service endpoints, when provided.

Related: https://issues.redhat.com/browse/IR-412
Update the IBM golang dependency packages and openshift/api
for IBMCloudServiceName's.
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023
@flavianmissi
Copy link
Member

/lgtm
it looks like QE needs some help testing this.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjschaef, flavianmissi

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 Nov 30, 2023
@sferich888
Copy link
Contributor

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Dec 5, 2023
@stevsmit
Copy link
Member

stevsmit commented Dec 5, 2023

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Dec 5, 2023
@wewang58
Copy link

wewang58 commented Dec 6, 2023

Now installer qe team is trying to provide a private cluster which support for endpoint overrides, they met some issues need time to resolve.

@wewang58
Copy link

Tested in version: 4.15.0-0.ci.test-2023-12-08-081140-ci-ln-1sdsy7t-latest, cos endpoint is override, thanks @jianlinliu provides the cluster
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 11, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 11, 2023

@cjschaef: This pull request references IR-412 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

Add support to override IBM Cloud Service endpoints, when provided.

Related: https://issues.redhat.com/browse/IR-412

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.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 59f66cd and 2 for PR HEAD 06da1d4 in total

@flavianmissi
Copy link
Member

/retest

1 similar comment
@flavianmissi
Copy link
Member

/retest

@jeffnowicki
Copy link

/retest-required

@jeffnowicki
Copy link

/test e2e-hypershift-conformance

@cjschaef
Copy link
Member Author

It looks like the e2e-hypershift-conformance failures are consistent across other PR's as well.
Not sure if that issue is being addressed, or whether we can ignore the failure, given the code changes aren't involved, as other PR's suffer the same failure.

@flavianmissi
Copy link
Member

Yep it does happen everywhere. Let's retry one last time before I make some noise.
/retest

@flavianmissi
Copy link
Member

last error looks different than the previous.
/retest

@jeffnowicki
Copy link

@flavianmissi the e2e-hypershift-conformance test is failing in other PRs as well. Are we certain there isn't an issue with the test itself? The code in this PR doesn't have any hypershift relationship.

@flavianmissi
Copy link
Member

oh yes there certainly is an issue with the these tests - I was just hoping they had fixed it between today and yesterday but it doesn't look like it. I'll have to find out how to skip them, will write back here when I know more.

@flavianmissi
Copy link
Member

/override ci/prow/e2e-hypershift-conformance

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 12, 2023

@flavianmissi: Overrode contexts on behalf of flavianmissi: ci/prow/e2e-hypershift-conformance

Details

In response to this:

/override ci/prow/e2e-hypershift-conformance

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 12, 2023

@cjschaef: all tests passed!

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-bot openshift-merge-bot bot merged commit 08206ec into openshift:master Dec 12, 2023
@cjschaef cjschaef deleted the ir-412 branch December 12, 2023 15:07
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-image-registry-operator-container-v4.16.0-202312121444.p0.g08206ec.assembly.stream for distgit ose-cluster-image-registry-operator.
All builds following this will include this PR.

@jianlinliu
Copy link

@cjschaef we need to backport this PR into 4.15 branch, right?

@cjschaef
Copy link
Member Author

cjschaef commented Jan 3, 2024

@jianlinliu It would appear so. Let me try to get that done.

/cherry-pick release-4.15

@openshift-cherrypick-robot

@cjschaef: new pull request created: #984

Details

In response to this:

@jianlinliu It would appear so. Let me try to get that done.

/cherry-pick release-4.15

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.