Implement deregistration functionality for resources and VMs by leehyeoklee · Pull Request #2286 · cloud-barista/cb-tumblebug

leehyeoklee · 2026-01-23T04:46:15Z

Summary

This PR introduces resource deregistration functionality across major resources and significantly improves the validation logic for subnet deletion to prevent accidental data inconsistency.

Key Changes

1. Feature: Resource Deregistration

Implemented Deregister operations for the following resources. This action removes the resource metadata from Spider and Tumblebug while preserving the actual resource on the CSP.

Supported Resources:
- SSH Keys, Security Groups, Data Disks, Custom Images (New Endpoints)
- VNets, Subnets (Logic implemented on existing endpoints)
Implementation Details:
- Utilizes Spider's deregister APIs (e.g., /regkeypair/{Name}, /regvpc/{Name}) instead of delete APIs.
- Dependency Cleanup: Invokes ClearDependencyFromVMs to remove references to the deregistered resource from associated VM fields.
API Endpoints:
- DELETE /ns/{nsId}/deregisterCspResource/sshKey/{sshKeyId} (New)
- DELETE /ns/{nsId}/deregisterCspResource/securityGroup/{securityGroupId} (New)
- DELETE /ns/{nsId}/deregisterCspResource/dataDisk/{dataDiskId} (New)
- DELETE /ns/{nsId}/deregisterCspResource/customImage/{customImageId} (New)
- (Note: Deregistration endpoints for VNet and Subnet already existed; logic has been updated.)

2. Fix: Subnet Deletion/Deregistration Validation

Problem:
Previously, the code checked subnetInfo.Status == NetworkInUse to prevent deleting in-use subnets. However, this status field was never properly set, allowing subnets attached to active VMs to be deleted or deregistered.

	// Todo: Check if the subnet is being used by any resouces, such as virtual machines, gateways, etc.
	// Check if the vNet has subnets or not
	if action == ActionNone && subnetInfo.Status == string(NetworkInUse) {
		err := fmt.Errorf("the subnet (%s) is in-use, may have any resources", subnetId)
		log.Error().Err(err).Msg("")
		return emptyRet, err
	}

Solution:

Implemented CheckSubnetInUseByVMs: A new function that scans all VMs in the namespace to verify if the target subnetId is in use.
Applied this validation to both DelSubnet and DeregisterSubnet.
The API now returns a clear error message listing the VMs currently using the subnet.

Todo:

Validation currently checks usage by VMs only. Future updates are needed to check for other resources.

seokho-son

Hi @leehyeoklee
I have some opinions for this PR. PTAL. :)

seokho-son · 2026-01-23T06:34:26Z

src/core/infra/manageInfo.go

+	err = common.CheckString(vmId)
+	if err != nil {
+		log.Error().Err(err).Msg("")
+		return err
+	}
+	check, _ := CheckVm(nsId, mciId, vmId)
+
+	if !check {
+		err := fmt.Errorf("The vm " + vmId + " does not exist.")
+		return err
+	}
+
+	log.Debug().Msg("[Deregister VM] " + vmId)
+
+	// get vm info
+	vmInfo, _ := GetVmObject(nsId, mciId, vmId)


GetVmObject will validate existence of the VM object too.
So, CheckVm related code can be moved.

and, vmInfo, _ := GetVmObject(nsId, mciId, vmId) needs err handling if we remove CheckVm here.

seokho-son · 2026-01-23T06:36:14Z

src/core/infra/manageInfo.go

+	}
+	log.Debug().Msg("Deregister request finished from " + url)
+
+	// delete vms info from TB


Suggested change

// delete vms info from TB

// delete the VM info from TB

seokho-son · 2026-01-23T06:42:32Z

src/core/infra/manageInfo.go

+				return err
+			}
+		}
+	}


I think the logic of this section is complex.

We can refine it more direct way. I guess.

identify the (one) subnet object to be removed before removing vm object.

remove identified subnet just after vm object has been removed.

seokho-son · 2026-01-23T07:34:49Z

src/core/infra/manageInfo.go

