From f070af67f77379a2d33a6152c2c8244862572332 Mon Sep 17 00:00:00 2001 From: SimFG Date: Thu, 23 Jan 2025 10:01:04 +0800 Subject: [PATCH] fix: deny to set the mmap param for the alter index api when enable auto index (#39518) - issue: #39517 Signed-off-by: SimFG --- internal/datacoord/index_service.go | 28 +++++++++++++++++++++++- internal/datacoord/index_service_test.go | 22 +++++++++++++++++++ internal/proxy/task_index.go | 2 +- internal/proxy/validate_util.go | 10 +-------- pkg/common/common.go | 12 ++++++++++ tests/restful_client_v2/api/milvus.py | 4 ++-- 6 files changed, 65 insertions(+), 13 deletions(-) diff --git a/internal/datacoord/index_service.go b/internal/datacoord/index_service.go index d039bf20ae..882c044983 100644 --- a/internal/datacoord/index_service.go +++ b/internal/datacoord/index_service.go @@ -28,6 +28,7 @@ import ( "github.com/milvus-io/milvus/internal/metastore/model" "github.com/milvus-io/milvus/internal/util/indexparamcheck" "github.com/milvus-io/milvus/internal/util/vecindexmgr" + pkgcommon "github.com/milvus-io/milvus/pkg/common" "github.com/milvus-io/milvus/pkg/log" "github.com/milvus-io/milvus/pkg/metrics" "github.com/milvus-io/milvus/pkg/proto/datapb" @@ -386,8 +387,33 @@ func (s *Server) AlterIndex(ctx context.Context, req *indexpb.AlterIndexRequest) return merr.Status(merr.WrapErrParameterInvalidMsg("cannot provide both DeleteKeys and ExtraParams")), nil } + collInfo, err := s.handler.GetCollection(ctx, req.GetCollectionID()) + if err != nil { + log.Warn("failed to get collection", zap.Error(err)) + return merr.Status(err), nil + } + schemaHelper, err := typeutil.CreateSchemaHelper(collInfo.Schema) + if err != nil { + log.Warn("failed to create schema helper", zap.Error(err)) + return merr.Status(err), nil + } + + reqIndexParamMap := funcutil.KeyValuePair2Map(req.GetParams()) + for _, index := range indexes { if len(req.GetParams()) > 0 { + fieldSchema, err := schemaHelper.GetFieldFromID(index.FieldID) + if err != nil { + log.Warn("failed to get field schema", zap.Error(err)) + return merr.Status(err), nil + } + isVecIndex := typeutil.IsVectorType(fieldSchema.DataType) + err = pkgcommon.ValidateAutoIndexMmapConfig(Params.AutoIndexConfig.Enable.GetAsBool(), isVecIndex, reqIndexParamMap) + if err != nil { + log.Warn("failed to validate auto index mmap config", zap.Error(err)) + return merr.Status(err), nil + } + // update user index params newUserIndexParams := UpdateParams(index, index.UserIndexParams, req.GetParams()) log.Info("alter index user index params", @@ -426,7 +452,7 @@ func (s *Server) AlterIndex(ctx context.Context, req *indexpb.AlterIndexRequest) } } - err := s.meta.indexMeta.AlterIndex(ctx, indexes...) + err = s.meta.indexMeta.AlterIndex(ctx, indexes...) if err != nil { log.Warn("failed to alter index", zap.Error(err)) return merr.Status(err), nil diff --git a/internal/datacoord/index_service_test.go b/internal/datacoord/index_service_test.go index b551a892f2..ebf7aa70da 100644 --- a/internal/datacoord/index_service_test.go +++ b/internal/datacoord/index_service_test.go @@ -551,6 +551,23 @@ func TestServer_AlterIndex(t *testing.T) { }, } + mockHandler := NewNMockHandler(t) + + mockGetCollectionInfo := func() { + mockHandler.EXPECT().GetCollection(mock.Anything, collID).Return(&collectionInfo{ + ID: collID, + Schema: &schemapb.CollectionSchema{ + Fields: []*schemapb.FieldSchema{ + { + FieldID: fieldID, + Name: "FieldFloatVector", + DataType: schemapb.DataType_FloatVector, + }, + }, + }, + }, nil).Once() + } + s := &Server{ meta: &meta{ catalog: catalog, @@ -608,6 +625,7 @@ func TestServer_AlterIndex(t *testing.T) { }, allocator: mock0Allocator, notifyIndexChan: make(chan UniqueID, 1), + handler: mockHandler, } t.Run("server not available", func(t *testing.T) { @@ -620,6 +638,7 @@ func TestServer_AlterIndex(t *testing.T) { s.stateCode.Store(commonpb.StateCode_Healthy) t.Run("mmap_unsupported", func(t *testing.T) { + mockGetCollectionInfo() indexParams[0].Value = "GPU_CAGRA" resp, err := s.AlterIndex(ctx, req) @@ -630,6 +649,7 @@ func TestServer_AlterIndex(t *testing.T) { }) t.Run("param_value_invalied", func(t *testing.T) { + mockGetCollectionInfo() req.Params[0].Value = "abc" resp, err := s.AlterIndex(ctx, req) assert.ErrorIs(t, merr.CheckRPCCall(resp, err), merr.ErrParameterInvalid) @@ -638,6 +658,7 @@ func TestServer_AlterIndex(t *testing.T) { }) t.Run("delete_params", func(t *testing.T) { + mockGetCollectionInfo() deleteReq := &indexpb.AlterIndexRequest{ CollectionID: collID, IndexName: indexName, @@ -657,6 +678,7 @@ func TestServer_AlterIndex(t *testing.T) { } }) t.Run("update_and_delete_params", func(t *testing.T) { + mockGetCollectionInfo() updateAndDeleteReq := &indexpb.AlterIndexRequest{ CollectionID: collID, IndexName: indexName, diff --git a/internal/proxy/task_index.go b/internal/proxy/task_index.go index a158f75a25..94d20468f3 100644 --- a/internal/proxy/task_index.go +++ b/internal/proxy/task_index.go @@ -594,7 +594,7 @@ func (t *alterIndexTask) SetID(uid UniqueID) { } func (t *alterIndexTask) Name() string { - return CreateIndexTaskName + return AlterIndexTaskName } func (t *alterIndexTask) Type() commonpb.MsgType { diff --git a/internal/proxy/validate_util.go b/internal/proxy/validate_util.go index 8dff4b62a1..b555d7c82c 100644 --- a/internal/proxy/validate_util.go +++ b/internal/proxy/validate_util.go @@ -932,13 +932,5 @@ func newValidateUtil(opts ...validateOption) *validateUtil { } func ValidateAutoIndexMmapConfig(isVectorField bool, indexParams map[string]string) error { - if !Params.AutoIndexConfig.Enable.GetAsBool() { - return nil - } - - _, ok := indexParams[common.MmapEnabledKey] - if ok && isVectorField { - return fmt.Errorf("mmap index is not supported to config for the collection in auto index mode") - } - return nil + return common.ValidateAutoIndexMmapConfig(Params.AutoIndexConfig.Enable.GetAsBool(), isVectorField, indexParams) } diff --git a/pkg/common/common.go b/pkg/common/common.go index e03ff150b5..1f95438411 100644 --- a/pkg/common/common.go +++ b/pkg/common/common.go @@ -430,3 +430,15 @@ func GetReplicateEndTS(kvs []*commonpb.KeyValuePair) (uint64, bool) { } return 0, false } + +func ValidateAutoIndexMmapConfig(autoIndexConfigEnable, isVectorField bool, indexParams map[string]string) error { + if !autoIndexConfigEnable { + return nil + } + + _, ok := indexParams[MmapEnabledKey] + if ok && isVectorField { + return fmt.Errorf("mmap index is not supported to config for the collection in auto index mode") + } + return nil +} diff --git a/tests/restful_client_v2/api/milvus.py b/tests/restful_client_v2/api/milvus.py index 8a8827d9b4..dc52efad6d 100644 --- a/tests/restful_client_v2/api/milvus.py +++ b/tests/restful_client_v2/api/milvus.py @@ -525,7 +525,7 @@ class CollectionClient(Requests): url = f"{self.endpoint}/v2/vectordb/collections/drop_properties" payload = { "collectionName": collection_name, - "deleteKeys": delete_keys + "propertyKeys": delete_keys } if self.db_name is not None: payload["dbName"] = self.db_name @@ -570,7 +570,7 @@ class CollectionClient(Requests): payload = { "collectionName": collection_name, "indexName": index_name, - "deleteKeys": delete_keys + "propertyKeys": delete_keys } if self.db_name is not None: payload["dbName"] = self.db_name