add agent sidecar flame for implementation #404

kpango · 2020-05-25T12:26:55Z

Description:

We decided to implement blob storage backup functionality.
so, I added a prototype of agent sidecar flame, @rinx please implement backup logic after this PR merged.

Related Issue:

How Has This Been Tested?:

Environment:

Golang Version: 1.14.3
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.11.5

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

Signed-off-by: kpango <[email protected]>

pull-assistant · 2020-05-25T12:26:57Z

Best reviewed: commit by commit

Optimal code review plan

add agent sidecar

fix

Powered by Pull Assistant. Last update 376857c ... 06897f5. Read the comment docs.

vdaas-ci · 2020-05-25T12:27:19Z

[WARNING] Changes in interal/config may require you to change Helm charts. Please check.

codecov · 2020-05-25T12:34:30Z

Codecov Report

Merging #404 into master will decrease coverage by 0.17%.
The diff coverage is 0.80%.

@@            Coverage Diff            @@
##           master    #404      +/-   ##
=========================================
- Coverage    7.88%   7.71%   -0.18%     
=========================================
  Files         356     372      +16     
  Lines       18380   18850     +470     
=========================================
+ Hits         1450    1454       +4     
- Misses      16721   17183     +462     
- Partials      209     213       +4

Impacted Files	Coverage Δ
cmd/agent/core/ngt/main.go	`0.00% <ø> (ø)`
cmd/agent/sidecar/main.go	`0.00% <0.00%> (ø)`
...k/benchmark/internal/starter/agent/core/ngt/ngt.go	`0.00% <ø> (ø)`
...enchmark/internal/starter/agent/core/ngt/option.go	`5.26% <ø> (ø)`
hack/tools/config/agent/core/ngt/main.go	`0.00% <ø> (ø)`
internal/client/agent/grpc/client.go	`0.00% <ø> (ø)`
internal/config/ngt.go	`0.00% <0.00%> (ø)`
internal/config/sidecar.go	`0.00% <0.00%> (ø)`
internal/file/watch/option.go	`0.00% <0.00%> (ø)`
internal/file/watch/watch.go	`0.00% <0.00%> (ø)`
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb392fc...06897f5. Read the comment docs.

pkg/agent/sidecar/handler/grpc/option.go

rinx

Overall it looks good. Please revise several lines i pointed out. 👍

internal/file/watch/option.go

internal/file/watch/watch.go

rinx · 2020-05-26T00:28:16Z

internal/file/watch/watch.go

+						err = w.onChmod(ctx, event.Name)
+					}
+				}
+			}


Suggested change

}

case err, ok = <-w.w.Errors:

}

and remove the goroutine started from L145.
it's better to reduce the number of useless goroutines.

fsnotify recommend to watch event and error in a different goroutine.
howeyc/fsnotify#7

I read it but i couldn't found a recommendation about that.
i think it's just about "not to create a fsnotify Watcher and monitor channels in a same goroutine".

I'm considering about the race condition about init() func when the w.w.Errors and w.w.Events closed simultaneously.

sorry they recommend this thing in README.md

Do I have to watch the Error and Event channels in a separate goroutine? As of now, yes. Looking into making this single-thread friendly (see howeyc #7)

and I agree with init race condition, I'll fix race condition problem about it but I think it's okay to run in different goroutine that mentioned in howeyc/fsnotify#7

as described in README.md of fsnotify, it is only needed "a separate goroutine".
we don't have to have two goroutines here.

a separate goroutine doesn't mean using different goroutine?

internal/config/sidecar.go

pkg/agent/sidecar/service/option.go

vdaas-ci · 2020-05-26T01:34:13Z

[WARNING] Changes in interal/config may require you to change Helm charts. Please check.

github-actions · 2020-05-26T01:39:01Z

internal/file/watch/watch.go

+	return w, nil
+}
+
+func (w *watch) Start(ctx context.Context) (<-chan error, error) {


[golangci] _{reported by reviewdog 🐶}
Function 'Start' is too long (81 > 60) (funlen)

github-actions · 2020-05-26T01:39:02Z

pkg/agent/sidecar/usecase/sidecard.go

+	so            service.StorageObserver
+}
+
+func New(cfg *config.Data) (r runner.Runner, err error) {


[golangci] _{reported by reviewdog 🐶}
Function 'New' is too long (76 > 60) (funlen)

github-actions · 2020-05-26T01:39:02Z

pkg/agent/sidecar/usecase/sidecard_test.go

+	"reflect"
+	"testing"
+
+	"github.com/vdaas/vald/internal/errors"


[golangci] _{reported by reviewdog 🐶}
File is not goimports-ed (goimports)

github-actions · 2020-05-26T01:39:02Z

pkg/agent/sidecar/usecase/sidecard.go

+}
+
+func (r *run) Start(ctx context.Context) (<-chan error, error) {
+	ech := make(chan error, 5)


[golangci] _{reported by reviewdog 🐶}
mnd: Magic number: 5, in detected (gomnd)

github-actions · 2020-05-26T01:39:02Z

pkg/agent/sidecar/service/option.go

+		return nil
+	}
+}
+


[golangci] _{reported by reviewdog 🐶}
File is not goimports-ed (goimports)

github-actions · 2020-05-26T01:39:03Z

pkg/agent/sidecar/handler/grpc/handler_test.go

+	"reflect"
+	"testing"
+
+	"github.com/vdaas/vald/internal/errors"


[golangci] _{reported by reviewdog 🐶}
File is not goimports-ed (goimports)

vdaas-ci · 2020-05-26T01:40:47Z

[WARNING] Changes in interal/config may require you to change Helm charts. Please check.

github-actions · 2020-05-26T01:45:48Z

internal/file/watch/watch.go

+}
+
+func (w *watch) Start(ctx context.Context) (<-chan error, error) {
+	ech := make(chan error, 10)


[golangci] _{reported by reviewdog 🐶}
mnd: Magic number: 10, in detected (gomnd)

github-actions · 2020-05-26T01:45:48Z

internal/file/watch/watch.go

+				handleErr(ctx, err)
+			}
+		}
+		return nil


[golangci] _{reported by reviewdog 🐶}
unreachable: unreachable code (govet)

github-actions · 2020-05-26T01:45:48Z

internal/file/watch/watch.go

+				}
+			}
+		}
+		return nil


[golangci] _{reported by reviewdog 🐶}
unreachable: unreachable code (govet)

github-actions · 2020-05-26T01:45:48Z

pkg/agent/sidecar/handler/rest/handler_test.go

+	"reflect"
+	"testing"
+
+	"github.com/vdaas/vald/internal/errors"


[golangci] _{reported by reviewdog 🐶}
File is not goimports-ed (goimports)

github-actions · 2020-05-26T01:45:49Z

pkg/agent/sidecar/service/option.go

+		}
+		d, err := timeutil.Parse(dur)
+		if err != nil {
+			d = time.Minute * 5


[golangci] _{reported by reviewdog 🐶}
mnd: Magic number: 5, in detected (gomnd)

Signed-off-by: kpango <[email protected]>

vdaas-ci · 2020-05-26T01:51:36Z

[WARNING] Changes in interal/config may require you to change Helm charts. Please check.

github-actions · 2020-05-26T01:56:33Z

internal/file/watch/option_test.go

+func TestWithOnChmod(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		f func(ctx context.Context, name string) error


[golangci] _{reported by reviewdog 🐶}
field f is unused (unused)

github-actions · 2020-05-26T01:56:33Z

internal/file/watch/option_test.go

+func TestWithOnError(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		f func(ctx context.Context, err error) error


[golangci] _{reported by reviewdog 🐶}
field f is unused (unused)

github-actions · 2020-05-26T01:56:33Z

internal/file/watch/option_test.go

+func TestWithOnWrite(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		f func(ctx context.Context, name string) error


[golangci] _{reported by reviewdog 🐶}
field f is unused (unused)

github-actions · 2020-05-26T01:56:33Z

internal/file/watch/option_test.go

+func TestWithDirs(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		dirs []string


[golangci] _{reported by reviewdog 🐶}
field dirs is unused (unused)

github-actions · 2020-05-26T01:56:33Z

internal/file/watch/option_test.go

+func TestWithErrGroup(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		eg errgroup.Group


[golangci] _{reported by reviewdog 🐶}
field eg is unused (unused)

github-actions · 2020-05-26T01:56:34Z

internal/db/storage/blob/s3/option_test.go

+func TestWithAccessKey(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		ak string


[golangci] _{reported by reviewdog 🐶}
field ak is unused (unused)

github-actions · 2020-05-26T01:56:34Z

internal/db/storage/blob/s3/option_test.go

+func TestWithSecretAccessKey(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		sak string


[golangci] _{reported by reviewdog 🐶}
field sak is unused (unused)

github-actions · 2020-05-26T01:56:35Z

pkg/agent/sidecar/router/option_test.go

+func TestWithTimeout(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		timeout string


[golangci] _{reported by reviewdog 🐶}
field timeout is unused (unused)

github-actions · 2020-05-26T01:56:35Z

pkg/agent/sidecar/router/option_test.go

+func TestWithHandler(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		h rest.Handler


[golangci] _{reported by reviewdog 🐶}
field h is unused (unused)

github-actions · 2020-05-26T01:56:35Z

pkg/agent/sidecar/service/storage_observer.go

+}
+
+func (o *observer) backup(ctx context.Context) (err error) {
+	ctx, span := trace.StartSpan(ctx, "vald/agent-sidecar/service/StorageObserver.backup")


[golangci] _{reported by reviewdog 🐶}
ineffectual assignment to ctx (ineffassign)

Signed-off-by: kpango <[email protected]>

vdaas-ci · 2020-05-26T02:29:54Z

[WARNING] Changes in interal/config may require you to change Helm charts. Please check.

rinx

LGTM 👍 Thanks.

Signed-off-by: kpango <[email protected]>

vdaas-ci · 2020-05-26T02:40:50Z

[WARNING] Changes in interal/config may require you to change Helm charts. Please check.

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

golangci

pkg/agent/sidecar/usecase/sidecard.go|137 col 2| only one cuddle assignment allowed before if statement (wsl)
pkg/agent/sidecar/usecase/sidecard.go|140 col 2| if statements should only be cuddled with assignments (wsl)
pkg/agent/sidecar/usecase/sidecard.go|147 col 2| assignments should only be cuddled with other assignments (wsl)
pkg/agent/sidecar/usecase/sidecard.go|167 col 2| return statements should not be cuddled if block has more than two lines (wsl)
pkg/agent/sidecar/usecase/sidecard.go|178 col 2| return statements should not be cuddled if block has more than two lines (wsl)
pkg/agent/sidecar/usecase/sidecard_test.go|38 col 2| declarations should never be cuddled (wsl)
pkg/agent/sidecar/usecase/sidecard_test.go|54 col 3| if statements should only be cuddled with assignments (wsl)
pkg/agent/sidecar/usecase/sidecard_test.go|237 col 3| if statements should only be cuddled with assignments (wsl)
pkg/agent/sidecar/usecase/sidecard.go|17 col 1| ST1000: at least one file in a package should have a package comment (stylecheck)
internal/file/watch/watch.go|94 col 1| cognitive complexity 54 of func (*watch).Start is high (> 30) (gocognit)

github-actions · 2020-05-26T02:45:30Z

pkg/agent/sidecar/service/option_test.go

+func TestWithBackupDuration(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		dur string


[golangci] _{reported by reviewdog 🐶}
field dur is unused (unused)

github-actions · 2020-05-26T02:45:30Z

pkg/agent/sidecar/service/option_test.go

+	type test struct {
+		name string
+		args args
+		want want


[golangci] _{reported by reviewdog 🐶}
field want is unused (unused)

github-actions · 2020-05-26T02:45:30Z

pkg/agent/sidecar/service/option_test.go

+	type args struct {
+		dur string
+	}
+	type want struct {


[golangci] _{reported by reviewdog 🐶}
type want is unused (unused)

github-actions · 2020-05-26T02:45:31Z

pkg/agent/sidecar/service/option_test.go

+func TestWithDirs(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		dirs []string


[golangci] _{reported by reviewdog 🐶}
field dirs is unused (unused)

github-actions · 2020-05-26T02:45:31Z

pkg/agent/sidecar/service/option_test.go

+func TestWithBackupDurationLimit(t *testing.T) {
+	type T = interface{}
+	type args struct {
+		dur string


[golangci] _{reported by reviewdog 🐶}
field dur is unused (unused)

github-actions · 2020-05-26T02:45:34Z

pkg/agent/sidecar/usecase/sidecard.go

+		service.WithBackupDurationLimit(cfg.AgentSidecar.AutoBackupDurationLimit),
+		service.WithDirs(cfg.AgentSidecar.WatchPaths...),
+	)
+	if err != nil {


[golangci] _{reported by reviewdog 🐶}
only one cuddle assignment allowed before if statement (wsl)

github-actions · 2020-05-26T02:45:34Z

pkg/agent/sidecar/usecase/sidecard.go

+	if err != nil {
+		return nil, err
+	}
+	g := handler.New(handler.WithStorageObserver(so))


[golangci] _{reported by reviewdog 🐶}
assignments should only be cuddled with other assignments (wsl)

github-actions · 2020-05-26T02:45:34Z

pkg/agent/sidecar/usecase/sidecard.go

+	if r.observability != nil {
+		return r.observability.PreStart(ctx)
+	}
+	return nil


[golangci] _{reported by reviewdog 🐶}
return statements should not be cuddled if block has more than two lines (wsl)

github-actions · 2020-05-26T02:45:34Z

pkg/agent/sidecar/usecase/sidecard.go

+
+func (r *run) Start(ctx context.Context) (<-chan error, error) {
+	ech := make(chan error, 5)
+	var soech, sech, oech <-chan error


[golangci] _{reported by reviewdog 🐶}
declarations should never be cuddled (wsl)

github-actions · 2020-05-26T02:45:34Z

pkg/agent/sidecar/usecase/sidecard.go

+func (r *run) Start(ctx context.Context) (<-chan error, error) {
+	ech := make(chan error, 5)
+	var soech, sech, oech <-chan error
+	var err error


[golangci] _{reported by reviewdog 🐶}
declarations should never be cuddled (wsl)

add agent sidecar

376857c

Signed-off-by: kpango <[email protected]>

github-actions bot added area/makefile team/set SET team team/sre SRE team type/ci area/agent/core area/agent/sidecar size/XXXL type/feature New feature labels May 25, 2020

kpango requested review from kmrmt and rinx May 25, 2020 12:29

rinx reviewed May 26, 2020

View reviewed changes

pkg/agent/sidecar/handler/grpc/option.go Outdated Show resolved Hide resolved

rinx suggested changes May 26, 2020

View reviewed changes

github-actions bot added the priority/medium label May 26, 2020

github-actions bot reviewed May 26, 2020

View reviewed changes

kpango force-pushed the feature/agent-sidecar/add-s3-sidecar branch from 2c18b64 to 6787380 Compare May 26, 2020 01:40

github-actions bot reviewed May 26, 2020

View reviewed changes

fix

d3bcfb8

Signed-off-by: kpango <[email protected]>

kpango force-pushed the feature/agent-sidecar/add-s3-sidecar branch from 6787380 to d3bcfb8 Compare May 26, 2020 01:51

github-actions bot reviewed May 26, 2020

View reviewed changes

fix

02544ec

Signed-off-by: kpango <[email protected]>

rinx previously approved these changes May 26, 2020

View reviewed changes

fix

06897f5

Signed-off-by: kpango <[email protected]>

kpango dismissed rinx’s stale review via 06897f5 May 26, 2020 02:40

rinx approved these changes May 26, 2020

View reviewed changes

github-actions bot reviewed May 26, 2020

View reviewed changes

rinx merged commit 7d5c1d1 into master May 26, 2020

rinx deleted the feature/agent-sidecar/add-s3-sidecar branch May 26, 2020 02:49

add agent sidecar flame for implementation #404

add agent sidecar flame for implementation #404

Conversation

kpango commented May 25, 2020

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

pull-assistant bot commented May 25, 2020 • edited Loading

vdaas-ci commented May 25, 2020

codecov bot commented May 25, 2020 • edited Loading

Codecov Report

rinx left a comment

Choose a reason for hiding this comment

rinx May 26, 2020

Choose a reason for hiding this comment

kpango May 26, 2020

Choose a reason for hiding this comment

rinx May 26, 2020

Choose a reason for hiding this comment

kpango May 26, 2020

Choose a reason for hiding this comment

rinx May 26, 2020

Choose a reason for hiding this comment

kpango May 26, 2020

Choose a reason for hiding this comment

vdaas-ci commented May 26, 2020

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

vdaas-ci commented May 26, 2020

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

vdaas-ci commented May 26, 2020

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

github-actions bot May 26, 2020

Choose a reason for hiding this comment

vdaas-ci commented May 26, 2020

rinx left a comment

Choose a reason for hiding this comment

vdaas-ci commented May 26, 2020

github-actions bot left a comment

Choose a reason for hiding this comment

pull-assistant bot commented May 25, 2020 •

edited

Loading

codecov bot commented May 25, 2020 •

edited

Loading