many features and improvements #22

aajkl · 2024-07-01T05:32:10Z

support ceph rbd
support ovn
support cloud-init
merge eru agent
many other improvements and bugfix

1. support ceph rbd 2. support ovn 3. support cloud-init 4. merge eru agent 5. many other improvements and bugfix

anrs · 2024-07-01T06:01:23Z

configs/codec.go

@@ -5,13 +5,13 @@ import (

 	"github.com/BurntSushi/toml"

-	"github.com/projecteru2/yavirt/pkg/errors"


actually, why not wraps the cockroachdb/errors within yavirt/pkg/errors, then don't need to change anything from upper pkgs.

Using upstream package directly is enough and adding a thin wrapper is needless. actually I just want to unify the log and errors pkg of core,yavirt and some components developed by me.

anrs · 2024-07-01T06:03:17Z

internal/eru/agent/agent.go

+		labels[parts[0]] = parts[1]
+	}
+	go func() {
+		// Core need to connect to the local grpc server, so sleep 30s here to wait local grpc server up


sending a ping packet to be more graceful

ping what? local machine? sleeping 30s is enough here, it is more simple and works well

anrs · 2024-07-01T06:05:13Z

internal/eru/agent/agent.go

+				logger.Error(ctx, err, "failed to add node")
+				return
+			}
+			if e.Code() == 1031 && strings.Contains(e.Message(), "node already exists") {


defines a var for (hardcoded) 1031 should be a good idea

okay, I'll fix this

these code numbers are from core, I just don't want to import too many dependencies, but I just read code and it's ok to import core here.

anrs · 2024-07-01T06:08:31Z

internal/eru/agent/guest.go

+		}
+	}
+
+	f1 := utils.CheckHTTP(ctx, g.ID, httpChecker, healthCheck.HTTPCode, timeout)


how about shortcut here for saving cpu/bandwidth

this is specified by user and when checker list is empty, checkHTTP do nothing

anrs · 2024-07-01T06:10:39Z

internal/eru/agent/metrics.go