+	// Update associated object lists
+	_, err = resource.UpdateAssociatedObjectList(nsId, model.StrImage, vmInfo.ImageId, model.StrDelete, key)
+	if err != nil {
+		resource.UpdateAssociatedObjectList(nsId, model.StrCustomImage, vmInfo.ImageId, model.StrDelete, key)


why do we have additional trial only for image type ? intentional?

I followed the delete method pattern but excluded associatedObjectList as it's not applicable to Images😓

seokho-son · 2026-01-23T07:36:57Z

src/core/resource/common.go

+	err := common.CheckString(nsId)
+	if err != nil {
+		log.Error().Err(err).Msg("")
+		return err
+	}
+
+	err = common.CheckString(resourceId)
+	if err != nil {
+		log.Error().Err(err).Msg("")
+		return err
+	}
+	check, err := CheckResource(nsId, resourceType, resourceId)
+
+	if err != nil {
+		log.Error().Err(err).Msg("")
+		return err
+	}
+
+	if !check {
+		errString := "The " + resourceType + " " + resourceId + " does not exist."
+		err := fmt.Errorf(errString)
+		return err
+	}


Let's reduce duplicated validations. :)

seokho-son · 2026-01-23T07:41:38Z

src/core/resource/common.go

+		log.Debug().Msg("Image deregistered successfully from database")
+		return nil
+
+	case model.StrCustomImage:


let's find an existing function for this operation.

I replaced the direct ORM query with the GetResource(nsId, model.StrCustomImage, resourceId) funtion!

seokho-son · 2026-01-23T07:42:15Z

src/core/resource/common.go

+		url = model.SpiderRestUrl + "/regmyimage/" + temp.CspImageName
+		uid = temp.Uid
+
+	case model.StrSpec:


no need to remove Specs in general. too

seokho-son · 2026-01-23T07:48:54Z

src/core/resource/common.go

+		mciId, vmId, err := parseVMKeyPath(vmKey)
+		if err != nil {
+			log.Warn().Err(err).Str("vmKey", vmKey).Msg("Failed to parse VM key")
+			continue
+		}
+
+		// Get VM object from kvstore
+		vmStoreKey := common.GenMciKey(nsId, mciId, vmId)
+		vmKeyValue, exists, err := kvstore.GetKv(vmStoreKey)
+		if err != nil || !exists {
+			log.Warn().Msgf("Failed to get VM %s to update fields", vmId)
+			continue
+		}


We need to avoid writing codes directly controlling kvstore and the other DBs. I think we have existing functions for this.

seokho-son · 2026-01-23T07:52:31Z

src/core/resource/common.go

+	log.Debug().Msg("Deregister request finished from " + url)
+
+	// Get associated VMs and update their fields to remove references to this resource
+	err = ClearDependencyFromVMs(nsId, resourceType, resourceId)


should we clear Clear Dependency From VMs?
or should we block it?

I updated the logic to prevent VM deregistration if any associated resources (SSH Key, Security Group, Data Disk) still exist. The system now checks for these dependencies and blocks the operation if any are found.

seokho-son · 2026-01-23T07:53:41Z

src/core/resource/common.go

+
+// parseVMKeyPath parses a VM key path and extracts mciId and vmId
+// Expected format: /ns/{nsId}/mci/{mciId}/vm/{vmId}
+func parseVMKeyPath(vmKey string) (mciId string, vmId string, err error) {


we should avoid this pattern.

I added the sys.securityGroup label to the VM creation/registration logic. This allowed me to remove the previous check by using a label selector to verify VM existence in the subnet instead

…ve-registration

…ment

…s before removal

…NetId

seokho-son

LGTM! @leehyeoklee thanks!

seokho-son · 2026-01-26T14:58:40Z

/approve

leehyeoklee added 2 commits January 23, 2026 13:36

Implement deregistration functionality for resources and VMs

4b3803e

Add deregistration endpoints and update swagger docs

bf50ec9

leehyeoklee requested review from seokho-son and yunkon-kim as code owners January 23, 2026 04:46

Correct field in VM info structures

41b810f

seokho-son reviewed Jan 23, 2026

View reviewed changes

leehyeoklee added 4 commits January 26, 2026 17:13

Merge remote-tracking branch 'origin/improve-registration' into impro…

08d133c

…ve-registration

Add subnetId label and improve subnet usage checks in resource manage…

9b6f940

…ment

Enhance VM deregistration process by checking for associated resource…

17c04c6

…s before removal

Remove redundant error message and clean up unused route

fa68101

github-actions bot added the src label Jan 26, 2026

leehyeoklee added 2 commits January 26, 2026 17:59

Merge branch 'main' into improve-registration

fb77ed7

Add VNetId label to VM creation and enhance subnet usage check with V…

7972de0

…NetId

seokho-son approved these changes Jan 26, 2026

View reviewed changes

github-actions bot approved these changes Jan 26, 2026

View reviewed changes

github-actions bot added the approved This PR is approved and will be merged soon. label Jan 26, 2026

cb-github-robot merged commit 9286e8a into cloud-barista:main Jan 26, 2026
4 checks passed

Conversation

leehyeoklee commented Jan 23, 2026

Summary

Key Changes

1. Feature: Resource Deregistration

2. Fix: Subnet Deletion/Deregistration Validation

Uh oh!

seokho-son left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seokho-son left a comment

Choose a reason for hiding this comment

Uh oh!

seokho-son commented Jan 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants