Add deployment mode switching for QueryNode #320
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hodie-aurora 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 76.15% 75.89% -0.26%
==========================================
Files 64 64
Lines 6761 6958 +197
==========================================
+ Hits 5149 5281 +132
- Misses 1406 1465 +59
- Partials 206 212 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hodie-aurora
force-pushed
the
feat/querynode-deploymode
branch
from
e55bae9 to
847f2fc
Compare
|
Thank you very much, @hodie-aurora! I'm a bit occupied with other stuffs, will review it soon |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces deployment mode switching for the QueryNode component by adding a new DeployMode field and corresponding reconciliation logic to enable switching between single (OneDeployMode) and dual (TwoDeployMode) deployment modes. Key changes include modifications to the controller logic for deployment mode reconciliation, new methods for switching deployment modes, and extensive test cases covering various operational scenarios.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/controllers/deploy_mode_changer_test.go | Added tests for ChangeToOneDeployMode with various scenarios, including error simulation. |
| pkg/controllers/deploy_mode_changer.go | Implemented ChangeToOneDeployMode logic for handling QueryNode mode switching in single deployment. |
| pkg/controllers/deploy_ctrl.go | Integrated deployment mode switching in the Reconcile flow and added parsing and error handling. |
| apis/milvus.io/v1beta1/components_types.go | Added DeployMode field in MilvusQueryNode with accompanying kubebuilder validations and defaults. |
Comments suppressed due to low confidence (2)
pkg/controllers/deploy_mode_changer_test.go:381
- [nitpick] Consider extracting the deployment name format into a shared constant for consistency and easier maintenance across both test and production code.
currentDeployName := fmt.Sprintf("%s-%s-0", QueryNode.Name, mc.Name)
pkg/controllers/deploy_mode_changer.go:232
- [nitpick] Extract the deployment naming pattern into a constant to ensure consistent formatting and reduce the risk of mismatches when referenced elsewhere.
currentDeployName := fmt.Sprintf("%s-%s-0", c.component.Name, mc.Name)
|
@haorenfsa Thanks for the quick response and for taking the time to review，No rush at all, please handle your other tasks first. This is my first code-related PR to Milvus, so if I’ve made any mistakes (like the extra commits from #291), I really appreciate your patience and guidance. Thanks again for your support! |
| // +kubebuilder:validation:Enum=OneDeployMode;TwoDeployMode | ||
| // +kubebuilder:default:=TwoDeployMode | ||
| // +kubebuilder:validation:Optional | ||
| DeployMode string `json:"deployMode,omitempty"` |
There was a problem hiding this comment.
You also need to run make generate-all after you changed the CRD.
| @@ -50,6 +50,48 @@ func (c *DeployControllerImpl) Reconcile(ctx context.Context, mc v1beta1.Milvus, | |||
| return errors.Wrap(err, "update milvus rolling mode status") | |||
| } | |||
| biz := c.bizFactory.GetBiz(component) | |||
| // Reconcile QueryNode deploy mode | |||
| if component == QueryNode { | |||
There was a problem hiding this comment.
you may move the code inside to a function named like ReconcileQueryNode() to avoid making Reconcile() too long.
| if err != nil { | ||
| return errors.Wrap(err, "scale down old deployment") | ||
| } | ||
| err = c.cli.Delete(ctx, oldDeploy) |
There was a problem hiding this comment.
I think the Above Update() is redundant. You can delete it directly, the pods will also be scaled down.
|
@haorenfsa Thank you for your guidance! I will make the suggested changes as soon as possible. I appreciate your time and effort in reviewing this PR. |
hodie-aurora
force-pushed
the
feat/querynode-deploymode
branch
from
847f2fc to
85321bd
Compare
- Added DeployMode field to MilvusQueryNode struct to support OneDeployMode and TwoDeployMode - Modified DeployControllerImpl to reconcile QueryNode deployment mode based on configuration - Implemented ChangeToOneDeployMode and ChangeToTwoDeployMode in DeployControllerBizImpl - Updated test cases in deploy_ctrl_test.go to cover deployment mode switching Signed-off-by: hodie-aurora <zhw1726195788@gmail.com>
hodie-aurora
force-pushed
the
feat/querynode-deploymode
branch
from
85321bd to
186cfb1
Compare
Signed-off-by: hodie-aurora <zhw1726195788@gmail.com>
hodie-aurora
force-pushed
the
feat/querynode-deploymode
branch
from
186cfb1 to
3e6cf9a
Compare
Overview
This PR introduces the functionality to switch between single deployment mode (
OneDeployMode) and dual deployment mode (TwoDeployMode) for the QueryNode component in Milvus. This is the first part of addressing issue #298.Changes
DeployModefield to theMilvusQueryNodestruct in thev1beta1package, supporting configurations for"OneDeployMode"or"TwoDeployMode", with a default value of"TwoDeployMode".Reconcilemethod inDeployControllerImplto reconcile the QueryNode based on the configured deployment mode.ChangeToOneDeployModeandChangeToTwoDeployModemethods inDeployControllerBizImplto manage the switching of deployment modes.deploy_ctrl_test.goto verify the logic for deployment mode switching.Purpose
Notes
Related Issue
Request
Please review the code, especially the modifications to the test cases, to ensure they meet the standards. If everything is in order, I will proceed with the HPA-related deployment mode switching functionality in the next PR after this one is merged. Thank you!