[patch] [agent-NGT, sidecar] Improve S3 backup/recover behavior #556

rinx · 2020-07-07T09:24:49Z

Signed-off-by: Rintaro Okamura [email protected]

Description:

Agent NGT
- write metadata file Agent sidecar may write uncompleted backup file to object storage #551
- load timeout pkg/agent/core/ngt/service/New blocks startup process of agent-ngt #503
- default pool size configuration
Agent Sidecar
- not to overwrite existing backup files
- watch metadata file Agent sidecar may write uncompleted backup file to object storage #551
- add watch_enabled & auto_backup_enabled options

sidecar now watches only changes in {index_path}/metadata.json and it triggers a backup process.
the post_stop_timeout has different meaning compared to before.
in the post-stop process, sidecar also watches changes in {index_path}/metadata.json, and if there's changes in it, it creates a backup in S3. however, there's no changes within post_stop_timeout, also it starts to create a backup.

7/13:
metadata.json now contains "is_invalid" field that indicates the backup files are valid or not.
if it is true, sidecar will not upload the backup files and terminate itself immediately.

Related Issue:

#503 #551 #581

How Has This Been Tested?:

nothing

Environment:

Go 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.

pull-assistant · 2020-07-07T09:24:51Z

Best reviewed: commit by commit

Optimal code review plan (10 warnings)

♻️ not to overwrite existing backup files

✅ update tests for internal/compress

🔧 add default_pool_size option / add internal/json package

.../agent/core/ngt/service/ngt.go 80% changes removed in ♻️ add defaul...

✨ Add metadata package to write agent metadata when the index...

✨ revise file.Open interface

.../agent/core/ngt/service/ngt.go 45% changes removed in ♻️ refactor i...

✅ fix file test

✅ fix json tests

🎨 use filepath.Join

✨ revise watching method for ngt index backup files

...r/service/observer/observer.go 60% changes removed in ♻️ revise obs...

✅ remove dps from ngt_test

✨ kill own process when NGT index cannot be loaded within tim...

.../agent/core/ngt/service/ngt.go 59% changes removed in ♻️ revise age...

🚨 fix deepsource issues

.../agent/core/ngt/service/ngt.go 50% changes removed in ♻️ refactor i...

🚚 move internal/json -> internal/encoding/json

