[SPVR-67] add "yavirt" as a new runtime #73

DuodenumL · 2021-09-14T08:35:55Z

除了Yavirt相关，本次PR也修了一些历史遗留bug和这次重构新产生的bug：

优化：Runtime daemon退出时会导致agent整体退出的问题
修复：Bufpipe在特殊情况下Read会无限阻塞的问题
修复：logs/Writer内的重试机制可能会导致goroutine数量爆炸的问题
修复：容器无限重启时，backoff retry可能会导致goroutine数量爆炸的问题
修复：容器无限重启时，logBroadcaster可能会阻塞的问题

突然发现我之前getFilter的用法有点问题。

在Docker里，filter应该是这样的格式：{Key: "label", Value: "ERU=1"}
而Yavirt里需要自己手动实现这块，在ETCD里把labels解析出来，所以Yavirt需要的格式是：{Key: "ERU", Value: "1"}

所以应该在manager的层面实现一个getBaseFilter，返回的是常规的键值对。如果Runtime层面想要对labels进行特殊处理的话，自己手动转换一下。

以及重构前的版本getFilter有一个extend参数，我感觉没什么用，给去掉了。

types.KV全部替换为map[string]string，当初为了应对docker里key可能重复（都叫"label"）的时候引入的。

DuodenumL · 2021-09-15T10:01:25Z

这个先不用合并，随着Yavirt的迁移工作，可能agent这里还会有若干细节要改动，放一个PR里好了...

比如我刚刚发现ListWorkloadIDs(ctx context.Context, all bool, filter map[string]string)里的all现在已经没有用了，所有用到这个方法的地方都是all=true。（以前agent启动时load的时候用的all=false，但是上次重构的时候改掉了。）

以及Yavirt里一些细节跟Docker是不一样的。例如Docker可以根据容器名字来解析出来app / entrypoint / ident，但是Yavirt里的VM只有一个ID，是解析不出来的。好在只有Attach用了这部分功能，但是Yavirt的Attach应该暂时不用做。

DuodenumL · 2021-09-17T09:01:04Z

因为libyavirt那边还没合，现在go mod里用我自己的版本replace了...所以在libyavirt合并之前，这里不应该合...

以及Yavirt里monitor部分的代码应该先去掉？还是和agent共存一段时间？

jschwinger233

manager 和 agent 退出逻辑好像有点问题, 你的写法是出现一个 event watch error 就直接整个进程关闭, 但我觉得不应该. event watch error 多正常, docker / yavirt 重启都会报错, agent 就等着重连嘛.

jschwinger233 · 2021-09-20T07:26:42Z

runtime/yavirt/yavirt.go

