feat: add nodepool scoped maestro readonly bundles controllers#4599
Open
miguelsorianod wants to merge 16 commits intomainfrom
Open
feat: add nodepool scoped maestro readonly bundles controllers#4599miguelsorianod wants to merge 16 commits intomainfrom
miguelsorianod wants to merge 16 commits intomainfrom
Conversation
Collaborator
Author
|
/hold |
miguelsorianod
force-pushed
the
msoriano-add-maestro-readonlynodepools-controllers
branch
from
7bdf0d9 to
60ec28b
Compare
Collaborator
ahitacat
reviewed
Mar 25, 2026
| readonlyBundleManagedByK8sLabelValueNodePoolScoped = "create-nodepool-scoped-maestro-readonly-bundles-controller" | ||
| ) | ||
|
|
||
| // createClusterScopedMaestroReadonlyBundlesSyncer is a controller that creates Maestro readonly bundles for the clusters. |
Collaborator
There was a problem hiding this comment.
Suggested change
| // createClusterScopedMaestroReadonlyBundlesSyncer is a controller that creates Maestro readonly bundles for the clusters. | |
| // createNodePoolScopedMaestroReadonlyBundlesSyncer is a controller that creates Maestro readonly bundles for the clusters. |
miguelsorianod
force-pushed
the
msoriano-add-maestro-readonlynodepools-controllers
branch
from
60ec28b to
2cb3525
Compare
Collaborator
Author
|
I've updated the PR and now the maestro readonly bundle of the nodepool's hypershift nodepool CR belongs to a Cosmos ManagementClusterContent named
In that way:
|
Collaborator
Author
|
In https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/Azure_ARO-HCP/4599/pull-ci-Azure-ARO-HCP-main-e2e-parallel/2038595829597474816 an example execution can be found. Taking an example the ARO-HCP Cluster cluster maestro readonly bundlenodepool maestro readonly bundleserviceproviderclusterserviceprovidernodepoolmaestro readonly orphan deletion logic referencing the maestro bundle IDs shown aboveDetails |
miguelsorianod
commented
Mar 30, 2026
miguelsorianod
commented
Mar 30, 2026
miguelsorianod
changed the title
miguelsorianod
force-pushed
the
msoriano-add-maestro-readonlynodepools-controllers
branch
from
2cb3525 to
c5b9d6c
Compare
ahitacat
reviewed
Mar 31, 2026
ahitacat
reviewed
Mar 31, 2026
miguelsorianod
force-pushed
the
msoriano-add-maestro-readonlynodepools-controllers
branch
from
c5b9d6c to
59d7cb2
Compare
deads2k
reviewed
Mar 31, 2026
deads2k
reviewed
Mar 31, 2026
Collaborator
|
type and database as expected. Some structural comments on exposure: remember less exposed is better, fewer points of configurability or injection is better. We may not be able to rely on the condition (OCP bug fixe needs backporting), but we should adjust #4695 to ensure we get something in the hostedcluster prior to merging this so we have e2e assurance it continues to function. |
Collaborator
Author
|
Two new changes have been performed:
|
miguelsorianod
force-pushed
the
msoriano-add-maestro-readonlynodepools-controllers
branch
from
87d20be to
3e1cf83
Compare
miguelsorianod
force-pushed
the
msoriano-add-maestro-readonlynodepools-controllers
branch
2 times, most recently
from
b7059af to
177cea9
Compare
Collaborator
|
/approve leaving lgtm to reviewer of your choice. |
Collaborator
Author
|
/hold cancel I rebased the PR from main which contains the race condition mentioned in #4599 (comment) (#4856).
About this. What we ended up doing is adding the ability to register an informer and deciding the max parent depth to look for when filtering the resources. |
|
@miguelsorianod: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Collaborator
Author
|
/retest |
Collaborator
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahitacat, deads2k, miguelsorianod The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ing a cosmos lister
…donly bundles logic Extract shared readonly-bundle helpers (build bundle, references, kube content) into maestro_readonly_bundle_helpers.go and use them from cluster- and nodepool-scoped create/persist controllers. Refactor DeleteOrphanedMaestroReadonlyBundles: - List SPC/SPNP with database.ListAll - Add maestro.ForEachMaestroBundle for paginated List and unit tests. - Reuse code in some places - Update/add controller tests for the new paths.
…smos resources Introduce distinct Cosmos resource types for managementClusterContents nested under an hcpOpenShiftCluster vs under a nodePool, and wire them through Cosmos CRUD, global listing (union query), and databasetesting mocks. We remove DBClient.ManagementClusterContents and we instead now provide its access through HCPClusters(...).ManagementClusterContents(...) or NodePools(...).ManagementClusterContents(...) instead. We now have a Cosmos global lister that retrieves all ManagementClusterContents at both cluster scope and nodepool scope. The same occurs for the corresponding informer. The cluster watching controller uses the ManagementclusterContent informer but it only ends up enqueuing cluster-scoped ManagementClusterContent documents. Now that NodePools have their own ManagementClusterContent subresource we update the maestro bundle internal name for node pools to have a fixed name of `readonlyHypershiftNodePool` instead of a prefix, as now there are no potential naming collisions anymore between nodepools of the same cluster or cross cluster.
The existing ManagementClusterContent in Cosmos might include conditions that already exist beforehand and that have been calculated in the new desired content. This commit ensures that LastTransitionTime of those conditions in the case where the status of them hasn't changed is preserved.
…at don't have CS ID We do not want to make the whole sync fail when there are some cosmos resources that do not have the ClusterServiceID attribute set yet. This could be a common occurrence when there's enough amount of creation activity of resources. It should be safe to skip those because if a maestro bundle in the maestro API exists it means that there should be a corresponding Cosmos resource with a maestro bundle reference. If for some reason during the orphan calculation inbetween the first read of cosmos resources and the read in the Maestro api there's a new bundle in maestro, the second read of cosmos resources will catch that and prevent accidental deletion.
miguelsorianod
force-pushed
the
msoriano-add-maestro-readonlynodepools-controllers
branch
from
58d409d to
60c2eda
Compare
|
New changes are detected. LGTM label has been removed. |
Collaborator
Author
|
I added a new commit that updates the orphan controller to take into account that eventually the CS ID won't always be set when controllers run, as we will move the creation of resources against CS in the backend and not in frontend. Details here on what's been done: We do not want to make the whole sync fail when there are some cosmos It should be safe to skip those because if a maestro bundle in the |
…ol readonly maestro bundles When we move CS creation of resources to the backend the CS ID might not be set yet once the controllers run so we skip their processing.
Collaborator
Author
|
I also added another commit that deals with the same scenario of CSID not being set for both the create and read maestro readonly bundles. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We add nodepool-scoped controllers in backend that are able to manage maestro readonly bundles.
With it, we introduce the ability to read the Hypershift's NodePool CRs associated to the node pools and persist it in Cosmos.
The Cosmos ManagementClusterContent type is a direct child of the node pool Cosmos type. Documents within the ManagementClusterContent Cosmos type are different readonly bundles internal references. A new ManagementClusterContent document per each Node Pool of the cluster is added. The maestro bundle internal name references associated to the hypershift nodepools is
readonlyHypershiftNodePool.