[agent-NGT] refactor pkg/agent/core/service using FOP (#566)

...ent/core/ngt/usecase/agentd.go 43% changes removed in ♻️ pass cfg s...

♻️ refactor initNGT func

✅ update

✨ add isValid flag to metadata

💚 revise fetch depth for run test workflow

♻️ use isInvalid because it should be default to false

internal/metadata/metadata.go 50% changes removed in 🎨 fix tag format...

🎨 fix tag format

♻️ fix poststop logic

✨ add Escape() method to errgroup

...nternal/errgroup/group_test.go 49% changes removed in Revert ":sparkles: a...

internal/errgroup/group.go 58% changes removed in Revert ":sparkles: a...

📦 add *.containers field

Revert ":package: add *.containers field"

✅ update

Revert ":white_check_mark: update"

internal/errgroup/group.go 44% changes removed in Revert ":sparkles: a...

Revert ":sparkles: add Escape() method to errgroup"

.../agent/core/ngt/service/ngt.go 83% changes removed in ♻️ exit safel...

♻️ exit safely using goroutine

🚚 move internal/metadata -> pkg/agent/internal/metadata

♻️ add defaultPoolSize const

♻️ pass cfg struct to agent service

🐳 fix dockerfiles

♻️ add watch_enabled & auto_backup_enabled options

✅ fix tests

♻️ revise agent-ngt codes based on suggestions

✅ update

♻️ revise observer poststop

Powered by Pull Assistant. Last update 7382a36 ... 25d315b. Read the comment docs.

vdaas-ci · 2020-07-07T09:25:12Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - add changelog comment
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master

codecov · 2020-07-07T09:33:30Z

Codecov Report

Merging #556 into master will decrease coverage by 0.03%.
The diff coverage is 9.42%.

@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
- Coverage   11.75%   11.71%   -0.04%     
==========================================
  Files         404      407       +3     
  Lines       20928    21207     +279     
==========================================
+ Hits         2460     2485      +25     
- Misses      18205    18445     +240     
- Partials      263      277      +14

Impacted Files	Coverage Δ
internal/compress/gob.go	`26.19% <0.00%> (-1.31%)`	⬇️
internal/compress/gzip.go	`26.98% <0.00%> (-7.02%)`	⬇️
internal/compress/lz4.go	`36.36% <0.00%> (-5.31%)`	⬇️
internal/compress/zstd.go	`29.50% <0.00%> (-5.79%)`	⬇️
internal/config/config.go	`15.78% <ø> (ø)`
internal/config/sidecar.go	`0.00% <ø> (ø)`
internal/errors/file.go	`0.00% <0.00%> (ø)`
internal/errors/ngt.go	`0.00% <ø> (ø)`
pkg/agent/core/ngt/service/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/usecase/agentd.go	`0.00% <0.00%> (ø)`
... and 14 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 ec75fba...25d315b. Read the comment docs.

vdaas-ci · 2020-07-08T06:02:58Z

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

internal/core/ngt/option.go

pkg/agent/core/ngt/service/ngt.go

internal/json/json.go

github-actions · 2020-07-09T06:29:43Z

pkg/agent/core/ngt/service/ngt.go

-	}
-	if n.dvc == nil {
-		n.dvc = new(vcaches)
+func (n *ngt) initNGT(opts ...core.Option) (err error) {


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

github-actions · 2020-07-09T06:29:43Z

pkg/agent/core/ngt/service/ngt.go

+			if err != nil {
+				return err
+			}
+			defer f.Close()


[golangci] _{reported by reviewdog 🐶}
Error return value of f.Close is not checked (errcheck)

github-actions · 2020-07-09T06:29:43Z

pkg/agent/core/ngt/service/ngt.go

+
+	eg, _ := errgroup.New(ctx)
+	eg.Go(safety.RecoverFunc(func() (err error) {
+		select {


[golangci] _{reported by reviewdog 🐶}
S1000: should use a simple channel send/receive instead of select with a single case (gosimple)

pkg/agent/core/ngt/service/ngt.go

pkg/agent/core/ngt/service/ngt_test.go

pkg/agent/core/ngt/service/option.go

Signed-off-by: Rintaro Okamura <[email protected]>

vdaas-ci · 2020-07-20T07:06:26Z

[FORMAT] Updating license headers and formatting go codes triggered by rinx.

charts/vald/values.yaml

pkg/agent/core/ngt/service/ngt.go

hlts2 · 2020-07-21T03:42:37Z

.github/workflows/test.yml

@@ -25,7 +25,7 @@ jobs:
      - name: Check out code.
        uses: actions/checkout@v1
        with:
-          fetch-depth: 1
+          fetch-depth: 10


could you please teach me about this change.

sometimes this workflow fails on master branch because of failing to fetch commits.
https://github.com/vdaas/vald/actions/runs/164399448
To prevent this, it is better to fetch not only the latest commit.

pkg/agent/sidecar/service/observer/observer.go

internal/config/ngt.go

internal/core/ngt/option.go

hlts2 · 2020-07-21T04:24:54Z

pkg/agent/core/ngt/service/ngt.go

 	}

-	if !n.inMem && len(cfg.IndexPath) != 0 {
-		n.path = cfg.IndexPath
+	if in, ok := n.indexing.Load().(bool); !ok || in {


when we call New method, indexing valueable always contains nil.
so I think the following is better 🤔

Suggested change

if in, ok := n.indexing.Load().(bool); !ok || in {

n.indexing.Store(false)

Thanks! i'll fix it.

internal/file/file_test.go

Signed-off-by: Rintaro Okamura <[email protected]>

rinx · 2020-07-21T07:58:08Z

@hlts2 @vankichi @kpango I've just fixed the codes according to your review comments.
Please review again 🙏

Signed-off-by: Rintaro Okamura <[email protected]>

internal/core/ngt/option.go

pkg/agent/core/ngt/service/ngt_test.go

pkg/agent/sidecar/service/observer/observer.go

Signed-off-by: Rintaro Okamura <[email protected]>

hlts2

LGTM

github-actions bot added area/agent/sidecar area/internal labels Jul 7, 2020

github-actions bot added size/L type/refactoring priority/low labels Jul 7, 2020

github-actions bot added team/set SET team size/XXL and removed size/L labels Jul 7, 2020

rinx force-pushed the refactor/agent-ngt-sidecar/improve-s3-backup-behavior branch from 0d3560f to acbcb8f Compare July 8, 2020 00:54

github-actions bot added size/XL and removed size/XL labels Jul 8, 2020

rinx force-pushed the refactor/agent-ngt-sidecar/improve-s3-backup-behavior branch from 833f237 to daa7c7c Compare July 8, 2020 06:02

github-actions bot added area/agent/core area/helm labels Jul 8, 2020

github-actions bot reviewed Jul 8, 2020

View reviewed changes

internal/core/ngt/option.go Outdated Show resolved Hide resolved

pkg/agent/core/ngt/service/ngt.go Show resolved Hide resolved

pkg/agent/core/ngt/service/ngt.go Outdated Show resolved Hide resolved

rinx changed the title ~~[agent-NGT, sidecar] Improve S3 backup/recover behavior~~ [patch] [agent-NGT, sidecar] Improve S3 backup/recover behavior Jul 9, 2020

rinx force-pushed the refactor/agent-ngt-sidecar/improve-s3-backup-behavior branch from e796e86 to 6544d39 Compare July 9, 2020 06:19

github-actions bot reviewed Jul 9, 2020

View reviewed changes

rinx force-pushed the refactor/agent-ngt-sidecar/improve-s3-backup-behavior branch from 0c68d8a to ca58ce7 Compare July 9, 2020 06:39

rinx marked this pull request as ready for review July 9, 2020 06:57

rinx requested review from kpango, vankichi and hlts2 July 9, 2020 06:58

rinx marked this pull request as draft July 9, 2020 07:16

rinx mentioned this pull request Jul 9, 2020

[agent-NGT] refactor pkg/agent/core/service using FOP #566

Merged

18 tasks

github-actions bot reviewed Jul 9, 2020

View reviewed changes

rinx added 7 commits July 20, 2020 07:02

♻️ exit safely using goroutine

30ae1f8

Signed-off-by: Rintaro Okamura <[email protected]>

🚚 move internal/metadata -> pkg/agent/internal/metadata

8c4ed90

Signed-off-by: Rintaro Okamura <[email protected]>

♻️ add defaultPoolSize const

fa8c6cb

Signed-off-by: Rintaro Okamura <[email protected]>

♻️ pass cfg struct to agent service

1554330

Signed-off-by: Rintaro Okamura <[email protected]>

🐳 fix dockerfiles

eac9b47

Signed-off-by: Rintaro Okamura <[email protected]>

♻️ add watch_enabled & auto_backup_enabled options

60a704d

Signed-off-by: Rintaro Okamura <[email protected]>

✅ fix tests

a8db9be

Signed-off-by: Rintaro Okamura <[email protected]>

vdaas-ci force-pushed the refactor/agent-ngt-sidecar/improve-s3-backup-behavior branch from 121eec7 to a8db9be Compare July 20, 2020 07:02

kpango reviewed Jul 21, 2020

View reviewed changes

charts/vald/values.yaml Outdated Show resolved Hide resolved

kpango reviewed Jul 21, 2020

View reviewed changes

pkg/agent/core/ngt/service/ngt.go Outdated Show resolved Hide resolved

kpango reviewed Jul 21, 2020

View reviewed changes

pkg/agent/core/ngt/service/ngt.go Outdated Show resolved Hide resolved

hlts2 reviewed Jul 21, 2020

View reviewed changes

kpango reviewed Jul 21, 2020

View reviewed changes

pkg/agent/sidecar/service/observer/observer.go Outdated Show resolved Hide resolved

hlts2 reviewed Jul 21, 2020

View reviewed changes

internal/config/ngt.go Show resolved Hide resolved

hlts2 reviewed Jul 21, 2020

View reviewed changes

internal/core/ngt/option.go Outdated Show resolved Hide resolved

hlts2 reviewed Jul 21, 2020

View reviewed changes

vankichi reviewed Jul 21, 2020

View reviewed changes

internal/file/file_test.go Show resolved Hide resolved

♻️ revise agent-ngt codes based on suggestions

c1755e5

Signed-off-by: Rintaro Okamura <[email protected]>

✅ update

bc77f2c

Signed-off-by: Rintaro Okamura <[email protected]>

github-actions bot reviewed Jul 21, 2020

View reviewed changes

internal/core/ngt/option.go Show resolved Hide resolved

internal/core/ngt/option.go Show resolved Hide resolved

internal/core/ngt/option.go Show resolved Hide resolved

pkg/agent/core/ngt/service/ngt_test.go Show resolved Hide resolved

kpango reviewed Jul 22, 2020

View reviewed changes

pkg/agent/sidecar/service/observer/observer.go Outdated Show resolved Hide resolved

kpango reviewed Jul 22, 2020

View reviewed changes

pkg/agent/sidecar/service/observer/observer.go Outdated Show resolved Hide resolved

♻️ revise observer poststop

25d315b

Signed-off-by: Rintaro Okamura <[email protected]>

kpango approved these changes Jul 22, 2020

View reviewed changes

hlts2 approved these changes Jul 22, 2020

View reviewed changes

kpango merged commit 031d78c into master Jul 22, 2020

kpango deleted the refactor/agent-ngt-sidecar/improve-s3-backup-behavior branch July 22, 2020 09:27

rinx mentioned this pull request Jul 22, 2020

Agent sidecar may write uncompleted backup file to object storage #551

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[patch] [agent-NGT, sidecar] Improve S3 backup/recover behavior #556

[patch] [agent-NGT, sidecar] Improve S3 backup/recover behavior #556

rinx commented Jul 7, 2020 •

edited

Loading

pull-assistant bot commented Jul 7, 2020 •

edited

Loading

vdaas-ci commented Jul 7, 2020

codecov bot commented Jul 7, 2020 •

edited

Loading

vdaas-ci commented Jul 8, 2020

github-actions bot Jul 9, 2020

github-actions bot Jul 9, 2020

github-actions bot Jul 9, 2020

vdaas-ci commented Jul 20, 2020

hlts2 Jul 21, 2020

rinx Jul 21, 2020

hlts2 Jul 21, 2020

rinx Jul 21, 2020

rinx commented Jul 21, 2020

hlts2 left a comment

	if in, ok := n.indexing.Load().(bool); !ok \|\| in {
	n.indexing.Store(false)

[patch] [agent-NGT, sidecar] Improve S3 backup/recover behavior #556

[patch] [agent-NGT, sidecar] Improve S3 backup/recover behavior #556

Conversation

rinx commented Jul 7, 2020 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

pull-assistant bot commented Jul 7, 2020 • edited Loading

vdaas-ci commented Jul 7, 2020

codecov bot commented Jul 7, 2020 • edited Loading

Codecov Report

vdaas-ci commented Jul 8, 2020

github-actions bot Jul 9, 2020

Choose a reason for hiding this comment

github-actions bot Jul 9, 2020

Choose a reason for hiding this comment

github-actions bot Jul 9, 2020

Choose a reason for hiding this comment

vdaas-ci commented Jul 20, 2020

hlts2 Jul 21, 2020

Choose a reason for hiding this comment

rinx Jul 21, 2020

Choose a reason for hiding this comment

hlts2 Jul 21, 2020

Choose a reason for hiding this comment

rinx Jul 21, 2020

Choose a reason for hiding this comment

rinx commented Jul 21, 2020

hlts2 left a comment

Choose a reason for hiding this comment

rinx commented Jul 7, 2020 •

edited

Loading

pull-assistant bot commented Jul 7, 2020 •

edited

Loading

codecov bot commented Jul 7, 2020 •

edited

Loading