Restore zero initial scale behavior on master branch#709
Conversation
Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
WalkthroughThis update removes all dependencies, RBAC permissions, and runtime checks related to the Knative Operator and its Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant KnativeAPI
User->>Controller: Create InferenceService/InferenceGraph (MinReplicas=0)
Controller->>Controller: Check allowZeroInitialScale flag
alt allowZeroInitialScale is true
Controller->>Controller: Set initialScale annotation to "0"
else allowZeroInitialScale is false
Controller->>Controller: Do not set initialScale annotation
end
Controller->>KnativeAPI: Deploy Knative Service with annotations
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (12)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
charts/kserve-resources/templates/clusterrole.yaml(0 hunks)cmd/manager/main.go(0 hunks)config/configmap/inferenceservice.yaml(0 hunks)config/overlays/odh/inferenceservice-config-patch.yaml(0 hunks)config/overlays/test/configmap/inferenceservice-openshift-ci-serverless-predictor.yaml(0 hunks)config/rbac/role.yaml(0 hunks)go.mod(0 hunks)pkg/constants/constants.go(0 hunks)pkg/controller/v1alpha1/inferencegraph/controller.go(2 hunks)pkg/controller/v1alpha1/inferencegraph/controller_test.go(5 hunks)pkg/controller/v1alpha1/utils/utils.go(1 hunks)pkg/controller/v1beta1/inferenceservice/components/explainer.go(3 hunks)pkg/controller/v1beta1/inferenceservice/components/predictor.go(3 hunks)pkg/controller/v1beta1/inferenceservice/components/transformer.go(3 hunks)pkg/controller/v1beta1/inferenceservice/controller.go(3 hunks)pkg/controller/v1beta1/inferenceservice/controller_test.go(5 hunks)pkg/openapi/openapi_generated.go(0 hunks)pkg/testing/envtest_setup.go(0 hunks)
💤 Files with no reviewable changes (10)
- config/configmap/inferenceservice.yaml
- config/rbac/role.yaml
- go.mod
- pkg/testing/envtest_setup.go
- charts/kserve-resources/templates/clusterrole.yaml
- config/overlays/test/configmap/inferenceservice-openshift-ci-serverless-predictor.yaml
- config/overlays/odh/inferenceservice-config-patch.yaml
- cmd/manager/main.go
- pkg/constants/constants.go
- pkg/openapi/openapi_generated.go
🧰 Additional context used
🧬 Code Graph Analysis (6)
pkg/controller/v1alpha1/utils/utils.go (1)
pkg/constants/constants.go (1)
DefaultMinReplicas(174-174)
pkg/controller/v1alpha1/inferencegraph/controller.go (1)
pkg/controller/v1alpha1/utils/utils.go (1)
ValidateInitialScaleAnnotation(114-167)
pkg/controller/v1beta1/inferenceservice/components/transformer.go (1)
pkg/controller/v1alpha1/utils/utils.go (1)
ValidateInitialScaleAnnotation(114-167)
pkg/controller/v1alpha1/inferencegraph/controller_test.go (2)
pkg/constants/constants.go (3)
InferenceServiceConfigMapName(51-51)KServeNamespace(41-41)Serverless(441-441)pkg/apis/serving/v1alpha1/inference_graph.go (7)
InferenceGraph(35-40)InferenceGraphSpec(44-89)InferenceRouter(256-272)GraphRootNodeName(117-117)Sequence(103-103)InferenceStep(305-331)InferenceTarget(276-287)
pkg/controller/v1beta1/inferenceservice/components/explainer.go (1)
pkg/controller/v1alpha1/utils/utils.go (1)
ValidateInitialScaleAnnotation(114-167)
pkg/controller/v1beta1/inferenceservice/components/predictor.go (1)
pkg/controller/v1alpha1/utils/utils.go (1)
ValidateInitialScaleAnnotation(114-167)
🔇 Additional comments (24)
pkg/controller/v1alpha1/inferencegraph/controller.go (2)
286-286: LGTM! Error message properly updated.The error message correctly reflects the removal of the KnativeServing CRD dependency while maintaining clarity about the requirement for Knative Serving.
295-295: LGTM! Function call updated with required parameter.The call to
ValidateInitialScaleAnnotationcorrectly includes thegraph.Spec.MinReplicasparameter, ensuring the new zero initial scale logic works properly for InferenceGraphs.pkg/controller/v1beta1/inferenceservice/controller.go (4)
203-204: LGTM! Improved variable declaration and comment.The variable declaration is clearer and the comment better describes the early abort condition for Serverless mode.
214-214: LGTM! Error message properly updated.The error message correctly reflects the simplified check for Knative Services availability.
218-218: LGTM! Proper variable assignment without redeclaration.The variable assignment correctly avoids redeclaring
allowZeroInitialScalewhile assigning the result fromCheckZeroInitialScaleAllowed.
235-241: LGTM! Constructor calls properly updated.All component constructors (Predictor, Transformer, Explainer) now correctly receive the
allowZeroInitialScaleparameter, ensuring consistent behavior across components.pkg/controller/v1beta1/inferenceservice/components/explainer.go (4)
35-35: LGTM! Required import added.The import for
knutilsis correctly added to support the validation function call.
51-51: LGTM! New field properly added.The
allowZeroInitialScaleboolean field is appropriately added to the Explainer struct.
56-56: LGTM! Constructor properly updated.The constructor signature and field assignment correctly handle the new
allowZeroInitialScaleparameter.Also applies to: 64-64
175-175: LGTM! Validation call properly placed.The call to
ValidateInitialScaleAnnotationis correctly placed in the Knative deployment path, passing all required parameters includingisvc.Spec.Explainer.MinReplicas.pkg/controller/v1beta1/inferenceservice/components/transformer.go (4)
38-38: LGTM! Required import added.The import for
knutilsis correctly added to support the validation function call.
54-54: LGTM! New field properly added.The
allowZeroInitialScaleboolean field is appropriately added to the Transformer struct.
59-59: LGTM! Constructor properly updated.The constructor signature and field assignment correctly handle the new
allowZeroInitialScaleparameter, maintaining consistency with other components.Also applies to: 67-67
237-237: LGTM! Validation call properly placed.The call to
ValidateInitialScaleAnnotationis correctly placed in thereconcileTransformerKnativeDeploymentmethod, ensuring it only applies to Knative deployments and passes all required parameters includingisvc.Spec.Transformer.MinReplicas.pkg/controller/v1beta1/inferenceservice/components/predictor.go (4)
43-43: LGTM - Importing utility functions for validation.The import of
knutilsfrom the v1alpha1 package is appropriate for accessing the shared validation utilities.
66-66: LGTM - Clean addition of configuration field.The
allowZeroInitialScaleboolean field follows Go naming conventions and integrates well with the existing struct design.
71-71: LGTM - Constructor properly updated.The constructor signature and field assignment correctly implement the new parameter. The change maintains consistency with the existing constructor pattern.
Also applies to: 79-79
720-720: LGTM - Proper validation implementation.The call to
ValidateInitialScaleAnnotationis correctly placed and passes all required parameters:
objectMeta.Annotations: The annotations map to validate/modifyp.allowZeroInitialScale: The configuration flagisvc.Spec.Predictor.MinReplicas: The minimum replicas settingp.Log: The logger instanceThis implementation aligns with the PR objective to restore zero initial scale behavior and follows the validation logic described in the utility function.
pkg/controller/v1beta1/inferenceservice/controller_test.go (3)
284-342: LGTM! Well-structured test for zero initial scale when disallowed.This test case properly verifies that when Knative is configured to not allow zero initial scale, an InferenceService with zero min replicas does not result in the
autoscaling.knative.dev/initial-scaleannotation being set. The test follows established patterns with proper setup, resource creation, and assertion.
556-615: LGTM! Comprehensive test for zero initial scale when allowed.This test case effectively verifies that when Knative is configured to allow zero initial scale, an InferenceService with zero min replicas correctly sets the
autoscaling.knative.dev/initial-scaleannotation to "0". The test complements the previous test case and together they provide complete coverage for the zero initial scale functionality. The implementation follows best practices with proper setup, teardown, and assertions.
387-387: Good practice: Service name updates for test isolation.The service name increments (initialscale5, initialscale6, initialscale7, initialscale8) properly isolate the new test cases from existing ones, preventing resource conflicts during test execution. This follows the established naming pattern in the test suite.
Also applies to: 448-448, 509-509, 570-570
pkg/controller/v1alpha1/inferencegraph/controller_test.go (3)
251-304: LGTM! Comprehensive test for zero min replicas behavior when zero initial scale is disallowed.This test correctly verifies that when
MinReplicasis set to 0 but zero initial scale is not allowed in the Knative configuration, theautoscaling.knative.dev/initialScaleannotation is not set on the resulting Knative Service. The test structure follows the established pattern and provides good coverage for this scenario.
348-348: Good practice: Updated graph names to avoid test conflicts.The graph names have been appropriately updated from "initialscale1/2/3" to "initialscale5/6/7" to prevent conflicts with the newly added test cases. This ensures test isolation and prevents potential race conditions.
Also applies to: 403-403, 458-458
500-553: LGTM! Comprehensive test for zero min replicas behavior when zero initial scale is allowed.This test correctly verifies that when
MinReplicasis set to 0 and zero initial scale is allowed in the Knative configuration, theautoscaling.knative.dev/initialScaleannotation is explicitly set to "0" on the resulting Knative Service. This test case complements the previous test and provides complete coverage for the zero initial scale feature restoration.The test logic properly:
- Sets up the environment with zero initial scale allowed
- Creates an InferenceGraph with
MinReplicasset to 0- Verifies the annotation is set to "0" as expected
…nitial-scale-behavior-master Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
This reverts commit 053cf51 because precommit does not like the changes. Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
brettmthompson
force-pushed
the
feature/restore-zero-initial-scale-behavior-master
branch
from
3ecb026 to
11d86f0
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andresllh, brettmthompson, israel-hdez 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 |
openshift-merge-bot
Bot
merged commit 3a7402f
into
opendatahub-io:master
github-project-automation
Bot
moved this from New/Backlog
to Done
in ODH Model Serving Planning
4 participants
What this PR does / why we need it:
RHOAIENG-27519
Restoring ODH specific zero initial scale behavior enabled in #537 after syncing upstream initial scale behavior
Type of changes
Please delete options that are not relevant.
Feature/Issue validation/testing:
Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Checklist:
Release note:
Re-running failed tests
/rerun-all- rerun all failed workflows./rerun-workflow <workflow name>- rerun a specific failed workflow. Only one workflow name can be specified. Multiple /rerun-workflow commands are allowed per comment.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests