Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lint #520

Merged
merged 1 commit into from
Dec 24, 2021
Merged

lint #520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 35 additions & 29 deletions cluster/calcium/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,40 @@ func (c *Calcium) GetNode(ctx context.Context, nodename string) (*types.Node, er
return node, logger.Err(ctx, errors.WithStack(err))
}

func (c *Calcium) setAllWorkloadsOnNodeDown(ctx context.Context, nodename string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里没有返回error，感觉没必要，因为不想因为这里的error就终止SetNode的其它操作，而且该打的错误日志在里面也都打过了。不过看起来不太好看

Copy link
Contributor

Choose a reason for hiding this comment

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

你指的是 workload 的后续 mark down 操作么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不是，是SetNode后面的修改metadata的其它操作

Copy link
Contributor Author

Choose a reason for hiding this comment

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

你指的是 workload 的后续 mark down 操作么

node_test.go里改了个测试用例，就是ListNodeWorkloads报错不会影响node本身metadata的更新

workloads, err := c.store.ListNodeWorkloads(ctx, nodename, nil)
if err != nil {
log.Errorf(ctx, "[setAllWorkloadsOnNodeDown] failed to list node workloads, node %v, err: %v", nodename, errors.WithStack(err))
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

需要注意一下这里跟之前的行为是不一样的，之前ListNodeWorkloads报错是会直接return err，在这里并不会阻塞后续SetNode的各种行为。我觉得这里的似乎更合理一些，但是我不一定对（逃

Copy link
Member

Choose a reason for hiding this comment

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

我倒是关心如果 set workloads down 里出错, 恐怕很难及时知道.
似乎应该把 live 上的 sentry 部署起来.

}

for _, workload := range workloads {
appname, entrypoint, _, err := utils.ParseWorkloadName(workload.Name)
if err != nil {
log.Errorf(ctx, "[setAllWorkloadsOnNodeDown] Set workload %s on node %s as inactive failed %v", workload.ID, nodename, err)
continue
}

if workload.StatusMeta == nil {
workload.StatusMeta = &types.StatusMeta{ID: workload.ID}
}
workload.StatusMeta.Running = false
workload.StatusMeta.Healthy = false

// Set these attributes to set workload status
workload.StatusMeta.Appname = appname
workload.StatusMeta.Nodename = workload.Nodename
workload.StatusMeta.Entrypoint = entrypoint

// mark workload which belongs to this node as unhealthy
if err = c.store.SetWorkloadStatus(ctx, workload.StatusMeta, 0); err != nil {
log.Errorf(ctx, "[SetNodeAvailable] Set workload %s on node %s as inactive failed %v", workload.ID, nodename, errors.WithStack(err))
} else {
log.Infof(ctx, "[SetNodeAvailable] Set workload %s on node %s as inactive", workload.ID, nodename)
}
}
}

// SetNode set node available or not
func (c *Calcium) SetNode(ctx context.Context, opts *types.SetNodeOptions) (*types.Node, error) { // nolint
logger := log.WithField("Calcium", "SetNode").WithField("opts", opts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

借个楼，L102（原L68）这句litter.Dump要不要留着？看着挺实用的。

Copy link
Contributor

Choose a reason for hiding this comment

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

留着吧

Copy link
Member

Choose a reason for hiding this comment

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

好像 litter.Dump 是 pretty print? 要不要改成打印成单行的日志, 这样 grep 和查询都更好一点.

Expand All @@ -82,35 +116,7 @@ func (c *Calcium) SetNode(ctx context.Context, opts *types.SetNodeOptions) (*typ
}
}
if opts.WorkloadsDown {
workloads, err := c.store.ListNodeWorkloads(ctx, opts.Nodename, nil)
if err != nil {
return logger.Err(ctx, errors.WithStack(err))
}
for _, workload := range workloads {
appname, entrypoint, _, err := utils.ParseWorkloadName(workload.Name)
if err != nil {
log.Errorf(ctx, "[SetNodeAvailable] Set workload %s on node %s inactive failed %v", workload.ID, opts.Nodename, err)
continue
}

if workload.StatusMeta == nil {
workload.StatusMeta = &types.StatusMeta{ID: workload.ID}
}
workload.StatusMeta.Running = false
workload.StatusMeta.Healthy = false

// Set these attributes to set workload status
workload.StatusMeta.Appname = appname
workload.StatusMeta.Nodename = workload.Nodename
workload.StatusMeta.Entrypoint = entrypoint

// mark workload which belongs to this node as unhealthy
if err = c.store.SetWorkloadStatus(ctx, workload.StatusMeta, 0); err != nil {
log.Errorf(ctx, "[SetNodeAvailable] Set workload %s on node %s as inactive failed %v", workload.ID, opts.Nodename, errors.WithStack(err))
} else {
log.Infof(ctx, "[SetNodeAvailable] Set workload %s on node %s as inactive", workload.ID, opts.Nodename)
}
}
c.setAllWorkloadsOnNodeDown(ctx, opts.Nodename)
}
// update node endpoint
if opts.Endpoint != "" {
Expand Down
4 changes: 2 additions & 2 deletions cluster/calcium/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,10 @@ func TestSetNode(t *testing.T) {
assert.Equal(t, n.Name, name)
assert.Equal(t, n.Endpoint, "tcp://127.0.0.1:2379")
// not available
// failed by list node workloads
// can still set node even if ListNodeWorkloads fails
store.On("ListNodeWorkloads", mock.Anything, mock.Anything, mock.Anything).Return(nil, types.ErrNoETCD).Once()
_, err = c.SetNode(ctx, &types.SetNodeOptions{Nodename: "test", StatusOpt: 0, WorkloadsDown: true})
assert.Error(t, err)
assert.NoError(t, err)
workloads := []*types.Workload{{Name: "wrong_name"}, {Name: "a_b_c"}}
store.On("ListNodeWorkloads", mock.Anything, mock.Anything, mock.Anything).Return(workloads, nil)
store.On("SetWorkloadStatus",
Expand Down