🚧 WIP: hypershift integration

[ironcladlou]

🚧 WIP: hypershift integration 🚧

Opening this exploratory PR early so others can get an idea of where my mind is going and discuss.

The idea is to make a backwards-compatible change to the API to support mapping goalertintegrations to different cluster representations: Hive clusterdeployments and Hypershift hostedcluster. Internally, I'm refactoring the reconciliation logic to decouple from the Hive API and instead work with the client.Object interface, using the concrete cluster API types only where needed (e.g. for extracting metadata for the cluster name, ID, etc.)

Since the hypershift API is still external to openshift/api, to use those concrete types we'll have to import github.com/openshift/hypershift which may end up forcing an upgrade to the k8s and controller-runtime versions (which I think may introduce more refactoring, we'll see).

I'll also probably introduce some tests for the reconciler (using a fake client) as needed.

Update 1: re: adding a dependency on the Hypershift API, I had totally missed and forgotten that I actually made the API a submodule long ago for this very reason. Thanks, old me: openshift/hypershift@d7c2eed

Update 2: The next architectural thing I'll be playing with is the assumption the controller currently makes that the GoAlert secret needs projected into a customer cluster, which is an assumption coupled to the implementation details of the ROSA Classic topology. For hypershift, the operator will be running on a management cluster hosting the control planes, and would be generating GoAlert Service endpoints to be consumed by a management cluster process responsible for e.g. syncing AlertManager configs for the co-located hosted clusters. So Hive syncsets wouldn't be involved at all.

Now that I've said written that out, it makes me wonder if there's really a need to make the API flexible at all. I don't know of a use case where we would run a single GoAlert Operator on a cluster that's managing both hypershift and hive clusterdeployments. In practice we would only ever run it on a hive cluster or a management cluster, period. To me this says the overall behavior could instead be modal instead of API driven. For example, keep the existing API, but drive whether to process ClusterDeployments or HostedClusters based on an operator mode (e.g. --cluster-mode=[hive|hypershift]). That would simply a few things.

Tomorrow I'm going to make a commit to demonstrate that and see how it compares.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ironcladlou
Once this PR has been reviewed and has the lgtm label, please assign theautoroboto for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[ironcladlou]

re: adding a dependency on the Hypershift API, I had totally missed and forgotten that I actually made the API a submodule long ago for this very reason. Thanks, old me

openshift/hypershift@d7c2eed

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 137 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.24%. Comparing base (e590a4c) to head (95940dd).

Files with missing lines	Patch %	Lines
...oalertintegration/goalertintegration_controller.go	0.00%	46 Missing ⚠️
...rs/goalertintegration/clusterdeployment_created.go	0.00%	34 Missing ⚠️
...rs/goalertintegration/clusterdeployment_deleted.go	0.00%	27 Missing ⚠️
main.go	0.00%	16 Missing ⚠️
controllers/goalertintegration/event_handlers.go	0.00%	8 Missing ⚠️
...llers/goalertintegration/heartbeatmonitor_check.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #116      +/-   ##
========================================
- Coverage   8.61%   8.24%   -0.38%     
========================================
  Files         14      14              
  Lines        731     764      +33     
========================================
  Hits          63      63              
- Misses       659     692      +33     
  Partials       9       9              

Files with missing lines	Coverage Δ
api/v1alpha1/goalertintegration_types.go	0.00% <ø> (ø)
...llers/goalertintegration/heartbeatmonitor_check.go	0.00% <0.00%> (ø)
controllers/goalertintegration/event_handlers.go	0.00% <0.00%> (ø)
main.go	0.00% <0.00%> (ø)
...rs/goalertintegration/clusterdeployment_deleted.go	0.00% <0.00%> (ø)
...rs/goalertintegration/clusterdeployment_created.go	0.00% <0.00%> (ø)
...oalertintegration/goalertintegration_controller.go	0.00% <0.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[openshift-ci]

@ironcladlou: 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/coverage	3c2a4c1	link	true	/test coverage
ci/prow/test	3c2a4c1	link	true	/test test
ci/prow/lint	3c2a4c1	link	true	/test lint
ci/prow/validate	3c2a4c1	link	true	/test validate

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

[ironcladlou]

The next architectural thing I'll be playing with is the assumption the controller currently makes that the GoAlert secret needs projected into a customer cluster, which is an assumption coupled to the implementation details of the ROSA Classic topology. For hypershift, the operator will be running on a management cluster hosting the control planes, and would be generating GoAlert Service endpoints to be consumed by a management cluster process responsible for e.g. syncing AlertManager configs for the co-located hosted clusters. So Hive syncsets wouldn't be involved at all.

Now that I've said written that out, it makes me wonder if there's really a need to make the API flexible at all. I don't know of a use case where we would run a single GoAlert Operator on a cluster that's managing both hypershift and hive clusterdeployments. In practice we would only ever run it on a hive cluster or a management cluster, period. To me this says the overall behavior could instead be modal instead of API driven. For example, keep the existing API, but drive whether to process ClusterDeployments or HostedClusters based on an operator mode (e.g. --cluster-mode=[hive|hypershift]). That would simply a few things.

Tomorrow I'm going to make a commit to demonstrate that and see how it compares.