feat(RHOAIENG-30795): Add configuration support to TrustyAI DSC component #2225
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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 |
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant Reconciler
participant TrustyAI_CR as TrustyAI CR
participant DSC as DSCInit (namespace)
participant K8s as ConfigMap (K8s)
end
Note over Reconciler,TrustyAI_CR: Reconciliation run for TrustyAI component
Reconciler->>TrustyAI_CR: Read spec.eval.lmeval.permitCodeExecution\nand spec.eval.lmeval.permitOnline
alt eval present or defaults
Reconciler->>DSC: Determine ApplicationsNamespace
Reconciler->>K8s: Create/Update `trustyai-dsc-config`\nset data keys "eval.lmeval.permitCodeExecution" and "eval.lmeval.permitOnline"
K8s-->>Reconciler: ACK / Created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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 |
ruivieira
changed the title
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/16581270657 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/api-overview.md (3)
560-563: Clarify optionality & defaults for the newevalfieldPlease state explicitly that the
evalblock is optional and that, when omitted, all nested flags default tofalse. This matches the controller logic and avoids any ambiguity for API consumers.
2004-2008: Duplicate description – keep one authoritative source
evalis documented both here (TrustyAICommonSpec) and in DSCTrustyAI. If you keep both, make sure the wording is identical; otherwise delete the duplicate to avoid drift.
2027-2060: Provide a concrete YAML example for the new structsReaders benefit from a minimal YAML snippet illustrating correct usage:
spec: trustyai: eval: lmeval: allowCodeExecution: true allowOnline: falseAdding this right after the tables will greatly improve discoverability.
internal/controller/components/trustyai/trustyai_test.go (1)
95-200: Consider adding test for ConfigMap updates.While the current tests cover creation scenarios well, consider adding a test case that verifies ConfigMap updates when the configuration changes. This would ensure the reconciliation logic handles updates correctly.
Here's an example test case to add:
t.Run("should update ConfigMap when configuration changes", func(t *testing.T) { g := NewWithT(t) ctx := context.Background() // Create initial ConfigMap trustyai := createTrustyAICRWithConfig(true, false) dsc := createDSCWithTrustyAI(operatorv1.Managed) dsciObj := createDSCI("test-namespace") cli, err := fakeclient.New(fakeclient.WithObjects(dsc, dsciObj)) g.Expect(err).ShouldNot(HaveOccurred()) rr := &types.ReconciliationRequest{ Client: cli, Instance: trustyai, DSCI: dsciObj, } err = createConfigMap(ctx, rr) g.Expect(err).ShouldNot(HaveOccurred()) // Update configuration trustyai.Spec.Eval.LMEval.AllowOnline = &[]bool{true}[0] rr.Instance = trustyai err = createConfigMap(ctx, rr) g.Expect(err).ShouldNot(HaveOccurred()) // Verify ConfigMap was updated configMap := &corev1.ConfigMap{} err = cli.Get(ctx, client.ObjectKey{ Name: "trustyai-dsc-config", Namespace: "test-namespace", }, configMap) g.Expect(err).ShouldNot(HaveOccurred()) g.Expect(configMap.Data["eval.lmeval.allowOnline"]).Should(Equal("true")) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/components/v1alpha1/trustyai_types.go(1 hunks)api/components/v1alpha1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/components.platform.opendatahub.io_trustyais.yaml(1 hunks)config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml(1 hunks)docs/api-overview.md(4 hunks)internal/controller/components/trustyai/trustyai_controller.go(1 hunks)internal/controller/components/trustyai/trustyai_controller_actions.go(3 hunks)internal/controller/components/trustyai/trustyai_test.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/components/v1alpha1/trustyai_types.go (1)
api/common/types.go (1)
DevFlagsSpec(36-40)
api/components/v1alpha1/zz_generated.deepcopy.go (1)
api/components/v1alpha1/trustyai_types.go (2)
TrustyAIEvalSpec(58-61)TrustyAILMEvalSpec(64-71)
internal/controller/components/trustyai/trustyai_controller_actions.go (4)
pkg/controller/types/types.go (1)
ReconciliationRequest(75-112)api/components/v1alpha1/trustyai_types.go (1)
TrustyAI(44-50)pkg/cluster/gvk/gvk.go (1)
Namespace(24-28)pkg/metadata/labels/types.go (3)
ODH(26-34)True(12-12)K8SCommon(19-23)
⏰ 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: golangci-lint
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (14)
internal/controller/components/trustyai/trustyai_controller.go (1)
71-71: LGTM! Clean integration of ConfigMap creation action.The new
createConfigMapaction is appropriately positioned in the reconciliation chain afterdevFlagsand beforereleases.NewAction(), ensuring configuration is established at the right stage of the reconciliation process.config/crd/bases/components.platform.opendatahub.io_trustyais.yaml (1)
77-94: LGTM! Well-structured CRD schema extension.The new
evalfield and nestedlmevalconfiguration are properly defined with:
- Clear descriptions for all fields
- Appropriate default values (
falsefor security-sensitive settings)- Consistent naming conventions
- Extensible structure for future evaluation configurations
config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml (1)
738-755: LGTM! Consistent schema extension across CRDs.The
evalfield structure in the DataScienceCluster CRD correctly mirrors the TrustyAI component CRD schema, maintaining API consistency. The placement within thetrustyaicomponent configuration section is appropriate.api/components/v1alpha1/trustyai_types.go (1)
57-77: LGTM! Well-designed API type definitions.The new evaluation configuration types are properly structured:
- Pointer types (
*bool) enable differentiation between unset and false values- Kubebuilder default annotations align with CRD schema
- Extensible design allows for future evaluation configuration types
- Integration into
TrustyAICommonSpecmakes configuration accessible at both component and DSC levelsinternal/controller/components/trustyai/trustyai_controller_actions.go (1)
81-127: LGTM! Well-implemented ConfigMap creation function.The
createConfigMapfunction demonstrates good practices:
- Proper type assertion with error handling
- Efficient early return when no configuration is specified
- Well-structured ConfigMap with appropriate labels and annotations
- Conditional data population only for non-nil configuration values
- Correct use of
client.ApplywithForceOwnershipfor idempotent operationsThe hierarchical data key format (
eval.lmeval.allowCodeExecution) provides clear configuration structure.api/components/v1alpha1/zz_generated.deepcopy.go (3)
2138-2138: LGTM! Proper deep copy implementation for new Eval field.The addition of
in.Eval.DeepCopyInto(&out.Eval)correctly ensures the new evaluation configuration field is deep copied when TrustyAICommonSpec is copied.
2167-2181: LGTM! Standard deep copy implementation for TrustyAIEvalSpec.The deep copy methods correctly implement the Kubernetes pattern, ensuring the nested LMEval field is recursively deep copied and proper nil handling is in place.
2183-2206: LGTM! Correct deep copy implementation for pointer fields.The deep copy methods properly handle the optional boolean pointer fields (AllowCodeExecution and AllowOnline) with appropriate nil checks and pointer allocation, following Kubernetes deep copy best practices.
internal/controller/components/trustyai/trustyai_test.go (6)
11-11: LGTM! Appropriate imports added for the new test functionality.The new imports are correctly added to support the ConfigMap testing functionality:
corev1for ConfigMap API objectsclientfor client operations in testsdscifor DSCInitialization resourcesAlso applies to: 13-13, 18-18
95-135: Comprehensive test coverage for ConfigMap creation with full configuration.The test properly validates:
- ConfigMap creation with both evaluation settings
- Correct data mapping to dot-notation keys
- Proper labels and annotations for component identification
- Error handling for successful operations
The test structure is well-organized and follows good testing practices.
137-165: Good test coverage for the no-configuration scenario.The test correctly verifies that when no evaluation configuration is provided, the ConfigMap is not created, which aligns with the expected behavior of skipping ConfigMap creation when there's nothing to configure.
167-200: Excellent test for partial configuration handling.This test validates an important edge case where only some configuration fields are set. The assertion on line 198 correctly checks for an empty string rather than checking for absence, which is appropriate for ConfigMap data fields.
300-306: Well-designed helper function for DSCI creation.The
createDSCIhelper function is clean and focused, properly setting up a DSCInitialization object with the necessary fields for testing.
332-343: Helper functions provide good test data setup.Both
createTrustyAICRWithConfigandcreateTrustyAICRWithPartialConfigare well-designed:
- They reuse the existing
createTrustyAICRfunction for consistency- They properly set pointer fields for the evaluation configuration
- They handle both full and partial configuration scenarios appropriately
|
is this planned for 2.24? |
internal/controller/components/trustyai/trustyai_controller_actions.go
Outdated
Show resolved
Hide resolved
internal/controller/components/trustyai/trustyai_controller_actions.go
Outdated
Show resolved
Hide resolved
|
according to refinement doc, the new field need rename to |
internal/controller/components/trustyai/trustyai_controller_actions.go
Outdated
Show resolved
Hide resolved
internal/controller/components/trustyai/trustyai_controller_actions.go
Outdated
Show resolved
Hide resolved
Since they have default values, no need to use pointers.
ruivieira
changed the title
|
cc @lburgazzoli can re-request a review? thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/components/trustyai/trustyai_test.go (2)
4-12: Missing import for context package (build break).
context.Background()is used but"context"isn’t imported.Apply this diff to add the import:
import ( + "context" "encoding/json" "testing"
295-301: Replace wrong alias in trustyai_test.go
Changetypes.ReconciliationRequesttoodhtypes.ReconciliationRequestto match the import.--- a/internal/controller/components/trustyai/trustyai_test.go +++ b/internal/controller/components/trustyai/trustyai_test.go @@ -295,7 +295,7 @@ - cs, err := handler.UpdateDSCStatus(ctx, &types.ReconciliationRequest{ + cs, err := handler.UpdateDSCStatus(ctx, &odhtypes.ReconciliationRequest{ Client: cli, Instance: dsc, Conditions: conditions.NewManager(dsc, ReadyConditionType), </blockquote></details> </blockquote></details>🧹 Nitpick comments (2)
internal/controller/components/trustyai/trustyai_test.go (2)
93-198: Strengthen ConfigMap tests; prefer t.Context() and assert GVK/metadata.
- Use
t.Context()(available in recent Go) for consistency with other subtests and automatic cancellation.- Also assert Kind/APIVersion and (if set by createConfigMap) expected labels/annotations to guard regressions.
Apply this diff to switch to
t.Context():- ctx := context.Background() + ctx := t.Context() @@ - ctx := context.Background() + ctx := t.Context() @@ - ctx := context.Background() + ctx := t.Context()Optionally extend assertions (example):
configMapResource := rr.Resources[0] g.Expect(configMapResource.GetName()).Should(Equal("trustyai-dsc-config")) g.Expect(configMapResource.GetNamespace()).Should(Equal("test-namespace")) + g.Expect(configMapResource.GetKind()).Should(Equal("ConfigMap")) + g.Expect(configMapResource.GetAPIVersion()).Should(Equal("v1")) + // If labels/annotations are set by createConfigMap, assert them too: + // g.Expect(configMapResource.GetLabels()["app.kubernetes.io/managed-by"]).Should(Equal("opendatahub-operator"))
358-369: Align helper param names with new API (“permit”).*Rename
allowCodeExecutionparam topermitCodeExecutionfor consistency with fields and test keys.Apply this diff:
-func createTrustyAICRWithPartialConfig(allowCodeExecution bool) *componentApi.TrustyAI { +func createTrustyAICRWithPartialConfig(permitCodeExecution bool) *componentApi.TrustyAI { c := createTrustyAICR(true) - c.Spec.Eval.LMEval.PermitCodeExecution = allowCodeExecution + c.Spec.Eval.LMEval.PermitCodeExecution = permitCodeExecution return c }📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/crd/bases/components.platform.opendatahub.io_trustyais.yaml(1 hunks)config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml(1 hunks)docs/api-overview.md(4 hunks)internal/controller/components/trustyai/trustyai_test.go(7 hunks)🚧 Files skipped from review as they are similar to previous changes (3)
- config/crd/bases/components.platform.opendatahub.io_trustyais.yaml
- config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml
- docs/api-overview.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-26T07:45:31.294Z
Learnt from: lburgazzoli PR: opendatahub-io/opendatahub-operator#2010 File: internal/controller/services/servicemesh/servicemesh_controller_actions.go:308-318 Timestamp: 2025-08-26T07:45:31.294Z Learning: In the opendatahub-operator reconciliation framework, the DSCI (DSCInitialization) field in ReconciliationRequest is guaranteed to be non-nil when action functions are executed. The reconciler fetches DSCI via cluster.GetDSCI() and sets rr.DSCI = dsci.DeepCopy() before executing any actions, so nil guards for rr.DSCI are unnecessary in ServiceMesh controller action functions.Applied to files:
internal/controller/components/trustyai/trustyai_test.go📚 Learning: 2025-08-26T07:45:31.294Z
Learnt from: lburgazzoli PR: opendatahub-io/opendatahub-operator#2010 File: internal/controller/services/servicemesh/servicemesh_controller_actions.go:308-318 Timestamp: 2025-08-26T07:45:31.294Z Learning: In the opendatahub-operator reconciliation framework, the DSCI (DSCInitialization) field in ReconciliationRequest is guaranteed to be non-nil when used in ServiceMesh controller actions. The reconciler ensures DSCI is populated before invoking action functions, so nil guards for rr.DSCI are unnecessary in this context.Applied to files:
internal/controller/components/trustyai/trustyai_test.go🧬 Code graph analysis (1)
internal/controller/components/trustyai/trustyai_test.go (2)
pkg/utils/test/fakeclient/fakeclient.go (1)
WithObjects(30-34)pkg/controller/types/types.go (1)
ReconciliationRequest(75-112)⏰ 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). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on tests/integration
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (4)
internal/controller/components/trustyai/trustyai_test.go (4)
213-218: Alias switch to odhtypes is correct.Good migration to
odhtypes.ReconciliationRequest.
241-246: Alias switch to odhtypes is correct.Consistent with the updated import.
268-273: Alias switch to odhtypes is correct.Consistent usage across tests.
326-333: Helper to create DSCI looks good.Correct GVK and namespace wiring; keeps tests focused.
|
@ruivieira: The following test 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. |
Description
This PR adds TrustyAI configuration support to the DataScienceCluster (DSC) resource, enabling control of TrustyAI component settings directly through the DSC. This addresses the current limitation where TrustyAI configuration requires manual patching of operator ConfigMaps and operator re-deployment to change global (admin level) TrustyAI settings.
New Feature
The PR introduces a new configuration structure for TrustyAI in the DSC:
Implementation Details
TrustyAIEvalSpecandTrustyAILMEvalSpecto support LMEval configurationcreateConfigMapaction to the TrustyAI reconciliation chaintrustyai-dsc-configConfigMap in the applications namespacefalse)See RHOAIENG-30795.
How Has This Been Tested?
Unit Tests
createConfigMapfunctionIntegration Testing
Manual Testing
Code Generation
make generateproduces correct CRDs with the new configuration schemaScreenshot or short clip
N/A
Merge criteria
Testing Instructions
Testing Instructions
To test this new TrustyAI configuration feature:
Build and deploy the operator:
cd opendatahub-operator make generate make build make image-build make deploy IMG=quay.io/ruimvieira/opendatahub-operator:dev-2.33.0Create DSCInitialization:
Apply DSC with TrustyAI configuration:
Verify the configuration:
Test configuration updates:
Summary by CodeRabbit
New Features
Documentation
Tests