fix: Make orchestrator replicas configurable#623
fix: Make orchestrator replicas configurable#623christinaexyou wants to merge 1 commit intotrustyai-explainability:mainfrom
Conversation
Reviewer's GuideMakes the GuardrailsOrchestrator deployment replica count configurable via the custom resource spec, with sane defaults and validation, and adds tests to verify a custom replica count is honored in the generated Deployment. Sequence diagram for configurable orchestrator replicas via GORCH CRsequenceDiagram
actor Operator
participant KubernetesAPI
participant GuardrailsOrchestratorController
participant DeploymentTemplate
participant OrchestratorDeployment
Operator->>KubernetesAPI: Apply GuardrailsOrchestrator (Spec.Replicas = N or omitted)
KubernetesAPI-->>GuardrailsOrchestratorController: Watch event for GuardrailsOrchestrator
GuardrailsOrchestratorController->>GuardrailsOrchestratorController: Read Spec.Replicas (default 1, minimum 1)
GuardrailsOrchestratorController->>DeploymentTemplate: Render deployment.tmpl.yaml with replicas = Spec.Replicas
DeploymentTemplate-->>GuardrailsOrchestratorController: Deployment manifest (spec.replicas = Spec.Replicas)
GuardrailsOrchestratorController->>KubernetesAPI: Create or Update OrchestratorDeployment
KubernetesAPI-->>OrchestratorDeployment: Ensure desired replicas count
OrchestratorDeployment-->>Operator: Pods running according to configured replica count
Class diagram for updated GuardrailsOrchestratorSpec replicas fieldclassDiagram
class GuardrailsOrchestratorSpec {
+int32 Replicas
+string OrchestratorConfig
+[]string TlsSecrets
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughReplicas for GuardrailsOrchestrator made optional with default/minimum=1 and omitempty; deployment template now uses spec.replicas; controller reconciler extracts TLS mounts via helper, patches deployments (preserves annotations, retries on missing TLS secrets); tests added for custom and scaled replicas. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User/API (CR create/update)
participant K8s as Kubernetes API (ConfigMap/Secret/Deployment)
participant Controller as Gorch Controller
User->>K8s: Create/Update GuardrailsOrchestrator (replicas optional or set)
K8s->>Controller: Watch -> Reconcile
Controller->>K8s: Get ConfigMap
Controller->>K8s: Get TLS Secret(s)
alt TLS Secret missing
K8s-->>Controller: NotFound
Controller->>Controller: Requeue / retry later
else TLS Secret(s) present
K8s-->>Controller: Secret(s) & ConfigMap
Controller->>K8s: Get existing Deployment
K8s-->>Controller: Deployment (or nil)
Controller->>Controller: Build Deployment (set replicas from spec, add TLS mounts, set configmap-hash)
Controller->>K8s: Create or Patch/Update Deployment if diff
K8s-->>Controller: Updated/Created Deployment
Controller->>K8s: Update status / emit events
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
createGuardrailsOrchestratorWithReplicas, thenameparameter is never used and the function instead relies on the globalorchestratorName/namespaceName, which is both confusing and error-prone; either use the passedname/namespace or remove the unused parameter and globals to make the helper consistent. - The call
createGuardrailsOrchestratorWithReplicas(ctx, orchestratorName+"-config", namespaceName, 3)appears to passnamespaceNameinto thenameparameter position, which is likely incorrect given the helper’s intent—double-check the parameter order and update the function signature or call site so name and namespace are clearly separated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `createGuardrailsOrchestratorWithReplicas`, the `name` parameter is never used and the function instead relies on the global `orchestratorName`/`namespaceName`, which is both confusing and error-prone; either use the passed `name`/namespace or remove the unused parameter and globals to make the helper consistent.
- The call `createGuardrailsOrchestratorWithReplicas(ctx, orchestratorName+"-config", namespaceName, 3)` appears to pass `namespaceName` into the `name` parameter position, which is likely incorrect given the helper’s intent—double-check the parameter order and update the function signature or call site so name and namespace are clearly separated.
## Individual Comments
### Comment 1
<location> `controllers/gorch/guardrailsorchestrator_controller_test.go:108-105` </location>
<code_context>
return err
}
+func createGuardrailsOrchestratorWithReplicas(ctx context.Context, orchestratorConfigMap string, name string, replicas int32) error {
+ typedNamespacedName := types.NamespacedName{Name: orchestratorName, Namespace: namespaceName}
+ err := k8sClient.Get(ctx, typedNamespacedName, &gorchv1alpha1.GuardrailsOrchestrator{})
+ if err != nil && errors.IsNotFound(err) {
+ gorch := &gorchv1alpha1.GuardrailsOrchestrator{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: typedNamespacedName.Name,
+ Namespace: typedNamespacedName.Namespace,
+ },
+ Spec: gorchv1alpha1.GuardrailsOrchestratorSpec{
+ Replicas: replicas,
+ OrchestratorConfig: &orchestratorConfigMap,
+ },
+ }
+ err = k8sClient.Create(ctx, gorch)
+ }
+ return err
+}
+
</code_context>
<issue_to_address>
**suggestion:** Helper ignores the `name` parameter and hardcodes namespace/name, which can make tests harder to read and reuse
In `createGuardrailsOrchestratorWithReplicas`, the `name` parameter is unused; the function instead always uses the global `orchestratorName`/`namespaceName` via:
```go
typedNamespacedName := types.NamespacedName{Name: orchestratorName, Namespace: namespaceName}
```
To avoid misleading callers and to improve reuse, either (a) use the `name` argument and pass namespace explicitly (or as a parameter), or (b) remove the parameter and clearly document that this helper is fixed to the global name/namespace.
Suggested implementation:
```golang
func createGuardrailsOrchestratorWithReplicas(ctx context.Context, orchestratorConfigMap string, name string, namespace string, replicas int32) error {
typedNamespacedName := types.NamespacedName{Name: name, Namespace: namespace}
err := k8sClient.Get(ctx, typedNamespacedName, &gorchv1alpha1.GuardrailsOrchestrator{})
if err != nil && errors.IsNotFound(err) {
gorch := &gorchv1alpha1.GuardrailsOrchestrator{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
},
Spec: gorchv1alpha1.GuardrailsOrchestratorSpec{
```
You will need to update all call sites of `createGuardrailsOrchestratorWithReplicas` in this test file (and anywhere else it might be used) to pass the `namespace` argument explicitly, e.g.:
- Before: `createGuardrailsOrchestratorWithReplicas(ctx, cfgMap, orchestratorName, replicas)`
- After: `createGuardrailsOrchestratorWithReplicas(ctx, cfgMap, orchestratorName, namespaceName, replicas)`
Adjust the last two arguments as appropriate if some callers use different names/namespaces.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @controllers/gorch/guardrailsorchestrator_controller_test.go:
- Around line 108-125: The function createGuardrailsOrchestratorWithReplicas
declares a unused parameter name while the code uses the orchestratorName
constant; change the function to use the name parameter when building
typedNamespacedName (types.NamespacedName{Name: name, Namespace: namespaceName})
and update the call site that currently passes namespaceName as the name
argument to pass orchestratorName instead so the intended name parameter is
used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/gorch/v1alpha1/guardrailsorchestrator_types.goconfig/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yamlcontrollers/gorch/guardrailsorchestrator_controller_test.gocontrollers/gorch/templates/deployment.tmpl.yaml
⏰ 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: Sourcery review
- GitHub Check: deploy
- GitHub Check: build
🔇 Additional comments (5)
controllers/gorch/templates/deployment.tmpl.yaml (1)
70-70: LGTM! Dynamic replica configuration implemented correctly.The template now dynamically references the replica count from the CR spec, which will use the default value of 1 when not specified.
config/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yaml (1)
218-226: LGTM! CRD validation and defaults are appropriate.The default value of 1 and minimum validation ensure that deployments always have at least one replica, preventing invalid configurations.
controllers/gorch/guardrailsorchestrator_controller_test.go (2)
444-506: LGTM! Test case validates configurable replicas correctly.The test properly verifies that a GuardrailsOrchestrator can be created with a custom replica count (3) and that the resulting Deployment reflects this configuration.
899-899: LGTM! Test properly integrated into the suite.api/gorch/v1alpha1/guardrailsorchestrator_types.go (1)
45-47: LGTM! Kubebuilder markers correctly define validation and defaults.The combination of
Minimum=1,default=1, andomitemptyensures that replicas are always valid (≥1) while allowing users to omit the field and get a sensible default.
christinaexyou
force-pushed
the
gorch-replicas-config
branch
from
afcc9df to
d99befa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yaml (1)
218-226: LGTM! Default and minimum values are appropriate.The replicas field is correctly configured with a default of 1 and a minimum of 1, which are sensible constraints for a deployment replica count.
Optional: Consider adding a maximum value
While not strictly necessary, you might consider adding a reasonable maximum value (e.g., 100) to prevent accidental misconfigurations:
default: 1 description: |- INSERT ADDITIONAL SPEC FIELDS - desired state of cluster Important: Run "make" to regenerate code after modifying this file Number of replicas format: int32 minimum: 1 + maximum: 100 type: integerThis is purely optional, as Kubernetes resource limits would ultimately constrain excessive replicas anyway.
api/gorch/v1alpha1/guardrailsorchestrator_types.go (1)
45-47: Add// +optionalmarker for consistency.The implementation is functionally correct, but for consistency with other optional fields in this struct (e.g.,
OrchestratorConfigat line 50,AutoConfigat line 53, and especiallyOTLPProtocolat line 87 which also has a default value), consider adding the// +optionalmarker.Suggested change for consistency
// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster // Important: Run "make" to regenerate code after modifying this file // Number of replicas + // +optional // +kubebuilder:validation:Minimum=1 // +kubebuilder:default=1 Replicas int32 `json:"replicas,omitempty"`
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/gorch/v1alpha1/guardrailsorchestrator_types.goconfig/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yamlcontrollers/gorch/guardrailsorchestrator_controller_test.gocontrollers/gorch/templates/deployment.tmpl.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/gorch/templates/deployment.tmpl.yaml
- controllers/gorch/guardrailsorchestrator_controller_test.go
⏰ 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: Sourcery review
- GitHub Check: build
- GitHub Check: deploy
🔇 Additional comments (1)
api/gorch/v1alpha1/guardrailsorchestrator_types.go (1)
45-47: The deployment template correctly uses the configurable replica count atreplicas: {{.Orchestrator.Spec.Replicas}}. No hardcoded replica values exist in the gorch-specific templates or controller code.
There was a problem hiding this comment.
what do you think about adding some additional tests here, e.g.:
- test default behavior (not specifying replicas)
- iInvalid value (replicas: 0)
- scaling existing deployment (update from 1 to 3)
There was a problem hiding this comment.
Currently the controller doesn't update the deployment when the CR changes (eg. scaling replicas) but I think it's useful to have so I added the following lines: https://github.com/trustyai-explainability/trustyai-service-operator/pull/623/changes#diff-ef06797c0d315f0614d1c745b355ae0cf01b2ab5f621d0f8820a720c7bed527cR327-R361
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controllers/gorch/guardrailsorchestrator_controller_test.go (2)
107-124: Helper function works correctly for current tests.The function creates a GuardrailsOrchestrator with a custom replica count. It hardcodes
orchestratorNameandnamespaceNamefrom global constants (line 108), which limits reusability but is acceptable for this test suite where all tests use the same namespace.Optional: Make helper more flexible
If you want to make this helper reusable across different namespaces or orchestrator names in the future, consider adding name and namespace parameters:
-func createGuardrailsOrchestratorWithReplicas(ctx context.Context, orchestratorConfigMap string, replicas int32) error { - typedNamespacedName := types.NamespacedName{Name: orchestratorName, Namespace: namespaceName} +func createGuardrailsOrchestratorWithReplicas(ctx context.Context, orchestratorConfigMap string, name string, namespace string, replicas int32) error { + typedNamespacedName := types.NamespacedName{Name: name, Namespace: namespace}Then update call sites to pass the explicit parameters. However, this is optional since the current implementation works fine for the existing test cases.
994-996: Consider adding a test for default replica behavior.The test suite now includes tests for custom replicas (3), invalid replicas (0), and scaling (1→3). However, a test verifying the default behavior (when replicas is not specified in the spec) would provide complete coverage.
Based on the CRD definition, when replicas is omitted, it should default to 1. A test could verify this by creating a GuardrailsOrchestrator without setting the Replicas field and confirming the deployment has 1 replica.
Example test for default behavior
func testCreateGuardrailsOrchestratorWithDefaultReplicas(namespaceName string) { It("Should create a deployment with default replica count (1) when not specified", func() { By("Creating a custom resource without specifying replicas") ctx := context.Background() typedNamespacedName := types.NamespacedName{Name: orchestratorName, Namespace: namespaceName} // Use the existing createGuardrailsOrchestrator helper which doesn't set replicas err := createGuardrailsOrchestrator(ctx, orchestratorName+"-config", orchestratorName, namespaceName) Expect(err).ToNot(HaveOccurred()) By("Reconciling the custom resource") reconciler := &GuardrailsOrchestratorReconciler{ Client: k8sClient, Scheme: k8sClient.Scheme(), Namespace: namespaceName, } _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typedNamespacedName}) Expect(err).ToNot(HaveOccurred()) By("Verifying the deployment has default replica count of 1") Eventually(func() error { deployment := &appsv1.Deployment{} if err = k8sClient.Get(ctx, typedNamespacedName, deployment); err != nil { return err } Expect(*deployment.Spec.Replicas).Should(Equal(int32(1))) return nil }, time.Second*10, time.Millisecond*10).Should(Succeed()) // Cleanup... }) }Add to test suite:
testCreateGuardrailsOrchestratorWithCustomReplicas(namespaceName) testCreateGuardrailsOrchestratorWithIncorrectReplicas(namespaceName) testCreateGuardrailsOrchestratorWithScaledUpReplicas(namespaceName) +testCreateGuardrailsOrchestratorWithDefaultReplicas(namespaceName)Based on learnings: A previous reviewer (m-misiura) suggested adding tests for default behavior, invalid values, and scaling. While the invalid and scaling tests have been added, the default behavior test was not included.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/gorch/guardrailsorchestrator_controller.gocontrollers/gorch/guardrailsorchestrator_controller_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
controllers/gorch/guardrailsorchestrator_controller_test.go (3)
api/gorch/v1alpha1/guardrailsorchestrator_types.go (2)
GuardrailsOrchestrator(136-142)GuardrailsOrchestratorSpec(41-80)controllers/constants/version.go (1)
ConfigMap(5-5)controllers/gorch/guardrailsorchestrator_controller.go (1)
GuardrailsOrchestratorReconciler(49-54)
controllers/gorch/guardrailsorchestrator_controller.go (1)
controllers/utils/status.go (1)
ReconcileFailed(22-22)
⏰ 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: Sourcery review
- GitHub Check: build
- GitHub Check: deploy
🔇 Additional comments (5)
controllers/gorch/guardrailsorchestrator_controller.go (2)
22-24: LGTM: Import organization improved.The addition of
strconvandtimeto the top-level imports is clean and both are used in the file (strconv.FormatBoolat line 87 andtime.Secondat line 437).
327-362: Update logic properly handles replica changes and deployment reconciliation.The new deployment update path correctly:
- Regenerates the deployment spec from the current orchestrator (picking up replica changes)
- Applies TLS mounts consistently
- Preserves and updates ConfigMap hash annotations
- Only triggers an update when
patchDeploymentdetects actual differencesThis ensures that replica count changes in the GuardrailsOrchestrator CR are propagated to the deployment.
controllers/gorch/guardrailsorchestrator_controller_test.go (3)
443-505: LGTM: Custom replica test is comprehensive.This test properly verifies that a GuardrailsOrchestrator created with 3 replicas results in a deployment with 3 replicas. The test flow is complete: creates prerequisites, creates the CR, reconciles, verifies the deployment replica count, and cleans up resources.
507-514: LGTM: Validation test correctly expects failure.The test verifies that creating a GuardrailsOrchestrator with 0 replicas fails due to CRD validation (minimum=1). The test correctly expects
k8sClient.Create()to return an error from the server-side validation.
516-601: LGTM: Scale-up test validates the deployment update path.This test comprehensively verifies the controller's ability to detect and apply replica changes:
- Creates with 1 replica and confirms deployment matches
- Updates the CR to 3 replicas
- Reconciles and verifies the deployment scales to 3 replicas
This exercises the new update logic added in the controller (lines 327-362 of guardrailsorchestrator_controller.go).
christinaexyou
force-pushed
the
gorch-replicas-config
branch
2 times, most recently
from
80f52bb to
195237f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @controllers/gorch/guardrailsorchestrator_controller.go:
- Around line 327-344: The update path fails to include
orchestrator.Spec.TLSSecrets in tlsMounts before calling addTLSMounts, so TLS
secrets are mounted on create but not on updates; modify the update branch where
newDeployment is created (in the else after createDeployment) to append
orchestrator.Spec.TLSSecrets to tlsMounts exactly as done in the create path (or
extract that logic into a helper used by both create and update) so addTLSMounts
receives the combined tlsMounts and orchestrator.Spec.TLSSecrets before
invocation.
🧹 Nitpick comments (1)
controllers/gorch/guardrailsorchestrator_controller.go (1)
346-352: Consider merging annotations instead of overwriting.The current logic uses
existingDeployment.Spec.Template.Annotationsas the base, which preserves existing annotations but could lose any new annotations added bycreateDeployment. If the template orcreateDeploymentlogic adds new annotations in the future, they would not be applied on updates.♻️ Optional: Merge both annotation maps
- // Preserve existing annotations and add configmap hashes - annotations := existingDeployment.Spec.Template.Annotations - if annotations == nil { - annotations = map[string]string{} - } - r.setConfigMapHashAnnotations(ctx, orchestrator, annotations) - newDeployment.Spec.Template.Annotations = annotations + // Merge new and existing annotations, then add configmap hashes + annotations := make(map[string]string) + // First, copy new deployment annotations + for k, v := range newDeployment.Spec.Template.Annotations { + annotations[k] = v + } + // Then, preserve existing annotations (existing takes precedence) + for k, v := range existingDeployment.Spec.Template.Annotations { + annotations[k] = v + } + r.setConfigMapHashAnnotations(ctx, orchestrator, annotations) + newDeployment.Spec.Template.Annotations = annotations
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/gorch/v1alpha1/guardrailsorchestrator_types.goconfig/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yamlcontrollers/gorch/guardrailsorchestrator_controller.gocontrollers/gorch/guardrailsorchestrator_controller_test.gocontrollers/gorch/templates/deployment.tmpl.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/gorch/guardrailsorchestrator_controller_test.go
- api/gorch/v1alpha1/guardrailsorchestrator_types.go
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/gorch/guardrailsorchestrator_controller.go (1)
controllers/utils/status.go (1)
ReconcileFailed(22-22)
⏰ 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: Sourcery review
- GitHub Check: deploy
- GitHub Check: build
🔇 Additional comments (4)
config/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yaml (1)
218-226: LGTM! Proper validation constraints for replicas field.The
default: 1andminimum: 1constraints ensure backward compatibility (omitted replicas defaults to 1) and prevent invalid configurations (zero or negative replicas). Theint32format is appropriate for Kubernetes replica counts.controllers/gorch/templates/deployment.tmpl.yaml (1)
70-70: LGTM! Dynamic replica count from spec.The template now correctly derives the replica count from the orchestrator spec. Since the CRD enforces
default: 1andminimum: 1, this will always render a valid value.controllers/gorch/guardrailsorchestrator_controller.go (2)
22-24: LGTM! Imports are correctly used.
strconvis used increateOrchestratorCreationMetrics(line 87-88) andtimeis used for theRequeueAfterduration (line 437).
354-361: No action required. ThepatchDeploymentfunction is correctly defined incontrollers/gorch/deployment.goand properly handles theReplicasfield. It compares the entireSpecusingreflect.DeepEqualand, when changes are detected (including replica count changes), replaces the entireSpecwith a deep copy from the new deployment. Scaling changes are properly detected and applied.
* Add reconciliation logic to update deployments * Add helper function to add TLS secrets to mounts
christinaexyou
force-pushed
the
gorch-replicas-config
branch
from
195237f to
3356b5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controllers/gorch/guardrailsorchestrator_controller_test.go (1)
68-68: Optional: Consider relying on default value for consistency.Lines 68 and 97 explicitly set
Replicas: 1, while thecreateGuardrailsOrchestratorhelper (line 40-56) omits the field and relies on the default value. For consistency with the newomitemptypattern and default value of 1, you could remove the explicitReplicas: 1assignments from these helpers. This is a minor consistency improvement and not blocking.Also applies to: 97-97
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/gorch/v1alpha1/guardrailsorchestrator_types.goconfig/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yamlcontrollers/gorch/guardrailsorchestrator_controller.gocontrollers/gorch/guardrailsorchestrator_controller_test.gocontrollers/gorch/templates/deployment.tmpl.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/gorch/guardrailsorchestrator_controller.go
- controllers/gorch/templates/deployment.tmpl.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/gorch/guardrailsorchestrator_controller_test.go (2)
api/gorch/v1alpha1/guardrailsorchestrator_types.go (2)
GuardrailsOrchestrator(136-142)GuardrailsOrchestratorSpec(41-80)controllers/gorch/guardrailsorchestrator_controller.go (1)
GuardrailsOrchestratorReconciler(49-54)
⏰ 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: Sourcery review
- GitHub Check: build
- GitHub Check: deploy
🔇 Additional comments (6)
config/crd/bases/trustyai.opendatahub.io_guardrailsorchestrators.yaml (1)
218-226: LGTM! Proper defaulting and validation for replicas field.The addition of
default: 1andminimum: 1constraints ensures the replicas field has a sensible default when omitted and prevents invalid configurations with zero or negative replicas.api/gorch/v1alpha1/guardrailsorchestrator_types.go (1)
45-47: LGTM! Proper API type configuration for optional replicas field.The kubebuilder markers align with the CRD validation, and adding
omitemptyto the JSON tag follows Kubernetes API conventions for optional fields with defaults. This allows users to omit the replicas field while still getting the default value of 1.controllers/gorch/guardrailsorchestrator_controller_test.go (4)
107-124: LGTM! Well-structured test helper for custom replicas.The helper function follows the established pattern of other test helpers in this file and provides a clean way to create GuardrailsOrchestrator resources with custom replica counts for testing.
507-592: Excellent test coverage for replica scaling scenario.This test validates the complete lifecycle of scaling replicas from 1 to 3, ensuring the controller properly reconciles changes to the replica count. The test correctly uses the Get-Update-Reconcile pattern and validates both the initial and scaled states.
985-986: LGTM! Proper integration of new tests into test suite.The new replica-related tests are correctly integrated into the existing test suite alongside other GuardrailsOrchestrator controller tests.
443-505: Test correctly validates custom replica count integration with deployment template.The test properly validates that a GuardrailsOrchestrator created with custom replicas results in a deployment with the specified replica count. The deployment template uses
{{.Orchestrator.Spec.Replicas}}(line 70 of deployment.tmpl.yaml), the helper function correctly passes the replica value to the orchestrator spec, and the test verifies the integration end-to-end by confirming the deployment has 3 replicas. The test structure, cleanup, and assertions are well-implemented.
|
@christinaexyou: 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. |
|
PR needs rebase. 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. |
2 participants
This PR addresses #622 by making the orchestrator replicas configurable via the GORCH CR
Summary by Sourcery
Make GuardrailsOrchestrator replica count configurable via the CRD and use it in the generated deployment.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.