+// ListWorkloadIDs lists workload IDs filtered by given condition
+func (y *Yavirt) ListWorkloadIDs(ctx context.Context, filters map[string]string) ([]string, error) {
+	var ids []string
+	var err error


可以把这个变量声明放到函数返回值里:

func (y *Yavirt) ListWorkloadIDs(ctx context.Context, filters map[string]string) (ids []string, err error) {

jschwinger233 · 2021-09-20T07:29:37Z

runtime/yavirt/yavirt.go

+	var yaEventChan <-chan yavirttypes.EventMessage
+	var yaErrChan <-chan error
+
+	yaEventChan, yaErrChan = y.client.Events(ctx, filters)


这两个变量好像不用单独声明类型, 直接 yaEventChan, yaErrChan :=

jschwinger233 · 2021-09-20T07:36:02Z

runtime/yavirt/guest.go

+	HTTPCode int
+}
+
+type healthCheckBridge struct {


为啥叫这名字

因为yavirt里就叫这个...

jschwinger233 · 2021-09-20T07:43:40Z

manager/workload/load.go

 	if err != nil {
 		return err
 	}

 	wg := &sync.WaitGroup{}
 	for _, wid := range workloadIDs {
-		log.Debugf("[load] detect workload %s", coreutils.ShortID(wid))
+		log.Debugf("[load] detect workload %s", wid)
 		wg.Add(1)
 		go func(ID string) {
 			defer wg.Done()


这个函数下面(36L) 的 log.Errorf 信息量不够, 如果真有报错, 看日志甚至不知道是哪个实例上报状态失败, 至少应该加上 ID

jschwinger233 · 2021-09-20T07:45:31Z

manager/workload/load.go

@@ -4,21 +4,19 @@ import (
 	"context"
 	"sync"

-	coreutils "github.com/projecteru2/core/utils"
-
 	log "github.com/sirupsen/logrus"
 )

 func (m *Manager) load(ctx context.Context) error {


这个名字也不是很好, "load workload" 听起来像是一个无副作用的操作

jschwinger233 · 2021-09-20T08:03:40Z

manager 部分的实现上次没细看, 这次看到了一些问题.

manager/node/manager.go 里的 Manager.Run(Context) 函数:

// Run runs a node manager
func (m *Manager) Run(ctx context.Context) error {
	log.Infof("[NodeManager] start node status heartbeat")
	go m.heartbeat(ctx)

	// wait for signal
	<-ctx.Done()
	log.Info("[NodeManager] exiting")
	log.Infof("[NodeManager] mark node %s as down", m.config.HostName)

	// ctx is now canceled. use a new context.
	var err error
	utils.WithTimeout(context.TODO(), m.config.GlobalConnectionTimeout, func(ctx context.Context) {
		err = m.store.SetNode(ctx, m.config.HostName, false)
	})
	if err != nil {
		log.Errorf("[NodeManager] failed to mark the node %s as down, err: %s", m.config.HostName, err)
		return err
	}
	return nil
}

由于你上层也 select 了 <-ctx.Done(), 所以信号来的时候上层直接 return 了然后结束进程, 你这里的 set node down 的操作根本来不及进行.

需要一个同步机制来实现 graceful termination, 收到信号, 等待清理工作做完, 再退出.

jschwinger233 · 2021-09-20T08:12:26Z

因此你在自测的时候, 也要覆盖到 dockerd / yavirtd 挂掉时候的场景.

jschwinger233

跑起来过吗

jschwinger233 · 2021-09-21T08:23:59Z

agent.go

+		defer cancel()
+		if err := workloadManager.Run(ctx); err != nil {
+			log.Errorf("[agent] workload manager err: %v, exiting", err)
+			cancel()


cancel 是不是太多啦, defer 也有

jschwinger233 · 2021-09-21T08:29:10Z

有仔细测试过各种场景吗, 包括 dockerd 挂掉, dockerd 卡住, dockerd 重启, 容器无限自动重启, 日志转发, 日志 attach, 还可以开一下 pprof 看看折腾完很多操作后有没有 goroutine 泄露

jschwinger233 · 2021-09-21T08:31:59Z

毕竟这影响的是业务的 failover, 要灵敏, 要准确, 就算出了问题也要可追溯, 日志的完整性足够吗, 比如 health check 的时候 tcp 连不上的日志有记录实例 id 吗, 如果 tcp 卡住会积压 ticker 吗, setstatus 失败日志能反映出问题吗, 几个重要的 IO 都去创造一下失败看看会怎样.

jschwinger233 · 2021-09-21T08:32:42Z

多个 core 实例的 HA 也测一下, 两个 core server, 两个端口, 轮流 kill -9 看看 agent 的表现.

jschwinger233 · 2021-09-21T08:34:10Z

graceful term 也测一下, 就是发个 15 信号, 看看能不能标记 node down, 还有容器会标记 unhealthy 吗?

DuodenumL · 2021-09-23T11:08:12Z

除了Yavirt相关，本次PR也修了一些历史遗留bug和这次重构新产生的bug：

优化：Runtime daemon退出时会导致agent整体退出的问题
修复：Bufpipe在特殊情况下Read会无限阻塞的问题
修复：logs/Writer内的重试机制可能会导致goroutine数量爆炸的问题
修复：容器无限重启时，backoff retry可能会导致goroutine数量爆炸的问题
修复：容器无限重启时，logBroadcaster可能会阻塞的问题

tonicmuroq · 2021-09-24T02:17:56Z

logs/writer.go

+// Close .
+func (w *Writer) Close() error {
+	// leave some time for the pending writing
+	time.Sleep(CloseWaitInterval)


那 Close 不是一个阻塞的操作了咩?

tonicmuroq · 2021-09-24T02:18:42Z

manager/workload/attach.go

 		return
 	}
+	defer func() {
+		go writer.Close()


... 还真是

= =要不我把go放Close里边？觉得异步调用Close有点奇怪

tonicmuroq · 2021-09-24T02:21:41Z

manager/workload/log.go

@@ -96,14 +115,22 @@ func (l *logBroadcaster) broadcast(log *types.Log) {
 	// use wait group to make sure the logs are ordered
 	wg := &sync.WaitGroup{}
 	wg.Add(len(subscribers))
-	for _, sub := range subscribers {
-		go func(sub *subscriber) {
+	for ID, sub := range subscribers {


... 我真的是受不了这个 ID, 是要 exported 吗

无视我= =

tonicmuroq · 2021-09-24T02:22:44Z

runtime/docker/docker.go

 	}()

 	return outr, errr, nil
 }

+// checkHostname check if ERU_NODE_NAME env in container is the hostname of this agent
+// TODO should be removed in the future, should always use label to filter


觉得这个遥遥无期了...

agent.go

CMGS · 2021-09-27T06:53:09Z

logs/writer.go

@@ -17,16 +17,25 @@ import (
 const Discard = "__discard__"

 // ErrConnecting means writer is in connecting status, waiting to be connected
-var ErrConnecting = errors.New("Connecting")
+var ErrConnecting = errors.New("connecting")


竟然有 error.go 了

CMGS · 2021-09-27T06:54:34Z

manager/workload/attach.go

+		if err != common.ErrNotImplemented {
+			log.Errorf("[attach] failed to get workload name, id: %v, err: %v", ID, err)
+		} else {
+			log.Debugf("[attach] should ignore this workload")


log.Debug, not format

manager/workload/event.go

CMGS · 2021-09-27T08:05:45Z

LGTM...maybe

DuodenumL force-pushed the feature/filter branch from e961115 to 43b9e49 Compare September 17, 2021 08:57

DuodenumL force-pushed the feature/filter branch from 43b9e49 to d7f5f58 Compare September 17, 2021 09:03

DuodenumL changed the title ~~make "getFilter" more universal~~ [WIP] add "yavirt" as a new runtime Sep 20, 2021

DuodenumL changed the title ~~[WIP] add "yavirt" as a new runtime~~ [WIP] [SPVR-67] add "yavirt" as a new runtime Sep 20, 2021

jschwinger233 requested changes Sep 20, 2021

View reviewed changes

add "yavirt" as a new runtime type

d4d39e5

DuodenumL force-pushed the feature/filter branch 5 times, most recently from 23b6eb3 to 8fbe7d0 Compare September 21, 2021 07:27

jschwinger233 reviewed Sep 21, 2021

View reviewed changes

DuodenumL force-pushed the feature/filter branch from 8fbe7d0 to 8ca0b73 Compare September 21, 2021 08:31

monitor with retry

7637a70

DuodenumL force-pushed the feature/filter branch 3 times, most recently from a81fa9f to 1fd2687 Compare September 21, 2021 10:47

fix attach bug

ddf6f4b

DuodenumL force-pushed the feature/filter branch 4 times, most recently from b8bd43a to 829e3a9 Compare September 22, 2021 07:46

refactor writer

c117560

DuodenumL force-pushed the feature/filter branch 4 times, most recently from eb7cbc7 to 84ec68f Compare September 23, 2021 03:44

refactor backoff retry

6c086a8

DuodenumL force-pushed the feature/filter branch 2 times, most recently from 54411c5 to 757f14f Compare September 23, 2021 10:31

DuodenumL changed the title ~~[WIP] [SPVR-67] add "yavirt" as a new runtime~~ [SPVR-67] add "yavirt" as a new runtime Sep 23, 2021

tonicmuroq reviewed Sep 24, 2021

View reviewed changes

DuodenumL force-pushed the feature/filter branch from 757f14f to 7e1fa43 Compare September 24, 2021 09:21

fix log broadcaster bug

99fd8c3

DuodenumL force-pushed the feature/filter branch from 7e1fa43 to 99fd8c3 Compare September 27, 2021 02:47

CMGS reviewed Sep 27, 2021

View reviewed changes

agent.go Outdated Show resolved Hide resolved

CMGS reviewed Sep 27, 2021

View reviewed changes

manager/workload/event.go Outdated Show resolved Hide resolved

standardize the usage of logrus

915477c

CMGS merged commit a56aaed into projecteru2:master Sep 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPVR-67] add "yavirt" as a new runtime #73

[SPVR-67] add "yavirt" as a new runtime #73

DuodenumL commented Sep 14, 2021 •

edited

Loading

DuodenumL commented Sep 15, 2021

DuodenumL commented Sep 17, 2021

jschwinger233 left a comment

jschwinger233 Sep 20, 2021

jschwinger233 Sep 20, 2021

jschwinger233 Sep 20, 2021

DuodenumL Sep 20, 2021

jschwinger233 Sep 20, 2021

jschwinger233 Sep 20, 2021

jschwinger233 commented Sep 20, 2021 •

edited

Loading

jschwinger233 commented Sep 20, 2021

jschwinger233 left a comment

jschwinger233 Sep 21, 2021

jschwinger233 commented Sep 21, 2021

jschwinger233 commented Sep 21, 2021

jschwinger233 commented Sep 21, 2021

jschwinger233 commented Sep 21, 2021

DuodenumL commented Sep 23, 2021

tonicmuroq Sep 24, 2021

tonicmuroq Sep 24, 2021

DuodenumL Sep 24, 2021

tonicmuroq Sep 24, 2021

tonicmuroq Sep 24, 2021

tonicmuroq Sep 24, 2021

CMGS Sep 27, 2021

CMGS Sep 27, 2021

CMGS commented Sep 27, 2021

[SPVR-67] add "yavirt" as a new runtime #73

[SPVR-67] add "yavirt" as a new runtime #73

Conversation

DuodenumL commented Sep 14, 2021 • edited Loading

DuodenumL commented Sep 15, 2021

DuodenumL commented Sep 17, 2021

jschwinger233 left a 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

jschwinger233 commented Sep 20, 2021 • edited Loading

jschwinger233 commented Sep 20, 2021

jschwinger233 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschwinger233 commented Sep 21, 2021

jschwinger233 commented Sep 21, 2021

jschwinger233 commented Sep 21, 2021

jschwinger233 commented Sep 21, 2021

DuodenumL commented Sep 23, 2021

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

CMGS commented Sep 27, 2021

DuodenumL commented Sep 14, 2021 •

edited

Loading

jschwinger233 commented Sep 20, 2021 •

edited

Loading