+func (e *MetricsCollector) Collect(ch chan<- prometheus.Metric) {
+	for _, v := range e.wrkStatusCache.Items() {
+		wrkStatus, _ := v.Object.(*types.WorkloadStatus)
+		if !wrkStatus.Running {


prefer to check wrkStatus if is nil in the first

I'll add a check here, actually wrkStatus never be nil here

anrs · 2024-07-01T06:14:36Z

internal/eru/agent/node.go

+		logger.Warn(ctx, "service is not healthy")
+		return
+	}
+	if err := m.store.CheckHealth(ctx); err != nil {


how about

m.mCol.coreHealthy.Store(m.store.CheckHealth(ctx) == nil)

for removing the if/else

here I'll add a log message

anrs · 2024-07-01T06:16:59Z

internal/eru/agent/workload.go

+	tick := time.NewTicker(time.Duration(m.config.HealthCheck.Interval) * time.Second)
+	defer tick.Stop()
+
+	_ = utils.Pool.Submit(func() { m.checkAllWorkloads(ctx) })


can remove _ = part from the statement

removing _= will cause a lint issue

anrs · 2024-07-01T06:17:28Z

internal/eru/agent/workload.go

+	for {
+		select {
+		case <-tick.C:
+			_ = utils.Pool.Submit(func() { m.checkAllWorkloads(ctx) })


also can remove the _ = part

same as above

anrs · 2024-07-01T06:19:52Z

internal/eru/agent/workload.go

+	}
+
+	for _, workloadID := range workloadIDs {
+		ID := workloadID


we can add comment to explain why we define a new var here.
(im not sure if is fixed the bug in golang higher version, if so, we can remove the new var)

this is copied from agent, https://github.com/projecteru2/agent/blob/master/manager/workload/health_check.go#L41 I also don't know why defining a new var, I'll delete this

can't be deleted because of https://go.dev/blog/loopvar-preview

I see, just use for idx := range workloadIDs and define a new var in loop, so it's not that strange.

anrs · 2024-07-01T06:20:24Z

internal/eru/agent/workload.go

+
+	for _, workloadID := range workloadIDs {
+		ID := workloadID
+		_ = utils.Pool.Submit(func() { m.checkOneWorkload(ctx, ID) })


dont need to _ = part

anrs · 2024-07-01T06:21:37Z

internal/eru/agent/workload.go

+
+	m.wrkStatusCache.Set(workloadStatus.ID, workloadStatus, 0)
+
+	if err = m.setWorkloadStatus(ctx, workloadStatus); err != nil {


prefer to a brand-new err := here

okay, this is also from agent

anrs · 2024-07-01T06:23:23Z

internal/eru/recycle/recycle.go

+	"github.com/projecteru2/yavirt/internal/utils"
+	"github.com/projecteru2/yavirt/internal/utils/notify/bison"
+	"github.com/samber/lo"
+	"google.golang.org/grpc/status"


prefer to group/separate pkgs from 3rd and yavirt

anrs · 2024-07-01T06:25:01Z

internal/eru/recycle/recycle.go

+		log.Debugf(ctx, "[recycle] notifier: %v", notifier)
+		if notifier != nil {
+			msgList := []string{
+				"<font color=#00CC33 size=10>delete dangling successfully </font>",


how about defining a template snippet here?

anrs · 2024-07-01T06:26:21Z

internal/eru/recycle/recycle.go

+
+		coreIDs, err := fetchWorkloads()
+		if err != nil {
+			continue


do we need to log the err but not found kind

anrs · 2024-07-01T06:27:26Z

internal/eru/recycle/recycle.go

+			continue
+		}
+		coreMap := map[string]struct{}{}
+		for _, id := range coreIDs {


can we utilize the lo.Map

lo.Map returns slice and here is a map and i think for-loop is more intuitive here

anrs · 2024-07-01T06:28:53Z

internal/eru/recycle/recycle.go

+	}
+}
+
+func Setup(ctx context.Context, cfg *configs.Config, t *testing.T) (err error) {


prefer to call this Setup and pass the store instance from unit test files

anrs · 2024-07-01T06:30:27Z

internal/eru/resources/core.go

+	defer cm.mu.Unlock()
+	for resName, rawParams := range capacity {
+		switch resName {
+		case "cpumem":


strongly rec constants rather than literal in the switch/case

anrs · 2024-07-01T06:33:52Z

internal/eru/resources/core.go

+	if ans != nil {
+		return
+	}
+	cm.fetchResources()


rename to fetchResourcesWithLock or FetchResources (with lock implicitly)
i'd like to the former and

func (cm *CoreResourcesManager) GetCpumem() (ans *cpumemtypes.NodeResource) { cm.mu.Lock() defer cm.mu.Unlock() ... }

only once

anrs · 2024-07-01T10:53:37Z

internal/eru/resources/gpu.go

+	if err := json.Unmarshal([]byte(gpuNameJSON), &gpuNameMap); err != nil {
+		return err
+	}
+	// merge name map from config to gpuNameMap


lo.Map would help?

I think for-loop is very intuitive here, using functional style code here don't have any advantages.

anrs · 2024-07-01T11:36:14Z

internal/types/cloud_init.go

+	DefaultGW CloudInitGateway `json:"-"`
+}
+
+func initTpls() (err error) {


how about func init() then we can remove all the if xxx == nil from the func

or sync.Once(initTpls)

I don't like init, I always run initialization code explicitly, sync.Once seems good

anrs · 2024-07-02T06:43:36Z

@aajkl can be merged after lint fixing

aajkl · 2024-07-02T06:49:40Z

@anrs done

* many features and improvements 1. support ceph rbd 2. support ovn 3. support cloud-init 4. merge eru agent 5. many other improvements and bugfix * fix lint and ut issues * fix review issues * don't return when check imageHub failed * remove fmt.Printf

aajkl added 2 commits July 1, 2024 13:28

many features and improvements

d90ab38

1. support ceph rbd 2. support ovn 3. support cloud-init 4. merge eru agent 5. many other improvements and bugfix

fix lint and ut issues

b7ec82a

aajkl force-pushed the master branch from 502588a to b7ec82a Compare July 1, 2024 05:50

anrs reviewed Jul 1, 2024

View reviewed changes

aajkl force-pushed the master branch 2 times, most recently from fc7746f to 28a6c2f Compare July 1, 2024 12:38

fix review issues

d74914e

aajkl force-pushed the master branch from 28a6c2f to d74914e Compare July 2, 2024 03:35

don't return when check imageHub failed

3f57978

remove fmt.Printf

1f5c676

aajkl force-pushed the master branch from b6bcc28 to 1f5c676 Compare July 2, 2024 06:45

CMGS merged commit 377de40 into projecteru2:master Jul 2, 2024
2 checks passed

		@@ -5,13 +5,13 @@ import (

		"github.com/BurntSushi/toml"

		"github.com/projecteru2/yavirt/pkg/errors"


		m.wrkStatusCache.Set(workloadStatus.ID, workloadStatus, 0)

		if err = m.setWorkloadStatus(ctx, workloadStatus); err != nil {

many features and improvements #22

many features and improvements #22

Conversation

aajkl commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anrs Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anrs Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anrs commented Jul 2, 2024

aajkl commented Jul 2, 2024

anrs Jul 1, 2024 •

edited

Loading

anrs Jul 1, 2024 •

edited

Loading