Thinpool gRPC Call And Monitor by Zllinc · Pull Request #58 · lingdie/sealos

Zllinc · 2025-09-04T02:50:53Z

No description provided.

dinoallo · 2025-09-04T04:00:47Z

 type NodeStatsProvider interface {
 	ContainerFsStats(ctx context.Context) (FsStats, error)
+	ThinPoolMetrics() ([]*ThinPoolMetrics, error)
+	ExportToVictoriaMetrics([]*ThinPoolMetrics) error


Metrics exporting should be done in lvm thin pool monitor?

dinoallo

Currently, the implementation of NodeStatsProviderImpl is coupled with LVM. Please make devbox controller as storage agnostic as possible, i.e. remove any logic related to LVM. Instead, you should implement NodeStatsProviderImpl in LVM monitor. NodeStats should only contain the information required for scheduling pods.

dinoallo · 2025-09-04T04:11:40Z

Also, the linting check has failed.

Copilot

Pull Request Overview

This PR implements "Thinpool gRPC Call And Monitor" functionality for the devbox controller, transitioning from v1alpha1 to v1alpha2 API version and introducing comprehensive LVM thin pool monitoring with gRPC communication and VictoriaMetrics integration.

Key changes include:

Complete API version migration from v1alpha1 to v1alpha2 with new devbox state management and commit recording system
Implementation of LVM monitoring service with gRPC server/client architecture for thin pool metrics collection
Addition of VictoriaMetrics integration for metrics export and monitoring dashboard support

Reviewed Changes

Copilot reviewed 54 out of 57 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
service/go.work.sum	Added dependencies for Alipay SDK and cenkalti/backoff package updates
controllers/devbox/internal/stat/	New LVM monitoring system with gRPC services, clients, and VictoriaMetrics integration
controllers/devbox/internal/controller/	Major refactor to v1alpha2 API with new state management, scheduling logic, and registry utilities
controllers/devbox/internal/commit/	New commit system implementation with containerd integration for image management
controllers/devbox/api/v1alpha2/	New API version with updated devbox resource definitions and state management
controllers/devbox/config/	Updated RBAC, samples, and deployment manifests for v1alpha2

Copilot · 2025-09-05T08:28:20Z

+	fmt.Printf("=== Thin Pool Metrics Test Results ===\n")
+	fmt.Printf("Total thin pools found: %d\n", len(metrics))


The test functions TestCollectThinPoolMetrics and TestExportToVictoriaMetrics contain duplicated code for printing test results. Consider extracting this into a helper function to reduce code duplication.

Copilot · 2025-09-05T08:28:20Z

+	fmt.Printf("=== Thin Pool Metrics Test Results ===\n")
+	fmt.Printf("Total thin pools found: %d\n", len(metrics))


The test functions TestCollectThinPoolMetrics and TestExportToVictoriaMetrics contain duplicated code for printing test results. Consider extracting this into a helper function to reduce code duplication.

Copilot · 2025-09-05T08:28:20Z

+	// todo : delete data and save length
+	logger.Info("[exportToVictoriaMetrics] Sending %d metrics to VM:\n%s\n", len(allPrometheusData), data)


The TODO comment indicates that logging the full data should be removed and only the length should be saved. This could expose sensitive information in logs and should be addressed.

Suggested change

// todo : delete data and save length

logger.Info("[exportToVictoriaMetrics] Sending %d metrics to VM:\n%s\n", len(allPrometheusData), data)

// Only log the count and size of metrics, not the full data to avoid exposing sensitive information

logger.Info("[exportToVictoriaMetrics] Sending %d metrics to VM (total size: %d bytes)", len(allPrometheusData), len(data))

Copilot · 2025-09-05T08:28:21Z

+}
+
+func RegisterLVMMonitorServiceServer(s grpc.ServiceRegistrar, srv LVMMonitorServiceServer) {
+	// If the following call pancis, it indicates UnimplementedLVMMonitorServiceServer was


There's a typo in the comment: 'pancis' should be 'panics'.

Suggested change

// If the following call pancis, it indicates UnimplementedLVMMonitorServiceServer was

// If the following call panics, it indicates UnimplementedLVMMonitorServiceServer was

Copilot · 2025-09-05T08:28:21Z

+	// test:
+	for _, metric := range metrics {
+		fmt.Printf("Client metric: %+v\n", *metric)


Debug code with fmt.Printf statements should be removed from production code or replaced with proper logging.

Suggested change

// test:

for _, metric := range metrics {

fmt.Printf("Client metric: %+v\n", *metric)

logger.Debug("Client metric: %+v", *metric)

Copilot · 2025-09-05T08:28:21Z

+				// TODO: check if this could lead to overflow
+				totalCPULimit += cpuLimit.Value()


Multiple TODO comments indicate potential overflow issues in resource calculation. These should be addressed by implementing proper overflow checks or using appropriate data types.

Copilot · 2025-09-05T08:28:22Z

+				// TODO: check if this could lead to overflow
+				totalMemoryRequest += memReq.Value()


Multiple TODO comments indicate potential overflow issues in resource calculation. These should be addressed by implementing proper overflow checks or using appropriate data types.

Copilot · 2025-09-05T08:28:22Z

+				// TODO: check if this could lead to overflow
+				totalMemoryLimit += memLimit.Value()


Multiple TODO comments indicate potential overflow issues in resource calculation. These should be addressed by implementing proper overflow checks or using appropriate data types.

Copilot · 2025-09-05T08:28:22Z

+	if username == "" && secret == "" {
+		hostOptions.Credentials = nil
+	} else {
+		// TODO: fix this, use flags or configs to set mulit registry credentials


There's a typo in the comment: 'mulit' should be 'multi'.

Suggested change

// TODO: fix this, use flags or configs to set mulit registry credentials

// TODO: fix this, use flags or configs to set multi registry credentials

Copilot · 2025-09-05T08:28:22Z

+		}
+	}
+
+	// todo: implement the schedule logic to replace the current logic


This TODO comment indicates incomplete scheduling logic implementation. The comment should be more specific about what needs to be implemented or removed if the logic is complete.

Suggested change

// todo: implement the schedule logic to replace the current logic

github-actions · 2025-09-08T02:53:44Z

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Thinpool gRPC Call And Monitor". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat
 - fix
 - docs
 - style
 - refactor
 - perf
 - test
 - build
 - ci
 - chore
 - revert

github-actions Bot added area/cloud area/test labels Sep 4, 2025

Zllinc force-pushed the thinpool branch 3 times, most recently from 3fff410 to 96dc7cd Compare September 4, 2025 03:36

dinoallo reviewed Sep 4, 2025

View reviewed changes

dinoallo requested changes Sep 4, 2025

View reviewed changes

dinoallo force-pushed the devbox-daemon-v2 branch from 63a284a to be6c2a1 Compare September 5, 2025 08:15

dinoallo requested a review from Copilot September 5, 2025 08:24

Copilot AI reviewed Sep 5, 2025

View reviewed changes

Cunzili and others added 4 commits September 8, 2025 10:50

add thinpool monitor

7c7d01d

implement ContainerFSStats method and add grpc call

5318560

thin pool grpc

6e3afec

go.sum file

e36e98e

Zllinc force-pushed the thinpool branch from 96dc7cd to e36e98e Compare September 8, 2025 02:53

		fmt.Printf("=== Thin Pool Metrics Test Results ===\n")
		fmt.Printf("Total thin pools found: %d\n", len(metrics))

		// todo : delete data and save length
		logger.Info("[exportToVictoriaMetrics] Sending %d metrics to VM:\n%s\n", len(allPrometheusData), data)

	// If the following call pancis, it indicates UnimplementedLVMMonitorServiceServer was
	// If the following call panics, it indicates UnimplementedLVMMonitorServiceServer was

		// TODO: check if this could lead to overflow
		totalCPULimit += cpuLimit.Value()

		// TODO: check if this could lead to overflow
		totalMemoryRequest += memReq.Value()

		// TODO: check if this could lead to overflow
		totalMemoryLimit += memLimit.Value()

	// TODO: fix this, use flags or configs to set mulit registry credentials
	// TODO: fix this, use flags or configs to set multi registry credentials

Conversation

Zllinc commented Sep 4, 2025

Uh oh!

dinoallo Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

dinoallo left a comment

Choose a reason for hiding this comment

Uh oh!

dinoallo commented Sep 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants