abstract whole agent #70

DuodenumL · 2021-08-25T10:50:27Z

中间版本，还在瞎改中，不代表最终品质

CMGS · 2021-08-25T10:58:08Z

agent.go

 	if err != nil {
 		return err
 	}

-	go api.Serve(config.API.Addr)
+	go profile.Serve(config.Profile.Addr)


我建议这里你不要改，你看 line 136，环境变量还是 API……另外这里确实不仅仅做 profile ，你看 metrics 接口我记得也是这里输出的

确实 profile 比 api 更不好一点, api 最多会造成一眼看过去不知道是什么 api 的困惑, profile 直接造成了误导. 想不到好名字的话就将就 api 用着先.

jschwinger233 · 2021-08-26T03:45:37Z

标题开头加上 [WIP]

jschwinger233 · 2021-08-26T05:46:42Z

runtime/runtime.go

+	Events(ctx context.Context, filters [][2]string) (<-chan types.WorkloadEventMessage, <-chan error)
+	GetStatus(ctx context.Context, workload workload.Workload) (*types.WorkloadStatus, error)
+	Runtime() string
+}


这里的 interface 有一些想法可以讨论一下:

workload.Workload 这个 interface, 我看你在 Runtime 实现里的都去做类型转化, 很僵硬, 那是不是可以考虑去掉这个模型, 直接传 ID, 比如 GetStatus(ctx, ID) 和 StatWorkload(ctx, ID).

AttachWorkload 的返回值没有区分 stdout 和 stderr

filters [][2]string 看类型猜不出参数构造方式

StatWorkload 这个接口内部去调用 mClient 了, 这一块的逻辑应该抽到上层; 一种思考方式是认为这不是 runtime 模型本身的事情, 另一种思考方式是考虑以后实现其他类型的 runtime, mClient 这一部分的逻辑就会被原封不动地复制进去, 不 DIY 了.

jschwinger233 · 2021-08-26T05:50:55Z

runtime/docker/docker.go

+		}
+	}()
+
+	return eventChan, errChan


这两个 chan 没有 close, 要尽量保证"谁创建谁关闭" 的原则; 同时这里没有处理 ctx.Done() 的情况.

jschwinger233 · 2021-08-26T07:27:09Z

@anrs 点评说这个任务很简单, 一周做完两周上线(

CMGS · 2021-08-26T07:37:20Z

我发现你喜欢用 pkg.NewPkg 这样，core 的习惯一般是 pkg.New，更直观

DuodenumL · 2021-08-30T06:41:47Z

顺便做了的事情：

曾经我们认为agent的运行状态可以代表node的alive状态，感觉不太靠谱。新的判断方式是：能否连上runtime daemon。
selfmon里用sync.Map做缓存，没有过期时间，换成了go-cache，给了个5分钟的过期时间，不知道这个设计是否合理？

DuodenumL · 2021-08-30T07:27:36Z

补充一下其它的行为变化：

之前版本agent启动时会加载所有workload并且上报状态，但是这个过程没有进行健康检查。现在在manager/workload/load.go里加上了健康检查，相应地agent启动时长会有所增加。

CMGS · 2021-08-30T07:30:27Z

profile/http.go

@@ -1,21 +1,20 @@
-package api
+package profile


not just profile

CMGS · 2021-08-30T07:41:21Z

api/api.go

@@ -0,0 +1,21 @@
+package api


CMGS · 2021-08-30T07:47:12Z

api/core/client.go

+)
+
+// CoreAPI use core to store meta
+type CoreAPI struct {


这里 golint 没报错么，理论上他 warn coreapi.CoreAPI, 比较合理的是 pkg name 叫 core，struct 叫 API，这样你 import 后调用就是 core.API 初始化函数是 core.New 调用方式是 core.Get

CMGS · 2021-08-30T07:53:21Z

api/core/node.go

+)
+
+// GetNode return a node by core
+func (c *CoreAPI) GetNode(ctx context.Context, nodename string) (coretypes.Node, error) {


返回值一致性，你下面用的是 *coretypes.Node 那么这里最好都统一

CMGS · 2021-08-30T07:54:15Z

api/core/node.go

+	cctx, cancel := context.WithTimeout(ctx, c.config.GlobalConnectionTimeout)
+	defer cancel()


我如果写的话会设计一个 decor

func WithTimeout(ctx context.Context, f func () error) error { cctx, cancel := context.WithTimeout(ctx, c.config.GlobalConnectionTimeout) defer cancel() return f() } # 那么这里就可以这样写 func (c *CoreAPI) GetNode(ctx context.Context, nodename string) (*coretypes.Node, error) { var node *coretypes.Node err := WithTimeout(ctx, func () error { bla bla }) return node, err }

CMGS · 2021-08-30T07:59:09Z

api/core/node.go

+	if node.Available {
+		opts.StatusOpt = coretypes.TriTrue
+	} else {
+		opts.StatusOpt = coretypes.TriFalse
+	}
+


see line 74....你可以先给 StatusOpt 一个默认值，减少分支预测

CMGS · 2021-08-30T07:59:24Z

api/core/node.go

+}
+
+// GetNodeStatus gets the status of node
+func (c *CoreAPI) GetNodeStatus(ctx context.Context, nodename string) (coretypes.NodeStatus, error) {


CMGS · 2021-08-30T08:00:15Z

api/core/rpcpool.go

+func checkAlive(ctx context.Context, rpc *clientWithStatus, timeout time.Duration) bool {
+	cctx, cancel := context.WithTimeout(ctx, timeout)
+	defer cancel()
+	_, err := rpc.client.Info(cctx, &pb.Empty{})


if in 1 line

CMGS · 2021-08-30T08:22:50Z

engine/status_report.go

@@ -30,7 +30,7 @@ func (e *Engine) heartbeat(ctx context.Context) {
 // nodeStatusReport does heartbeat, tells core this node is alive.
 // The TTL is set to double of HeartbeatInterval, by default it will be 360s,
 // which means if a node is not available, subcriber will notice this after at least 360s.
-// HealthCheck.Timeout is used as timeout of requesting core API
+// HealthCheck.Timeout is used as timeout of requesting core Profile


没理解为啥是 profile

这里是用Goland refactor功能的时候，它搞了个自动的字符串替换。。。太睿智了

manager/workload/attach.go

CMGS · 2021-08-30T08:38:28Z

api/api.go

+	GetNodeStatus(ctx context.Context, nodename string) (coretypes.NodeStatus, error)
+	SetWorkloadStatus(ctx context.Context, status *types.WorkloadStatus, ttl int64) error
+	SetNode(ctx context.Context, node string, status bool) error
+	GetCoreIdentifier(ctx context.Context) string


GetStoreIdentifier

CMGS · 2021-08-30T08:39:09Z

manager/workload/filter.go

+
+	if m.config.CheckOnlyMine && utils.UseLabelAsFilter() {
+		f = append(f, types.KV{Key: "label", Value: fmt.Sprintf("eru.nodename=%s", m.config.HostName)})
+		if m.coreIdentifier != "" {


dont think here is coreIdentifier, here is identify which store used, core is just one of implementation.

CMGS · 2021-08-30T08:59:31Z

types/config.go

@@ -114,8 +122,8 @@ func (config *Config) PrepareConfig(c *cli.Context) {
 	if len(c.StringSlice("metrics-transfers")) > 0 {
 		config.Metrics.Transfers = c.StringSlice("metrics-transfers")
 	}
-	if c.String("api-addr") != "" {
-		config.API.Addr = c.String("api-addr")
+	if c.String("profile-addr") != "" {


记得改回去后的一致性

DuodenumL · 2021-08-30T09:18:10Z

本来想把agent分成三个manager: WorkloadManager，NodeManager，Selfmon；但是觉得Manager没必要抽象出一个接口出来...？

CMGS · 2021-08-30T09:29:01Z

没有共同性不如就3个模块呗

jschwinger233 · 2021-08-31T06:32:56Z

runtime/runtime.go

+	GetStatus(ctx context.Context, ID string, checkHealth bool) (*types.WorkloadStatus, error)
+	GetWorkloadName(ctx context.Context, ID string) (string, error)
+	LogFieldsExtra(ctx context.Context, ID string) (map[string]string, error)
+	IsConnected(ctx context.Context) bool


我看了一下 IsConnected 的调用, 在 heartbeat.go 里, 但是哪里的语义来看, 你的调用目的是来判断本地的 runtime 是否可用? 那可能换个名字更好

jschwinger233 · 2021-08-31T06:34:22Z

runtime/runtime.go

+// Runtime provides runtime-related functions
+type Runtime interface {
+	AttachWorkload(ctx context.Context, ID string) (io.Reader, io.Reader, error)
+	StatWorkload(ctx context.Context, ID string)


这个接口语义不明确啊, 比如我现在要实现一个 yavirt 的 runtime, 那应该怎么实现这个方法? 语义是什么?

jschwinger233 · 2021-08-31T06:44:45Z

暂时没有什么集成测试方案(在做了在做了), 所以你得自己仔细测试每一个功能....

jschwinger233 · 2021-08-31T06:45:27Z

哦你本来就要写单测来着, 好耶

jschwinger233 · 2021-08-31T08:25:51Z

有两个重复的 Container type, 删掉一个; 然后再仔细检查下建模和接口, 设想一下假如要扩展新的 runtime 实现需要哪些改动, 来评估抽象的合理性.

DuodenumL · 2021-08-31T09:54:13Z

修复了这个issue。出现这个问题的原因是：某个workload如果被attach第二次的时候，会造成prometheus的重复register，加个缓存就行了。

DuodenumL · 2021-09-01T03:36:59Z

目前基本功能测试着都没啥问题...准备mock了！

DuodenumL · 2021-09-01T11:00:48Z

现在的一个问题是我能启动一个workload manager，用纯mock的runtime和store正常运行。但是这玩意如果一直运行，怎么assert呢...

CMGS · 2021-09-01T11:18:09Z

mock只能做白盒测试…否则就只能做测试框架了 DuodenumL ***@***.***>于2021年9月1日周三19:00写道：

现在的一个问题是我能启动一个workload manager，用纯mock的runtime和store正常运行。但是这玩意如果一直运行，怎么assert呢... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD3S2HIDRPUQCYJK5ZGFBLT7YBWXANCNFSM5CYZFYIQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- - CMGS

jschwinger233 · 2021-09-02T03:54:48Z

愿意的话集成测试也搞起来啊, 不过我这边本来也有计划把 agent 集成起来.

DuodenumL · 2021-09-03T09:08:35Z

改了一下selfmon的register实现

DuodenumL · 2021-09-03T09:56:37Z

还应该在哪里加test呢...

agent.yaml.sample

manager/node/manager.go

CMGS · 2021-09-06T07:25:43Z

agent.yaml.sample

+
+# kv_type defines the type of kv store.
+# This option is not required as the default value is "etcd".
+kv_type: etcd


我讨厌下划线在 yaml 中

CMGS · 2021-09-06T07:33:37Z

api/http.go

@@ -32,7 +31,7 @@ func (h *Handler) version(w http.ResponseWriter, _ *http.Request) {
 	_ = json.NewEncoder(w).Encode(JSON{"version": version.VERSION})
 }

-// URL /profile/
+// URL /api/


actually...no need to change

= =这里也是Goland自动干的...吐了

CMGS · 2021-09-06T07:41:40Z

manager/node/manager.go

+		if m.runtimeClient == nil {
+			return nil, errors.New("failed to get runtime client")
+		}
+	case "mocks":


jschwinger233

type RPCClientPool interface {
	GetClient() pb.CoreRPCClient
}

这个抽象是出于什么考虑?

jschwinger233 · 2021-09-07T03:42:28Z

store/core/rpcpool.go

-// CoreRPCClientPool implement of RPCClientPool
-type CoreRPCClientPool struct {
+// ClientPool implement of RPCClientPool
+type ClientPool struct {
 	rpcClients []*clientWithStatus
 }

 func checkAlive(ctx context.Context, rpc *clientWithStatus, timeout time.Duration) bool {


其实我觉得这个方法似乎可以放到 grpc client 内部

jschwinger233 · 2021-09-07T03:48:13Z

store/store.go

+	SetNode(ctx context.Context, node string, status bool) error
+	GetIdentifier(ctx context.Context) string
+	NodeStatusStream(ctx context.Context) (<-chan *coretypes.NodeStatus, <-chan error)
+	ListPodNodes(ctx context.Context, all bool, podname string, labels map[string]string) ([]*coretypes.Node, error)
 }


要不要考虑把 coretypes 都干掉

jschwinger233 · 2021-09-07T04:42:03Z

话说 watcher 那一部分没有改啊, 从建模边界来看应该是属于 workload manager 的活吧

CMGS · 2021-09-07T06:39:41Z

LGTM

@jschwinger23 和 @anrs 你们再看看

jschwinger233 · 2021-09-07T06:51:18Z

和他又勾兑了一会儿, 再改一版应该就好了, 改 watcher/log 那块

CMGS · 2021-09-08T07:19:53Z

manager/workload/log.go

+	}
+
+	ID := coreutils.RandomString(8)
+	ctx, cancel := context.WithCancel(context.TODO())


我看了你的代码，从最外层其实可以传入 req.Context()

而且从逻辑上确实也应该如此

jschwinger233 · 2021-09-08T07:21:20Z

manager/workload/log.go

+
+	l.subscribers[app][ID] = &subscriber{buf, cancel}
+	logrus.Infof("%s %s log subscribed", app, ID)
+	<-ctx.Done()


好像会永久阻塞...是预期的设计?

嗷是同步式 API...

cancel会作为subscriber的成员传到broadcast里，broadcast时如果err != nil就会调用cancel

CMGS reviewed Aug 25, 2021

View reviewed changes

DuodenumL changed the title ~~Feature/abstract~~ [WIP]Feature/abstract Aug 26, 2021

jschwinger233 reviewed Aug 26, 2021

View reviewed changes

CMGS reviewed Aug 30, 2021

View reviewed changes

profile/http.go Outdated

@@ -1,21 +1,20 @@

package api

package profile

Copy link

Contributor

CMGS Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not just profile

CMGS reviewed Aug 30, 2021

View reviewed changes

api/api.go Outdated

@@ -0,0 +1,21 @@

package api

Copy link

Contributor

CMGS Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store

CMGS reviewed Aug 30, 2021

View reviewed changes

manager/workload/attach.go Show resolved Hide resolved

CMGS reviewed Aug 30, 2021

View reviewed changes

jschwinger233 reviewed Aug 31, 2021

View reviewed changes

tonicmuroq reviewed Sep 6, 2021

View reviewed changes

agent.yaml.sample Show resolved Hide resolved

tonicmuroq reviewed Sep 6, 2021

View reviewed changes

manager/node/manager.go Show resolved Hide resolved

CMGS reviewed Sep 6, 2021

View reviewed changes

jschwinger233 reviewed Sep 7, 2021

View reviewed changes

CMGS and others added 5 commits September 7, 2021 15:41

revise selfmon context

8b9759f

Abstract "runtime"

2986220

Abstract "store"

346acbf

Replace "Engine" with "WorkloadManager" and "NodeManager"

2f158f0

Refactor selfmon

a195e70

DuodenumL force-pushed the feature/abstract branch from 7d1cd0f to a195e70 Compare September 8, 2021 07:06

CMGS reviewed Sep 8, 2021

View reviewed changes

jschwinger233 reviewed Sep 8, 2021

View reviewed changes

jschwinger233 approved these changes Sep 8, 2021

View reviewed changes

Add "HAKeepaliveInterval" in config

6102853

DuodenumL force-pushed the feature/abstract branch from c2bcd54 to 6102853 Compare September 8, 2021 08:15

CMGS changed the title ~~[WIP]Feature/abstract~~ abstract whole agent Sep 8, 2021

CMGS merged commit 16ae194 into projecteru2:master Sep 8, 2021

		cctx, cancel := context.WithTimeout(ctx, c.config.GlobalConnectionTimeout)
		defer cancel()

abstract whole agent #70

abstract whole agent #70

Conversation

DuodenumL commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschwinger233 commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschwinger233 commented Aug 26, 2021

CMGS commented Aug 26, 2021

DuodenumL commented Aug 30, 2021

DuodenumL commented Aug 30, 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

CMGS Aug 30, 2021 • 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

DuodenumL commented Aug 30, 2021

CMGS commented Aug 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschwinger233 commented Aug 31, 2021

jschwinger233 commented Aug 31, 2021

jschwinger233 commented Aug 31, 2021

DuodenumL commented Aug 31, 2021

DuodenumL commented Sep 1, 2021

DuodenumL commented Sep 1, 2021

CMGS commented Sep 1, 2021 via email

jschwinger233 commented Sep 2, 2021

DuodenumL commented Sep 3, 2021

DuodenumL commented Sep 3, 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

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

jschwinger233 commented Sep 7, 2021

CMGS commented Sep 7, 2021

jschwinger233 commented Sep 7, 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

CMGS Aug 30, 2021 •

edited

Loading