feat(maas): implement ModelsAsService controller #2964
Conversation
Add ModelsAsService controller component implementation following OpenDataHub patterns (implementing componentHandler interface): - Comprehensive Owns clauses - Basic MaaS Gateway validation and Kustomize manifest parametrization - Include temporary Gateway hostname configuration for cluster domain substitution - Register the component in main controller manager - Add RBAC permissions for ModelsAsService and related resources The component is currently disabled by default until DataScienceCluster integration is complete. Signed-off-by: Edgar Hernández <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughA new ModelsAsService component is integrated into the DataScienceCluster workflow with a component handler, controller reconciler setup, and supporting action logic for gateway configuration, manifest customization, and resource reconciliation. Changes
Sequence Diagram(s)sequenceDiagram
participant DSC as DataScienceCluster<br/>Reconciler
participant MH as ModelsAsService<br/>ComponentHandler
participant RC as ComponentReconciler<br/>Actions
participant GW as Gateway API/<br/>Kubernetes
participant MF as Manifest<br/>System
DSC->>MH: IsEnabled(dsc)
MH-->>DSC: false (placeholder)
Note over RC: Reconciliation Pipeline
RC->>RC: initialize()
activate RC
RC->>MF: Load base manifest
MF-->>RC: Manifest
deactivate RC
RC->>RC: validateGateway()
activate RC
RC->>GW: Resolve gateway reference
GW-->>RC: Gateway validated
deactivate RC
RC->>RC: customizeManifests()
activate RC
RC->>MF: Inject gateway namespace/name
MF-->>RC: Customized manifest
deactivate RC
RC->>RC: configureMaaSGatewayHostname()
activate RC
RC->>GW: Query cluster ingress domain
GW-->>RC: Domain
RC->>MF: Substitute ${CLUSTER_DOMAIN} in hostnames
MF-->>RC: Updated manifest
deactivate RC
RC->>GW: Deploy resources (ConfigMap, Service,<br/>ServiceAccount, Deployment,<br/>ClusterRole, ClusterRoleBinding,<br/>HTTPRoute, Gateway)
GW-->>RC: Resources deployed
RC->>MH: UpdateDSCStatus()
MH->>DSC: Sync status with readiness conditions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ E2E Update Requirement Check FailedNo e2e tests were added/updated as part of this PR. Action required from the PR author: Please either:
For more info, please refer to: workflow run details Why this check exists:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/controller/components/modelsasservice/modelsasservice_controller_actions.go (2)
24-27: Consider clearer import aliases.The current aliases are ambiguous:
v2for OpenShift config API could be confused with other v2 versionstypes2suggests a conflict resolution but the collision isn't obviousv1for gateway-api is genericConsider more descriptive aliases for clarity:
- v2 "github.com/openshift/api/config/v1" - "k8s.io/apimachinery/pkg/runtime" - types2 "k8s.io/apimachinery/pkg/types" - v1 "sigs.k8s.io/gateway-api/apis/v1" + configv1 "github.com/openshift/api/config/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stypes "k8s.io/apimachinery/pkg/types" + gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
78-83: Consider using the definedimagesMapinstead ofnil.The support file defines
imagesMapfor image parameter mappings, butnilis passed here. If image substitution is needed, use the defined map; otherwise, remove the unusedimagesMapfrommodelsasservice_support.go.- if err := odhdeploy.ApplyParams(rr.Manifests[0].String(), "params.env", nil, map[string]string{ + if err := odhdeploy.ApplyParams(rr.Manifests[0].String(), "params.env", imagesMap, map[string]string{internal/controller/components/modelsasservice/modelsasservice.go (1)
78-82: Consider making gateway configuration more flexible.The gateway namespace and name are currently hardcoded. While this may be acceptable for the initial implementation, consider making these values configurable through the component spec or platform configuration to support different deployment scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/main.go(1 hunks)internal/controller/components/modelsasservice/modelsasservice.go(1 hunks)internal/controller/components/modelsasservice/modelsasservice_controller.go(1 hunks)internal/controller/components/modelsasservice/modelsasservice_controller_actions.go(1 hunks)internal/controller/components/modelsasservice/modelsasservice_support.go(1 hunks)internal/controller/datasciencecluster/kubebuilder_rbac.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/controller/components/modelsasservice/modelsasservice_controller_actions.go (5)
api/components/v1alpha1/modelsasservice_types.go (1)
ModelsAsService(45-51)pkg/cluster/gvk/gvk.go (2)
Namespace(29-33)KubernetesGateway(377-381)internal/controller/components/modelsasservice/modelsasservice_support.go (3)
DefaultGatewayNamespace(32-32)DefaultGatewayName(33-33)BaseManifestsSourcePath(36-36)pkg/deploy/envParams.go (1)
ApplyParams(75-123)pkg/resources/resources.go (2)
FormatObjectReference(416-424)ToUnstructured(45-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (13)
internal/controller/components/modelsasservice/modelsasservice_support.go (2)
56-62: LGTM!The
baseManifestInfohelper follows the established pattern for manifest initialization in this codebase.
39-54: Verify usage ofimagesMapandextraParamsMap.These variables are declared but not referenced in the provided action files. In
customizeManifests, a different inline map is passed toApplyParamsinstead of usingextraParamsMap, andimagesMapis not used at all. If these are intended for future use, add a TODO comment. Otherwise, remove them to avoid dead code.cmd/main.go (1)
96-96: LGTM!The side-effect import correctly registers the ModelsAsService component, following the established pattern for other components in this file.
internal/controller/datasciencecluster/kubebuilder_rbac.go (1)
300-306: RBAC structure follows established patterns.The RBAC entries for ModelsAsService and related Kuadrant resources are consistent with other components in this file.
internal/controller/components/modelsasservice/modelsasservice_controller_actions.go (2)
44-48: Spec mutation may not persist without explicit update.Setting defaults on
maas.Spec.Gatewaymutates the in-memory object. If the reconciler doesn't subsequently update the CR, users won't see these defaults reflected in the resource spec.Verify this is intentional behavior (internal defaults only) or if an explicit status update/patch is needed to persist defaults.
88-122: Acknowledged: Temporary code marked for removal.The TODO comment clearly indicates this function should be removed once users create their own Gateways. The implementation is sound for its temporary purpose.
One minor note:
strings.Replacewith count1only replaces the first occurrence. If${CLUSTER_DOMAIN}could appear multiple times in a hostname, considerstrings.ReplaceAll.internal/controller/components/modelsasservice/modelsasservice_controller.go (2)
56-75: LGTM!The action pipeline is well-structured and follows the expected order:
- Initialize manifests
- Validate configuration
- Customize manifests with parameters
- Apply kustomize labels
- Configure runtime values (gateway hostname)
- Deploy resources
- Update deployment status
- Garbage collection (final)
The TODO comments are appropriately documented for future cleanup.
40-55: Verify if namespace-scoped RBAC resources need Owns clauses.The reconciler owns
ClusterRoleandClusterRoleBindingbut notRoleandRoleBinding. If the manifests deploy namespace-scoped RBAC resources, consider adding:Owns(&rbacv1.Role{}). Owns(&rbacv1.RoleBinding{}).Otherwise, changes to those resources won't trigger reconciliation.
internal/controller/components/modelsasservice/modelsasservice.go (5)
40-44: LGTM!The component registration follows standard OpenDataHub operator patterns. The empty struct is appropriate for interface implementation.
46-49: LGTM!Simple getter implementation correctly returns the component name constant.
51-60: LGTM!The initialization logic correctly applies manifest parameters with proper error handling. The referenced constants and functions are expected to be defined in the support file.
64-65: Acknowledged TODO for future integration.The comment appropriately flags that ModelsAsService should be integrated into the KServe component. This aligns with the PR objectives stating the component is disabled by default until DataScienceCluster integration is complete.
86-91: LGTM!The method correctly returns false as per the PR objectives stating the component is disabled by default until DataScienceCluster integration is complete. The TODO comment appropriately documents the future work needed.
| if err := rr.Client.Get(ctx, client.ObjectKeyFromObject(&c), &c); err != nil && !k8serr.IsNotFound(err) { | ||
| return cs, nil | ||
| } |
There was a problem hiding this comment.
Fix error handling to propagate actual errors.
When Get returns an error that is not NotFound, the current code returns (cs, nil) which silently swallows the error. This masks legitimate errors such as permission issues or API server problems. The actual error should be returned to the caller.
Apply this diff to fix the error handling:
if err := rr.Client.Get(ctx, client.ObjectKeyFromObject(&c), &c); err != nil && !k8serr.IsNotFound(err) {
- return cs, nil
+ return cs, fmt.Errorf("failed to get ModelsAsService object: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := rr.Client.Get(ctx, client.ObjectKeyFromObject(&c), &c); err != nil && !k8serr.IsNotFound(err) { | |
| return cs, nil | |
| } | |
| if err := rr.Client.Get(ctx, client.ObjectKeyFromObject(&c), &c); err != nil && !k8serr.IsNotFound(err) { | |
| return cs, fmt.Errorf("failed to get ModelsAsService object: %w", err) | |
| } |
🤖 Prompt for AI Agents
In internal/controller/components/modelsasservice/modelsasservice.go around
lines 100-102, the Get call currently swallows non-NotFound errors by returning
(cs, nil); change the error handling so that when rr.Client.Get returns an error
that is not k8serr.IsNotFound(err) you return that error to the caller (e.g.,
return cs, err) instead of nil, leaving the NotFound branch unchanged.
| rr.Conditions.MarkFalse( | ||
| ReadyConditionType, | ||
| conditions.WithReason(string(operatorv1.Removed)), | ||
| conditions.WithMessage("Component ManagementState is set to %s", string(operatorv1.Removed)), | ||
| conditions.WithSeverity(common.ConditionSeverityInfo), | ||
| ) |
There was a problem hiding this comment.
Clarify the condition message for disabled components.
The message states "Component ManagementState is set to Removed", but this branch executes when IsEnabled(dsc) returns false, which indicates the component is disabled/not deployed. This is not the same as the ManagementState being set to Removed. Consider updating the message to reflect that the component is disabled or not enabled, rather than implying a specific ManagementState value.
Suggested improvement:
rr.Conditions.MarkFalse(
ReadyConditionType,
- conditions.WithReason(string(operatorv1.Removed)),
- conditions.WithMessage("Component ManagementState is set to %s", string(operatorv1.Removed)),
+ conditions.WithReason("Disabled"),
+ conditions.WithMessage("Component is not enabled"),
conditions.WithSeverity(common.ConditionSeverityInfo),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rr.Conditions.MarkFalse( | |
| ReadyConditionType, | |
| conditions.WithReason(string(operatorv1.Removed)), | |
| conditions.WithMessage("Component ManagementState is set to %s", string(operatorv1.Removed)), | |
| conditions.WithSeverity(common.ConditionSeverityInfo), | |
| ) | |
| rr.Conditions.MarkFalse( | |
| ReadyConditionType, | |
| conditions.WithReason("Disabled"), | |
| conditions.WithMessage("Component is not enabled"), | |
| conditions.WithSeverity(common.ConditionSeverityInfo), | |
| ) |
🤖 Prompt for AI Agents
In internal/controller/components/modelsasservice/modelsasservice.go around
lines 123-128, the ReadyCondition MarkFalse call logs "Component ManagementState
is set to Removed" while this branch runs when IsEnabled(dsc) is false
(component is disabled/not deployed); update the conditions.WithMessage to
clearly state the component is disabled or not enabled (e.g., "Component is
disabled / not enabled") and, if there is a more accurate reason enum/value for
a disabled state, replace conditions.WithReason(string(operatorv1.Removed)) with
that reason; keep the severity as-is.
| // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=modelsasservices/finalizers,verbs=update | ||
| // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=modelsasservices/finalizers,verbs=update |
There was a problem hiding this comment.
Remove duplicate RBAC entry.
Lines 303 and 304 are identical. Remove the duplicate entry.
// +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=modelsasservices/finalizers,verbs=update
-// +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=modelsasservices/finalizers,verbs=update📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=modelsasservices/finalizers,verbs=update | |
| // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=modelsasservices/finalizers,verbs=update | |
| // +kubebuilder:rbac:groups=components.platform.opendatahub.io,resources=modelsasservices/finalizers,verbs=update |
🤖 Prompt for AI Agents
In internal/controller/datasciencecluster/kubebuilder_rbac.go around lines 303
to 304, there is a duplicated kubebuilder RBAC annotation line; remove one of
the identical lines so the RBAC entry appears only once, keeping a single
correct annotation for modelsasservices/finalizers with verbs=update.
|
@israel-hdez: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
2 participants
Description
Add ModelsAsService controller component implementation following OpenDataHub patterns (implementing componentHandler interface):
NOTES:
_test.gofile will be contributed in a follow-up PR.Related to: https://issues.redhat.com/browse/RHOAIENG-38246
How Has This Been Tested?
Build image using code on this PR and the other mentioned PRs. Deploy (
make deploy). Then, create the following (internal) resource:Check the operator deploys without errors.
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification