fix: 修复安全组合并导致重复计算虚拟机绑定安全组数量&优化安全组操作 (#7496)

Co-authored-by: Qu Xuan <quxuan@yunionyun.com>

cmd/climc/shell/compute/servers.go

@@ -494,11 +494,7 @@ func init() {
 	})
 
 	R(&options.ServerSecGroupsOptions{}, "server-set-secgroup", "Set security groups to a VM", func(s *mcclient.ClientSession, opts *options.ServerSecGroupsOptions) error {
-		params, err := options.StructToParams(opts)
-		if err != nil {
-			return err
-		}
-		srv, err := modules.Servers.PerformAction(s, opts.ID, "set-secgroup", params)
+		srv, err := modules.Servers.PerformAction(s, opts.ID, "set-secgroup", opts.Parmas())
 		if err != nil {
 			return err
 		}
@@ -507,11 +503,7 @@ func init() {
 	})
 
 	R(&options.ServerSecGroupsOptions{}, "server-add-secgroup", "Add security group to a VM", func(s *mcclient.ClientSession, opts *options.ServerSecGroupsOptions) error {
-		params, err := options.StructToParams(opts)
-		if err != nil {
-			return err
-		}
-		srv, err := modules.Servers.PerformAction(s, opts.ID, "add-secgroup", params)
+		srv, err := modules.Servers.PerformAction(s, opts.ID, "add-secgroup", opts.Parmas())
 		if err != nil {
 			return err
 		}
@@ -546,11 +538,7 @@ func init() {
 	})
 
 	R(&options.ServerSecGroupsOptions{}, "server-revoke-secgroup", "Revoke security group from VM", func(s *mcclient.ClientSession, opts *options.ServerSecGroupsOptions) error {
-		params, err := options.StructToParams(opts)
-		if err != nil {
-			return err
-		}
-		srv, err := modules.Servers.PerformAction(s, opts.ID, "revoke-secgroup", params)
+		srv, err := modules.Servers.PerformAction(s, opts.ID, "revoke-secgroup", opts.Parmas())
 		if err != nil {
 			return err
 		}

pkg/apis/compute/guests.go

@@ -356,3 +356,53 @@ type GuestMigrateInput struct {
 type GuestLiveMigrateInput struct {
 	PreferHost string `json:"prefer_host"`
 }
+
+type GuestSetSecgroupInput struct {
+	// 安全组Id列表
+	// 实例必须处于运行,休眠或者关机状态
+	//
+	//
+	// | 平台		 | 最多绑定安全组数量	|
+	// |-------------|-------------------	|
+	// | Azure       | 1					|
+	// | VMware      | 不支持安全组			|
+	// | Baremetal   | 不支持安全组			|
+	// | ZStack	     | 1					|
+	// | 其他	     | 5					|
+	SecgroupIds []string `json:"secgroup_ids"`
+}
+
+type GuestRevokeSecgroupInput struct {
+	// 安全组Id列表
+	// 实例必须处于运行,休眠或者关机状态
+	SecgroupIds []string `json:"secgroup_ids"`
+}
+
+type GuestAssignSecgroupInput struct {
+	// 安全组Id
+	// 实例必须处于运行,休眠或者关机状态
+	SecgroupId string `json:"secgroup_id"`
+
+	// swagger:ignore
+	// Deprecated
+	Secgrp string `json:"secgrp" "yunion:deprecated-by":"secgroup_id"`
+
+	// swagger:ignore
+	// Deprecated
+	Secgroup string `json:"secgroup" "yunion:deprecated-by":"secgroup_id"`
+}
+
+type GuestAddSecgroupInput struct {
+	// 安全组Id列表
+	// 实例必须处于运行,休眠或者关机状态
+	//
+	//
+	// | 平台		 | 最多绑定安全组数量	|
+	// |-------------|-------------------	|
+	// | Azure       | 1					|
+	// | VMware      | 不支持安全组			|
+	// | Baremetal   | 不支持安全组			|
+	// | ZStack	     | 1					|
+	// | 其他	     | 5					|
+	SecgroupIds []string `json:"secgroup_ids"`
+}

pkg/apis/compute/secgroup.go

@@ -248,3 +248,12 @@ type GuestsecgroupDetails struct {
 	// 安全组名称
 	Secgroup string `json:"secgroup"`
 }
+
+type SecgroupMergeInput struct {
+	// 安全组id列表
+	SecgroupIds []string `json:"secgroup_ids"`
+
+	// swagger:ignore
+	// Deprecated
+	Secgroups []string `json:"secgroup" "yunion:deprecated-by":"secgroup_ids"`
+}

pkg/apis/compute/secgroup_const.go

@@ -17,4 +17,6 @@ package compute
 const (
 	SECGROUP_STATUS_READY    = "ready"
 	SECGROUP_STATUS_DELETING = "deleting"
+
+	SECGROUP_DEFAULT_ID = "default"
 )

pkg/apis/compute/zz_generated.model.go

@@ -1592,8 +1592,9 @@ type SNetwork struct {
 	// 分配策略
 	AllocPolicy        string `json:"alloc_policy"`
 	AllocTimoutSeconds int    `json:"alloc_timout_seconds"`
-	// 是否自动分配IP
-	IsAutoAlloc bool `json:"is_auto_alloc"`
+	// 该网段是否用于自动分配IP地址，如果为false，则用户需要明确选择该网段，才会使用该网段分配IP，
+	// 如果为true，则用户不指定网段时，则自动从该值为true的网络中选择一个分配地址
+	IsAutoAlloc *bool `json:"is_auto_alloc,omitempty"`
 }
 
 // SNetworkInterface is an autogenerated struct via yunion.io/x/onecloud/pkg/compute/models.SNetworkInterface.

pkg/compute/guestdrivers/managedvirtual.go

@@ -319,7 +319,10 @@ func (self *SManagedVirtualizedGuestDriver) RequestDeployGuestOnHost(ctx context
 			return errors.Wrap(err, "GetSecurityGroupVpcId")
 		}
 
-		secgroups := guest.GetSecgroups()
+		secgroups, err := guest.GetSecgroups()
+		if err != nil {
+			return errors.Wrap(err, "GetSecgroups")
+		}
 		for i, secgroup := range secgroups {
 			externalId, err := region.GetDriver().RequestSyncSecurityGroup(ctx, task.GetUserCred(), vpcId, vpc, &secgroup, desc.ProjectId)
 			if err != nil {
@@ -985,7 +988,10 @@ func (self *SManagedVirtualizedGuestDriver) RequestSyncSecgroupsOnHost(ctx conte
 		}
 	}
 
-	secgroups := guest.GetSecgroups()
+	secgroups, err := guest.GetSecgroups()
+	if err != nil {
+		return errors.Wrap(err, "GetSecgroups")
+	}
 	externalIds := []string{}
 	for _, secgroup := range secgroups {
 		externalId, err := region.GetDriver().RequestSyncSecurityGroup(ctx, task.GetUserCred(), vpcId, vpc, &secgroup, remoteProjectId)

pkg/compute/models/cloudsync.go

@@ -740,14 +740,7 @@ func syncVMSecgroups(ctx context.Context, userCred mcclient.TokenCredential, pro
 		// log.Errorf(msg)
 		return errors.Wrap(err, "remoteVM.GetSecurityGroupIds")
 	}
-	result := localVM.SyncVMSecgroups(ctx, userCred, provider, secgroupIds)
-	msg := result.Result()
-	log.Infof("SyncVMSecgroups for VM %s result: %s", localVM.Name, msg)
-	if result.IsError() {
-		return result.AllError()
-	}
-	// db.OpsLog.LogEvent(provider, db.ACT_SYNC_HOST_COMPLETE, msg, userCred)
-	return nil
+	return localVM.SyncVMSecgroups(ctx, userCred, secgroupIds)
 }
 
 func syncSkusFromPrivateCloud(ctx context.Context, userCred mcclient.TokenCredential, syncResults SSyncResultSet, provider *SCloudprovider, remoteRegion cloudprovider.ICloudRegion) {

pkg/compute/models/guest_actions.go

@@ -25,6 +25,8 @@ import (
 	"time"
 	"unicode"
 
+	"gopkg.in/fatih/set.v0"
+
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
 	"yunion.io/x/pkg/errors"
@@ -1075,61 +1077,63 @@ func (self *SGuest) AllowPerformAddSecgroup(ctx context.Context, userCred mcclie
 	return self.IsOwner(userCred)
 }
 
-func (self *SGuest) PerformAddSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) (jsonutils.JSONObject, error) {
+// 绑定多个安全组
+func (self *SGuest) PerformAddSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAddSecgroupInput) (jsonutils.JSONObject, error) {
 	if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) {
-		return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status)
+		return nil, httperrors.NewInputParameterError("Cannot add security groups in status %s", self.Status)
 	}
 
 	maxCount := self.GetDriver().GetMaxSecurityGroupCount()
 	if maxCount == 0 {
-		return nil, httperrors.NewUnsupportOperationError("Cannot assign security group for this guest %s", self.Name)
+		return nil, httperrors.NewUnsupportOperationError("Cannot add security groups for hypervisor %s", self.Hypervisor)
 	}
 
-	secgrpJsonArray := jsonutils.GetArrayOfPrefix(data, "secgrp")
-	if len(secgrpJsonArray) == 0 {
-		return nil, httperrors.NewInputParameterError("Missing secgrp.0 secgrp.1 ... parameters")
+	if len(input.SecgroupIds) == 0 {
+		return nil, httperrors.NewMissingParameterError("secgroup_ids")
 	}
 
-	originSecgroups := self.GetSecgroups()
-	if len(originSecgroups)+len(secgrpJsonArray) > maxCount {
+	secgroups, err := self.GetSecgroups()
+	if err != nil {
+		return nil, httperrors.NewGeneralError(errors.Wrap(err, "GetSecgroups"))
+	}
+	if len(secgroups)+len(input.SecgroupIds) > maxCount {
 		return nil, httperrors.NewUnsupportOperationError("guest %s band to up to %d security groups", self.Name, maxCount)
 	}
 
-	originSecgroupIds := []string{}
-	for _, secgroup := range originSecgroups {
-		originSecgroupIds = append(originSecgroupIds, secgroup.Id)
+	secgroupIds := []string{}
+	for _, secgroup := range secgroups {
+		secgroupIds = append(secgroupIds, secgroup.Id)
 	}
 
-	newSecgroups := []*SSecurityGroup{}
-	newSecgroupNames := []string{}
-	for idx := 0; idx < len(secgrpJsonArray); idx++ {
-		secgroupId, _ := secgrpJsonArray[idx].GetString()
+	secgroupNames := []string{}
+	for _, secgroupId := range input.SecgroupIds {
 		secgrp, err := SecurityGroupManager.FetchByIdOrName(userCred, secgroupId)
 		if err != nil {
-			if err == sql.ErrNoRows {
-				return nil, httperrors.NewInputParameterError("failed to find secgroup %s for params %s", secgroupId, fmt.Sprintf("secgrp.%d", idx))
+			if errors.Cause(err) == sql.ErrNoRows {
+				return nil, httperrors.NewResourceNotFoundError2("secgroup", secgroupId)
 			}
-			return nil, httperrors.NewGeneralError(err)
+			return nil, httperrors.NewGeneralError(errors.Wrapf(err, "SecurityGroupManager.FetchByIdOrName(%s)", secgroupId))
 		}
 
-		if err := SecurityGroupManager.ValidateName(secgrp.GetName()); err != nil {
+		err = SecurityGroupManager.ValidateName(secgrp.GetName())
+		if err != nil {
 			return nil, httperrors.NewInputParameterError("The secgroup name %s does not meet the requirements, please change the name", secgrp.GetName())
 		}
 
-		if utils.IsInStringArray(secgrp.GetId(), originSecgroupIds) {
+		if utils.IsInStringArray(secgrp.GetId(), secgroupIds) {
 			return nil, httperrors.NewInputParameterError("security group %s has already been assigned to guest %s", secgrp.GetName(), self.Name)
 		}
-		newSecgroups = append(newSecgroups, secgrp.(*SSecurityGroup))
-		newSecgroupNames = append(newSecgroupNames, secgrp.GetName())
+		secgroupIds = append(secgroupIds, secgrp.GetId())
+		secgroupNames = append(secgroupNames, secgrp.GetName())
 	}
 
-	for _, secgroup := range newSecgroups {
-		if _, err := GuestsecgroupManager.newGuestSecgroup(ctx, userCred, self, secgroup); err != nil {
-			return nil, httperrors.NewInputParameterError(err.Error())
-		}
+	err = self.saveSecgroups(ctx, userCred, secgroupIds)
+	if err != nil {
+		return nil, httperrors.NewGeneralError(errors.Wrap(err, "saveSecgroups"))
 	}
 
-	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_ASSIGNSECGROUP, fmt.Sprintf("secgroups: %s", strings.Join(newSecgroupNames, ",")), userCred, true)
+	notes := map[string][]string{"secgroups": secgroupNames}
+	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_ASSIGNSECGROUP, notes, userCred, true)
 	return nil, self.StartSyncTask(ctx, userCred, true, "")
 }
 
@@ -1148,101 +1152,93 @@ func (self *SGuest) saveDefaultSecgroupId(userCred mcclient.TokenCredential, sec
 			return nil
 		})
 		if err != nil {
-			log.Errorf("saveDefaultSecgroupId fail %s", err)
-			return err
+			return errors.Wrap(err, "db.Update")
 		}
 		db.OpsLog.LogEvent(self, db.ACT_UPDATE, diff, userCred)
 	}
 	return nil
 }
 
-func (self *SGuest) revokeSecgroup(ctx context.Context, userCred mcclient.TokenCredential, secgroup *SSecurityGroup) error {
-	if secgroup == nil {
-		return fmt.Errorf("failed to revoke null secgroup")
-	}
-	if self.SecgrpId != secgroup.Id {
-		return GuestsecgroupManager.DeleteGuestSecgroup(ctx, userCred, self, secgroup)
-	}
-	secgroups := self.GetSecgroups()
-	if len(secgroups) <= 1 {
-		return self.saveDefaultSecgroupId(userCred, "default")
-	}
-	for _, _secgroup := range secgroups {
-		// 从guestsecgroups中移除一个安全组，并将guest的 secgroupId 设为此安全组ID
-		if _secgroup.Id != secgroup.Id {
-			err := GuestsecgroupManager.DeleteGuestSecgroup(ctx, userCred, self, &_secgroup)
-			if err != nil {
-				return err
-			}
-			return self.saveDefaultSecgroupId(userCred, _secgroup.Id)
-		}
-	}
-	return nil
-}
-
-func (self *SGuest) PerformRevokeSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) (jsonutils.JSONObject, error) {
+func (self *SGuest) PerformRevokeSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestRevokeSecgroupInput) (jsonutils.JSONObject, error) {
 	if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) {
-		return nil, httperrors.NewInputParameterError("Cannot revoke security rules in status %s", self.Status)
+		return nil, httperrors.NewInputParameterError("Cannot revoke security groups in status %s", self.Status)
 	}
 
-	revokeSecgroups := []*SSecurityGroup{}
-	secgrpJsonArray := jsonutils.GetArrayOfPrefix(data, "secgrp")
-	originSecgroups := self.GetSecgroups()
-	originSecgroupIds := []string{}
-	for _, originSecgroup := range originSecgroups {
-		originSecgroupIds = append(originSecgroupIds, originSecgroup.Id)
+	if len(input.SecgroupIds) == 0 {
+		return nil, nil
 	}
 
-	if len(secgrpJsonArray) == 0 {
-		revokeSecgroups = append(revokeSecgroups, self.getSecgroup())
+	secgroups, err := self.GetSecgroups()
+	if err != nil {
+		return nil, httperrors.NewGeneralError(errors.Wrap(err, "GetSecgroups"))
+	}
+	secgroupMaps := map[string]string{}
+	for _, secgroup := range secgroups {
+		secgroupMaps[secgroup.Id] = secgroup.Name
 	}
 
-	revokeSecgroupNames := []string{}
-	for idx := 0; idx < len(secgrpJsonArray); idx++ {
-		secgroupId, _ := secgrpJsonArray[idx].GetString()
+	secgroupNames := []string{}
+	for _, secgroupId := range input.SecgroupIds {
 		secgrp, err := SecurityGroupManager.FetchByIdOrName(userCred, secgroupId)
 		if err != nil {
-			if err == sql.ErrNoRows {
-				return nil, httperrors.NewInputParameterError("failed to find secgroup %s for params %s", secgroupId, fmt.Sprintf("secgrp.%d", idx))
+			if errors.Cause(err) == sql.ErrNoRows {
+				return nil, httperrors.NewResourceNotFoundError2("secgroup", secgroupId)
 			}
-			return nil, httperrors.NewGeneralError(err)
+			return nil, httperrors.NewGeneralError(errors.Wrapf(err, "SecurityGroupManager.FetchByIdOrName(%s)", secgroupId))
 		}
-		if !utils.IsInStringArray(secgrp.GetId(), originSecgroupIds) {
+		_, ok := secgroupMaps[secgrp.GetId()]
+		if !ok {
 			return nil, httperrors.NewInputParameterError("security group %s not assigned to guest %s", secgrp.GetName(), self.Name)
 		}
-		revokeSecgroups = append(revokeSecgroups, secgrp.(*SSecurityGroup))
-		revokeSecgroupNames = append(revokeSecgroupNames, secgrp.GetName())
+		delete(secgroupMaps, secgrp.GetId())
+		secgroupNames = append(secgroupNames, secgrp.GetName())
 	}
 
-	for _, secgroup := range revokeSecgroups {
-		if err := self.revokeSecgroup(ctx, userCred, secgroup); err != nil {
-			return nil, err
-		}
+	secgrpIds := []string{}
+	for secgroupId := range secgroupMaps {
+		secgrpIds = append(secgrpIds, secgroupId)
 	}
-	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_REVOKESECGROUP, fmt.Sprintf("secgroups: %s", strings.Join(revokeSecgroupNames, ",")), userCred, true)
+
+	err = self.saveSecgroups(ctx, userCred, secgrpIds)
+	if err != nil {
+		return nil, httperrors.NewGeneralError(errors.Wrap(err, "saveSecgroups"))
+	}
+
+	notes := map[string][]string{"secgroups": secgroupNames}
+	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_REVOKESECGROUP, notes, userCred, true)
 	return nil, self.StartSyncTask(ctx, userCred, true, "")
 }
 
-func (self *SGuest) PerformAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) (jsonutils.JSONObject, error) {
+// +onecloud:swagger-gen-ignore
+func (self *SGuest) PerformAssignSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestAssignSecgroupInput) (jsonutils.JSONObject, error) {
 	if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) {
 		return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status)
 	}
-	secgrpV := validators.NewModelIdOrNameValidator("secgrp", "secgroup", userCred)
-	if err := secgrpV.Validate(data.(*jsonutils.JSONDict)); err != nil {
-		return nil, err
+
+	if len(input.SecgroupId) == 0 {
+		return nil, httperrors.NewMissingParameterError("secgroup_id")
 	}
 
-	err := SecurityGroupManager.ValidateName(secgrpV.Model.GetName())
+	secgroup, err := SecurityGroupManager.FetchByIdOrName(userCred, input.SecgroupId)
 	if err != nil {
-		return nil, httperrors.NewInputParameterError("The secgroup name %s does not meet the requirements, please change the name", secgrpV.Model.GetName())
+		if errors.Cause(err) == sql.ErrNoRows {
+			return nil, httperrors.NewResourceNotFoundError2("secgroup", input.SecgroupId)
+		}
+		return nil, httperrors.NewGeneralError(errors.Wrapf(err, "SecurityGroupManager.FetchByIdOrName(%s)", input.SecgroupId))
 	}
 
-	err = self.saveDefaultSecgroupId(userCred, secgrpV.Model.GetId())
+	err = SecurityGroupManager.ValidateName(secgroup.GetName())
+	if err != nil {
+		return nil, httperrors.NewInputParameterError("The secgroup name %s does not meet the requirements, please change the name", secgroup.GetName())
+	}
+
+	err = self.saveDefaultSecgroupId(userCred, secgroup.GetId())
 	if err != nil {
 		return nil, err
 	}
 
-	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_ASSIGNSECGROUP, fmt.Sprintf("secgroup: %s", secgrpV.Model.GetName()), userCred, true)
+	notes := map[string]string{"name": secgroup.GetName(), "id": secgroup.GetId()}
+	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_ASSIGNSECGROUP, notes, userCred, true)
 	return nil, self.StartSyncTask(ctx, userCred, true, "")
 }
 
@@ -1250,61 +1246,110 @@ func (self *SGuest) AllowPerformSetSecgroup(ctx context.Context, userCred mcclie
 	return self.IsOwner(userCred) || db.IsAdminAllowPerform(userCred, self, "set-secgroup")
 }
 
-func (self *SGuest) PerformSetSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) (jsonutils.JSONObject, error) {
+// 全量覆盖安全组
+func (self *SGuest) PerformSetSecgroup(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.GuestSetSecgroupInput) (jsonutils.JSONObject, error) {
 	if !utils.IsInStringArray(self.Status, []string{api.VM_READY, api.VM_RUNNING, api.VM_SUSPEND}) {
-		return nil, httperrors.NewInputParameterError("Cannot assign security rules in status %s", self.Status)
+		return nil, httperrors.NewInputParameterError("Cannot set security rules in status %s", self.Status)
 	}
-	secgrpJsonArray := jsonutils.GetArrayOfPrefix(data, "secgrp")
-	if len(secgrpJsonArray) == 0 {
-		return nil, httperrors.NewInputParameterError("Missing secgrp.0 secgrp.1 ... parameters")
+	if len(input.SecgroupIds) == 0 {
+		return nil, httperrors.NewMissingParameterError("secgroup_ids")
 	}
 
 	maxCount := self.GetDriver().GetMaxSecurityGroupCount()
 	if maxCount == 0 {
-		return nil, httperrors.NewUnsupportOperationError("Cannot assign security group for this guest %s", self.Name)
+		return nil, httperrors.NewUnsupportOperationError("Cannot set security group for this guest %s", self.Name)
 	}
 
-	if len(secgrpJsonArray) > maxCount {
+	if len(input.SecgroupIds) > maxCount {
 		return nil, httperrors.NewUnsupportOperationError("guest %s band to up to %d security groups", self.Name, maxCount)
 	}
 
-	setSecgroups := []*SSecurityGroup{}
-	setSecgroupNames := []string{}
-	for idx := 0; idx < len(secgrpJsonArray); idx++ {
-		secgroupId, _ := secgrpJsonArray[idx].GetString()
+	secgroupIds := []string{}
+	secgroupNames := []string{}
+	for _, secgroupId := range input.SecgroupIds {
 		secgrp, err := SecurityGroupManager.FetchByIdOrName(userCred, secgroupId)
 		if err != nil {
-			if err == sql.ErrNoRows {
-				return nil, httperrors.NewInputParameterError("failed to find secgroup %s for params %s", secgroupId, fmt.Sprintf("secgrp.%d", idx))
+			if errors.Cause(err) == sql.ErrNoRows {
+				return nil, httperrors.NewResourceNotFoundError2("secgroup", secgroupId)
 			}
-			return nil, httperrors.NewGeneralError(err)
+			return nil, httperrors.NewGeneralError(errors.Wrapf(err, "FetchByIdOrName(%s)", secgroupId))
 		}
 
-		if err := SecurityGroupManager.ValidateName(secgrp.GetName()); err != nil {
+		err = SecurityGroupManager.ValidateName(secgrp.GetName())
+		if err != nil {
 			return nil, httperrors.NewInputParameterError("The secgroup name %s does not meet the requirements, please change the name", secgrp.GetName())
 		}
 
-		setSecgroups = append(setSecgroups, secgrp.(*SSecurityGroup))
-		setSecgroupNames = append(setSecgroupNames, secgrp.GetName())
+		if !utils.IsInStringArray(secgrp.GetId(), secgroupIds) {
+			secgroupIds = append(secgroupIds, secgrp.GetId())
+			secgroupNames = append(secgroupNames, secgrp.GetName())
+		}
 	}
 
-	err := self.RevokeAllSecgroups(ctx, userCred)
+	err := self.saveSecgroups(ctx, userCred, secgroupIds)
 	if err != nil {
-		return nil, err
+		return nil, httperrors.NewGeneralError(errors.Wrapf(err, "saveSecgroups"))
 	}
 
-	for i := 0; i < len(setSecgroups); i++ {
-		if i == 0 {
-			err = self.saveDefaultSecgroupId(userCred, setSecgroups[i].Id)
-		} else {
-			_, err = GuestsecgroupManager.newGuestSecgroup(ctx, userCred, self, setSecgroups[i])
-		}
-		if err != nil {
-			return nil, httperrors.NewInputParameterError(err.Error())
+	notes := map[string][]string{"secgroups": secgroupNames}
+	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_SETSECGROUP, notes, userCred, true)
+	return nil, self.StartSyncTask(ctx, userCred, true, "")
+}
+
+func (self *SGuest) GetGuestSecgroups() ([]SGuestsecgroup, error) {
+	gss := []SGuestsecgroup{}
+	q := GuestsecgroupManager.Query().Equals("guest_id", self.Id)
+	err := db.FetchModelObjects(GuestsecgroupManager, q, &gss)
+	if err != nil {
+		return nil, errors.Wrapf(err, "db.FetchModelObjects")
+	}
+	return gss, nil
+}
+
+func (self *SGuest) saveSecgroups(ctx context.Context, userCred mcclient.TokenCredential, secgroupIds []string) error {
+	if len(secgroupIds) == 0 {
+		return self.RevokeAllSecgroups(ctx, userCred)
+	}
+	oldIds := set.New(set.ThreadSafe)
+	newIds := set.New(set.ThreadSafe)
+	gss, err := self.GetGuestSecgroups()
+	if err != nil {
+		return errors.Wrapf(err, "GetGuestSecgroups")
+	}
+	secgroupMaps := map[string]SGuestsecgroup{}
+	for i := range gss {
+		oldIds.Add(gss[i].SecgroupId)
+		secgroupMaps[gss[i].SecgroupId] = gss[i]
+	}
+	for i := 1; i < len(secgroupIds); i++ {
+		newIds.Add(secgroupIds[i])
+	}
+	for _, removed := range set.Difference(oldIds, newIds).List() {
+		id := removed.(string)
+		gs, ok := secgroupMaps[id]
+		if ok {
+			err = gs.Delete(ctx, userCred)
+			if err != nil {
+				return errors.Wrapf(err, "Delete guest secgroup for guest %s secgroup %s", self.Name, id)
+			}
 		}
 	}
-	logclient.AddActionLogWithContext(ctx, self, logclient.ACT_VM_SETSECGROUP, fmt.Sprintf("secgroups: %s", strings.Join(setSecgroupNames, ",")), userCred, true)
-	return nil, self.StartSyncTask(ctx, userCred, true, "")
+	for _, added := range set.Difference(newIds, oldIds).List() {
+		id := added.(string)
+		err = self.newGuestSecgroup(ctx, id)
+		if err != nil {
+			return errors.Wrapf(err, "New guest secgroup for guest %s with secgroup %s", self.Name, id)
+		}
+	}
+	return self.saveDefaultSecgroupId(userCred, secgroupIds[0])
+}
+
+func (self *SGuest) newGuestSecgroup(ctx context.Context, secgroupId string) error {
+	gs := &SGuestsecgroup{}
+	gs.SetModelManager(GuestsecgroupManager, gs)
+	gs.GuestId = self.Id
+	gs.SecgroupId = secgroupId
+	return GuestsecgroupManager.TableSpec().Insert(ctx, gs)
 }
 
 func (self *SGuest) AllowPerformPurge(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) bool {
@@ -2541,14 +2586,17 @@ func (self *SGuest) StartChangeConfigTask(ctx context.Context, userCred mcclient
 }
 
 func (self *SGuest) RevokeAllSecgroups(ctx context.Context, userCred mcclient.TokenCredential) error {
-	err := GuestsecgroupManager.DeleteGuestSecgroup(ctx, userCred, self, nil)
+	gss, err := self.GetGuestSecgroups()
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "GetGuestSecgroups")
 	}
-	if secgroup := self.getSecgroup(); secgroup != nil {
-		return self.revokeSecgroup(ctx, userCred, secgroup)
+	for i := range gss {
+		err = gss[i].Delete(ctx, userCred)
+		if err != nil {
+			return errors.Wrap(err, "Delete")
+		}
 	}
-	return nil
+	return self.saveDefaultSecgroupId(userCred, api.SECGROUP_DEFAULT_ID)
 }
 
 func (self *SGuest) DoPendingDelete(ctx context.Context, userCred mcclient.TokenCredential) {

pkg/compute/models/guests.go

@@ -2193,15 +2193,20 @@ func (self *SGuest) IsWindows() bool {
 	}
 }
 
-func (self *SGuest) getSecgroupJson() []jsonutils.JSONObject {
-	secgroups := []jsonutils.JSONObject{}
-	for _, secGrp := range self.GetSecgroups() {
-		secgroups = append(secgroups, secGrp.getDesc())
+func (self *SGuest) getSecgroupJson() ([]jsonutils.JSONObject, error) {
+	objs := []jsonutils.JSONObject{}
+
+	secgroups, err := self.GetSecgroups()
+	if err != nil {
+		return nil, errors.Wrap(err, "GetSecgroups")
 	}
-	return secgroups
+	for _, secGrp := range secgroups {
+		objs = append(objs, secGrp.getDesc())
+	}
+	return objs, nil
 }
 
-func (self *SGuest) GetSecgroups() []SSecurityGroup {
+func (self *SGuest) GetSecgroups() ([]SSecurityGroup, error) {
 	secgrpQuery := SecurityGroupManager.Query()
 	secgrpQuery.Filter(
 		sqlchemy.OR(
@@ -2210,11 +2215,11 @@ func (self *SGuest) GetSecgroups() []SSecurityGroup {
 		),
 	)
 	secgroups := []SSecurityGroup{}
-	if err := db.FetchModelObjects(SecurityGroupManager, secgrpQuery, &secgroups); err != nil {
-		log.Errorf("Get security group error: %v", err)
-		return nil
+	err := db.FetchModelObjects(SecurityGroupManager, secgrpQuery, &secgroups)
+	if err != nil {
+		return nil, errors.Wrapf(err, "db.FetchModelObjects")
 	}
-	return secgroups
+	return secgroups, nil
 }
 
 func (self *SGuest) getSecgroup() *SSecurityGroup {
@@ -2268,7 +2273,7 @@ func (self *SGuest) getSecurityRules() string {
 
 //获取多个安全组规则，优先级降序排序
 func (self *SGuest) getSecurityGroupsRules() string {
-	secgroups := self.GetSecgroups()
+	secgroups, _ := self.GetSecgroups()
 	secgroupids := []string{}
 	for _, secgroup := range secgroups {
 		secgroupids = append(secgroupids, secgroup.Id)
@@ -3860,7 +3865,8 @@ func (self *SGuest) GetJsonDescAtHypervisor(ctx context.Context, host *SHost) *j
 		desc.Add(jsonutils.NewString(secGrp.Name), "secgroup")
 	}
 
-	if secgroups := self.getSecgroupJson(); len(secgroups) > 0 {
+	secgroups, _ := self.getSecgroupJson()
+	if secgroups != nil {
 		desc.Add(jsonutils.NewArray(secgroups...), "secgroups")
 	}
 
@@ -4696,92 +4702,32 @@ func (self *SGuest) SyncVMEip(ctx context.Context, userCred mcclient.TokenCreden
 	return result
 }
 
-func (self *SGuest) getSecgroupExternalIds(provider *SCloudprovider) []string {
-	secgroups := self.GetSecgroups()
-	secgroupids := []string{}
-	for i := 0; i < len(secgroups); i++ {
-		secgroupids = append(secgroupids, secgroups[i].Id)
+func (self *SGuest) getSecgroupsBySecgroupExternalIds(externalIds []string) ([]SSecurityGroup, error) {
+	host := self.GetHost()
+	if host == nil {
+		return nil, errors.Error("not found host for guest")
 	}
-	q := SecurityGroupCacheManager.Query().Equals("manager_id", provider.Id)
-	q = q.Filter(sqlchemy.In(q.Field("secgroup_id"), secgroupids))
-	secgroupcaches := []SSecurityGroupCache{}
-	if err := db.FetchModelObjects(SecurityGroupCacheManager, q, &secgroupcaches); err != nil {
-		log.Errorf("failed to fetch secgroupcaches for provider %s error: %v", provider.Name, err)
-		return nil
-	}
-	externalIds := []string{}
-	for i := 0; i < len(secgroupcaches); i++ {
-		externalIds = append(externalIds, secgroupcaches[i].ExternalId)
-	}
-	return externalIds
-}
-
-func (self *SGuest) getSecgroupByCache(provider *SCloudprovider, externalId string) (*SSecurityGroup, error) {
-	q := SecurityGroupCacheManager.Query().Equals("manager_id", provider.Id).Equals("external_id", externalId)
-	cache := SSecurityGroupCache{}
-	cache.SetModelManager(SecurityGroupCacheManager, &cache)
-	count, err := q.CountWithError()
+	sq := SecurityGroupCacheManager.Query("secgroup_id").In("external_id", externalIds).Equals("manager_id", host.ManagerId)
+	q := SecurityGroupManager.Query().In("id", sq.SubQuery())
+	secgroups := []SSecurityGroup{}
+	err := db.FetchModelObjects(SecurityGroupManager, q, &secgroups)
 	if err != nil {
-		return nil, fmt.Errorf("getSecgroupByCache fail %s", err)
+		return nil, errors.Wrapf(err, "db.FetchModelObjects")
 	}
-	if count == 0 {
-		return nil, fmt.Errorf("failed find secgroup cache from provider %s externalId %s", provider.Name, externalId)
-	}
-	if count > 1 {
-		return nil, fmt.Errorf("duplicate secgroup cache for provider %s externalId %s", provider.Name, externalId)
-	}
-	if err := q.First(&cache); err != nil {
-		return nil, err
-	}
-	return cache.GetSecgroup()
+	return secgroups, nil
 }
 
-func (self *SGuest) SyncVMSecgroups(ctx context.Context, userCred mcclient.TokenCredential, provider *SCloudprovider, secgroupIds []string) compare.SyncResult {
-	syncResult := compare.SyncResult{}
-
-	secgroupExternalIds := self.getSecgroupExternalIds(provider)
-
-	_secgroupIds := []string{}
-	for _, secgroupId := range secgroupIds {
-		secgroup, err := self.getSecgroupByCache(provider, secgroupId)
-		if err != nil {
-			syncResult.AddError(err)
-			continue
-		}
-		_secgroupIds = append(_secgroupIds, secgroup.Id)
-		if !utils.IsInStringArray(secgroupId, secgroupExternalIds) {
-			if len(self.SecgrpId) == 0 {
-				_, err := db.Update(self, func() error {
-					self.SecgrpId = secgroup.Id
-					return nil
-				})
-				if err != nil {
-					log.Errorf("update guest secgroup error: %v", err)
-					syncResult.AddError(err)
-				}
-			} else {
-				if _, err := GuestsecgroupManager.newGuestSecgroup(ctx, userCred, self, secgroup); err != nil {
-					log.Errorf("failed to bind secgroup %s for guest %s error: %v", secgroup.Name, self.Name, err)
-					syncResult.AddError(err)
-				}
-			}
-			syncResult.Add()
-		}
+func (self *SGuest) SyncVMSecgroups(ctx context.Context, userCred mcclient.TokenCredential, externalIds []string) error {
+	secgroups, err := self.getSecgroupsBySecgroupExternalIds(externalIds)
+	if err != nil {
+		return errors.Wrap(err, "getSecgroupsBySecgroupExternalIds")
+	}
+	secgroupIds := []string{}
+	for _, secgroup := range secgroups {
+		secgroupIds = append(secgroupIds, secgroup.Id)
 	}
 
-	//移除公有云未关联的安全组
-	secgroups := self.GetSecgroups()
-	for i := 0; i < len(secgroups); i++ {
-		if !utils.IsInStringArray(secgroups[i].Id, _secgroupIds) {
-			err := self.revokeSecgroup(ctx, userCred, &secgroups[i])
-			if err != nil {
-				log.Errorf("revoke secgroup %s(%s) error: %v", secgroups[i].Name, secgroups[i].Id, err)
-				continue
-			}
-			syncResult.Delete()
-		}
-	}
-	return syncResult
+	return self.saveSecgroups(ctx, userCred, secgroupIds)
 }
 
 func (self *SGuest) GetIVM() (cloudprovider.ICloudVM, error) {
@@ -5074,7 +5020,7 @@ func (self *SGuest) ToCreateInput(userCred mcclient.TokenCredential) *api.Server
 	userInput.ProjectId = userCred.GetProjectId()
 	userInput.ProjectDomainId = userCred.GetProjectDomainId()
 	userInput.Secgroups = []string{}
-	secgroups := self.GetSecgroups()
+	secgroups, _ := self.GetSecgroups()
 	for _, secgroup := range secgroups {
 		userInput.Secgroups = append(userInput.Secgroups, secgroup.Id)
 	}

pkg/compute/models/guestsecgroups.go

@@ -16,7 +16,6 @@ package models
 
 import (
 	"context"
-	"fmt"
 
 	"yunion.io/x/jsonutils"
 	"yunion.io/x/log"
@@ -25,7 +24,6 @@ import (
 
 	api "yunion.io/x/onecloud/pkg/apis/compute"
 	"yunion.io/x/onecloud/pkg/cloudcommon/db"
-	"yunion.io/x/onecloud/pkg/cloudcommon/db/lockman"
 	"yunion.io/x/onecloud/pkg/mcclient"
 	"yunion.io/x/onecloud/pkg/util/stringutils2"
 )
@@ -56,7 +54,6 @@ type SGuestsecgroup struct {
 	SGuestJointsBase
 
 	SSecurityGroupResourceBase `width:"36" charset:"ascii" nullable:"false" list:"user" create:"required"`
-	// SecgroupId string `width:"36" charset:"ascii" nullable:"false" list:"user" create:"required"` // Column(VARCHAR(36, charset='ascii'), nullable=False)
 }
 
 func (manager *SGuestsecgroupManager) GetSlaveFieldName() string {
@@ -74,63 +71,6 @@ func (self *SGuestsecgroup) getSecgroup() *SSecurityGroup {
 	return secgroup
 }
 
-func (manager *SGuestsecgroupManager) newGuestSecgroup(ctx context.Context, userCred mcclient.TokenCredential, guest *SGuest, secgroup *SSecurityGroup) (*SGuestsecgroup, error) {
-	q := manager.Query()
-	q = q.Equals("guest_id", guest.Id).Equals("secgroup_id", secgroup.Id)
-	count, err := q.CountWithError()
-	if err != nil {
-		return nil, err
-	}
-	if count > 0 {
-		return nil, fmt.Errorf("security group %s has already been assigned to guest %s", secgroup.Name, guest.Name)
-	}
-
-	gs := SGuestsecgroup{}
-	gs.SecgroupId = secgroup.Id
-	gs.SetModelManager(manager, &gs)
-	gs.GuestId = guest.Id
-
-	lockman.LockObject(ctx, secgroup)
-	defer lockman.ReleaseObject(ctx, secgroup)
-
-	return &gs, manager.TableSpec().Insert(ctx, &gs)
-}
-
-func (manager *SGuestsecgroupManager) GetGuestSecgroups(guest *SGuest, secgroup *SSecurityGroup) ([]SGuestsecgroup, error) {
-	guestsecgroups := []SGuestsecgroup{}
-	q := manager.Query()
-	if guest != nil {
-		q = q.Equals("guest_id", guest.Id)
-	}
-	if secgroup != nil {
-		q = q.Equals("secgroup_id", secgroup.Id)
-	}
-	if err := db.FetchModelObjects(manager, q, &guestsecgroups); err != nil {
-		return nil, err
-	}
-	return guestsecgroups, nil
-}
-
-func (manager *SGuestsecgroupManager) DeleteGuestSecgroup(ctx context.Context, userCred mcclient.TokenCredential, guest *SGuest, secgroup *SSecurityGroup) error {
-	gss := []SGuestsecgroup{}
-	q := manager.Query()
-	if guest != nil {
-		q = q.Equals("guest_id", guest.Id)
-	}
-	if secgroup != nil {
-		q = q.Equals("secgroup_id", secgroup.Id)
-	}
-	if err := db.FetchModelObjects(manager, q, &gss); err != nil {
-		return err
-	}
-	for _, gs := range gss {
-		if err := gs.Delete(ctx, userCred); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 func (self *SGuestsecgroup) Delete(ctx context.Context, userCred mcclient.TokenCredential) error {
 	return db.DeleteModel(ctx, userCred, self)
 }

pkg/compute/models/instance_snapshots.go

@@ -274,7 +274,7 @@ func (manager *SInstanceSnapshotManager) CreateInstanceSnapshot(
 	if osVersion := guest.GetMetadata("os_version", nil); len(osVersion) > 0 {
 		serverMetadata.Set("os_version", jsonutils.NewString(osVersion))
 	}
-	secs := guest.GetSecgroups()
+	secs, _ := guest.GetSecgroups()
 	if len(secs) > 0 {
 		secIds := make([]string, len(secs))
 		for i := 0; i < len(secs); i++ {

pkg/compute/models/secgroups.go

@@ -690,18 +690,20 @@ func (self *SSecurityGroup) AllowPerformMerge(ctx context.Context, userCred mccl
 	return self.IsOwner(userCred) || db.IsAdminAllowPerform(userCred, self, "merge")
 }
 
-func (self *SSecurityGroup) PerformMerge(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) (jsonutils.JSONObject, error) {
-	secgroupIds := jsonutils.GetQueryStringArray(data, "secgroups")
-	if len(secgroupIds) == 0 {
-		return nil, httperrors.NewMissingParameterError("secgroups")
+func (self *SSecurityGroup) PerformMerge(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, input api.SecgroupMergeInput) (jsonutils.JSONObject, error) {
+	if len(input.SecgroupIds) == 0 {
+		return nil, httperrors.NewMissingParameterError("secgroup_ids")
 	}
 	inAllowList := self.GetInAllowList()
 	outAllowList := self.GetOutAllowList()
 	secgroups := []*SSecurityGroup{}
-	for _, secgroupId := range secgroupIds {
+	for _, secgroupId := range input.SecgroupIds {
 		_secgroup, err := SecurityGroupManager.FetchByIdOrName(userCred, secgroupId)
 		if err != nil {
-			return nil, httperrors.NewResourceNotFoundError("failed to find secgroup %s error: %v", secgroupId, err)
+			if errors.Cause(err) == sql.ErrNoRows {
+				return nil, httperrors.NewResourceNotFoundError2("secgroup", secgroupId)
+			}
+			return nil, httperrors.NewGeneralError(err)
 		}
 		secgroup := _secgroup.(*SSecurityGroup)
 		secgroup.SetModelManager(SecurityGroupManager, secgroup)
@@ -718,12 +720,14 @@ func (self *SSecurityGroup) PerformMerge(ctx context.Context, userCred mcclient.
 
 	for i := 0; i < len(secgroups); i++ {
 		secgroup := secgroups[i]
-		if err := self.migrateSecurityGroupCache(secgroup); err != nil {
-			return nil, err
+		err := self.mergeSecurityGroupCache(secgroup)
+		if err != nil {
+			return nil, httperrors.NewGeneralError(errors.Wrapf(err, "mergeSecurityGroupCache"))
 		}
 
-		if err := self.migrateGuestSecurityGroup(secgroup); err != nil {
-			return nil, err
+		err = self.mergeGuestSecurityGroup(ctx, userCred, secgroup)
+		if err != nil {
+			return nil, httperrors.NewGeneralError(errors.Wrapf(err, "mergeGuestSecurityGroup"))
 		}
 		secgroup.RealDelete(ctx, userCred)
 	}
@@ -754,12 +758,10 @@ func (self *SSecurityGroup) getSecurityGroupRuleSet() secrules.SecurityGroupRule
 	return srs
 }
 
-func (self *SSecurityGroup) migrateSecurityGroupCache(secgroup *SSecurityGroup) error {
-	caches := []SSecurityGroupCache{}
-	q := SecurityGroupCacheManager.Query().Equals("secgroup_id", secgroup.Id)
-	err := db.FetchModelObjects(SecurityGroupCacheManager, q, &caches)
+func (self *SSecurityGroup) mergeSecurityGroupCache(secgroup *SSecurityGroup) error {
+	caches, err := secgroup.GetSecurityGroupCaches()
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "GetSecurityGroupCaches")
 	}
 	for i := 0; i < len(caches); i++ {
 		cache := caches[i]
@@ -768,46 +770,37 @@ func (self *SSecurityGroup) migrateSecurityGroupCache(secgroup *SSecurityGroup)
 			return nil
 		})
 		if err != nil {
-			return err
+			return errors.Wrap(err, "db.Update")
 		}
 	}
 	return nil
 }
 
-func (self *SSecurityGroup) migrateGuestSecurityGroup(secgroup *SSecurityGroup) error {
-	guests := secgroup.GetGuests()
+func (self *SSecurityGroup) mergeGuestSecurityGroup(ctx context.Context, userCred mcclient.TokenCredential, fade *SSecurityGroup) error {
+	guests := fade.GetGuests()
 	for i := 0; i < len(guests); i++ {
-		guest := guests[i]
-		_, err := db.Update(&guest, func() error {
-			if guest.SecgrpId == secgroup.Id {
-				guest.SecgrpId = self.Id
-			}
-			if guest.AdminSecgrpId == secgroup.Id {
-				guest.AdminSecgrpId = self.Id
-			}
-			return nil
-		})
+		secgroups, err := guests[i].GetSecgroups()
 		if err != nil {
-			return err
+			return errors.Wrapf(err, "GetSecgroups for guest %s(%s)", guests[i].Name, guests[i].Id)
+		}
+		secgroupIds := []string{}
+		for i := range secgroups {
+			if secgroups[i].Id == fade.Id {
+				continue
+			}
+			if utils.IsInStringArray(secgroups[i].Id, secgroupIds) {
+				continue
+			}
+			secgroupIds = append(secgroupIds, secgroups[i].Id)
+		}
+		if !utils.IsInStringArray(self.Id, secgroupIds) {
+			secgroupIds = append(secgroupIds, self.Id)
+		}
+		err = guests[i].saveSecgroups(ctx, userCred, secgroupIds)
+		if err != nil {
+			return errors.Wrap(err, "saveSecgroups")
 		}
 	}
-
-	guestsecgroups, err := GuestsecgroupManager.GetGuestSecgroups(nil, secgroup)
-	if err != nil {
-		return err
-	}
-
-	for i := 0; i < len(guestsecgroups); i++ {
-		guestsecgroup := guestsecgroups[i]
-		_, err := db.Update(&guestsecgroup, func() error {
-			guestsecgroup.SecgroupId = self.Id
-			return nil
-		})
-		if err != nil {
-			return err
-		}
-	}
-
 	return nil
 }
 
@@ -1024,14 +1017,15 @@ func (self *SSecurityGroup) ValidateDeleteCondition(ctx context.Context) error {
 	return self.SSharableVirtualResourceBase.ValidateDeleteCondition(ctx)
 }
 
-func (self *SSecurityGroup) GetSecurityGroupCaches() []SSecurityGroupCache {
+func (self *SSecurityGroup) GetSecurityGroupCaches() ([]SSecurityGroupCache, error) {
 	caches := []SSecurityGroupCache{}
 	q := SecurityGroupCacheManager.Query()
 	q = q.Filter(sqlchemy.Equals(q.Field("secgroup_id"), self.Id))
-	if err := db.FetchModelObjects(SecurityGroupCacheManager, q, &caches); err != nil {
-		log.Errorf("get secgroupcache for secgroup %s error: %v", self.Name, err)
+	err := db.FetchModelObjects(SecurityGroupCacheManager, q, &caches)
+	if err != nil {
+		return nil, errors.Wrapf(err, "db.FetchModelObjects")
 	}
-	return caches
+	return caches, nil
 }
 
 func (self *SSecurityGroup) CustomizeDelete(ctx context.Context, userCred mcclient.TokenCredential, query jsonutils.JSONObject, data jsonutils.JSONObject) error {

pkg/compute/tasks/security_group_delete_task.go

@@ -51,7 +51,12 @@ func (self *SecurityGroupDeleteTask) OnInit(ctx context.Context, obj db.IStandal
 
 func (self *SecurityGroupDeleteTask) OnSecurityGroupUncacheComplete(ctx context.Context, obj db.IStandaloneModel, data jsonutils.JSONObject) {
 	secgroup := obj.(*models.SSecurityGroup)
-	secgroupCaches := secgroup.GetSecurityGroupCaches()
+	secgroupCaches, err := secgroup.GetSecurityGroupCaches()
+	if err != nil {
+		secgroup.SetStatus(self.UserCred, api.SECGROUP_STATUS_READY, "")
+		self.SetStageFailed(ctx, jsonutils.Marshal(err))
+		return
+	}
 	errCount := self.getErrorCount()
 	if len(secgroupCaches) == int(errCount) {
 		if errCount == 0 {

pkg/mcclient/options/servers.go

@@ -555,8 +555,12 @@ type ServerSecGroupOptions struct {
 }
 
 type ServerSecGroupsOptions struct {
-	ID     string   `help:"ID or Name of server" metavar:"Guest" json:"-"`
-	Secgrp []string `help:"Ids of Security Groups" metavar:"Security Groups" positional:"true"`
+	ID          string   `help:"ID or Name of server" metavar:"Guest" json:"-"`
+	SecgroupIds []string `help:"Ids of Security Groups" metavar:"Security Groups" positional:"true"`
+}
+
+func (opts *ServerSecGroupsOptions) Parmas() jsonutils.JSONObject {
+	return jsonutils.Marshal(map[string][]string{"secgroup_ids": opts.SecgroupIds})
 }
 
 type ServerModifySrcCheckOptions struct {