[patch] add storage backup option to agent #367

kpango · 2020-05-11T03:32:08Z

Description:

Vald's agent is not compatible with ngtd, there is no persistent file backup.
so I implement it for the persistence storage store.

Related Issue:

How Has This Been Tested?:

Environment:

Golang Version: 1.14
Docker Version: 19.03.5
Kubernetes Version: 1.17.3
NGT Version: 1.9.1

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-05-11T03:32:10Z

Best reviewed: commit by commit

Optimal code review plan (2 warnings)

[patch] add storage backup option to agent

charts/vald/values.yaml 56% changes removed in fix

Update charts/vald/values.yaml

charts/vald/values.yaml 50% changes removed in fix

fix

Merge branch 'master' into feature/agent-ngt/add-storage-backup-functi...

🤖 Update license headers and formatting go codes

Powered by Pull Assistant. Last update 5d3fd4f ... aa23c97. Read the comment docs.

rinx

@kpango it seems okay.
there's no changes in stetefulset.yaml of agent?

Makefile.d/functions.mk

charts/vald/templates/agent/pvc.yaml

rinx · 2020-05-11T03:55:43Z

charts/vald/values.yaml

@@ -616,6 +616,15 @@ agent:
  terminationGracePeriodSeconds: 30
  # agent.podManagementPolicy -- pod management policy: OrderedReady or Parallel
  podManagementPolicy: OrderedReady
+  volumeStore:


Does this object volumeStore mean PVC settings? If it is, it should be named as pvc.

PVC is quite difficult to understand, some users such as k8s professional can understand about pv, I think it is better to describe this field as a different common name.
how about just storage

I agree.
but storage sounds too ambiguous for me.
how about to use persistentVolume like cockroachdb chart or persistence like cassandra chart?

but cockroachdb uses storage for their values.yaml

I think persistence or storage looks good

in the case of cockroachdb, storage is treated as a bigger category. not in the case of us.

it's okay to use storage as the name of this field if it is included to the description that it enables PVC. 👌

okay, I agree with 'persistentVolume'

charts/vald/values.yaml

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

Co-authored-by: Rintaro Okamura <[email protected]>

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

kpango · 2020-05-11T06:50:10Z

@rinx thank you for your review, I revised about you pointed out

codecov · 2020-05-11T06:55:34Z

Codecov Report

Merging #367 into master will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master    #367      +/-   ##
=========================================
- Coverage    7.94%   7.91%   -0.03%     
=========================================
  Files         345     346       +1     
  Lines       17733   17796      +63     
=========================================
  Hits         1408    1408              
- Misses      16144   16207      +63     
  Partials      181     181

Impacted Files	Coverage Δ
hack/benchmark/internal/assets/loader.go	`0.00% <0.00%> (ø)`
internal/file/file.go	`0.00% <0.00%> (ø)`
pkg/agent/ngt/handler/grpc/handler.go	`0.00% <0.00%> (ø)`
pkg/agent/ngt/service/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/ngt/usecase/agentd.go	`0.00% <0.00%> (ø)`

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 ca49f71...aa23c97. Read the comment docs.

charts/vald/templates/agent/statefulset.yaml

rinx · 2020-05-11T09:46:03Z

come to think of it, we should add volumeMount entry in our statefulset.yaml template to mount pvc

rinx · 2020-05-11T09:46:35Z

Sorry i just closed and reopen this PR accidentally 🙇

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

kpango · 2020-05-11T10:02:43Z

@rinx fixed

kpango · 2020-05-11T10:03:30Z

/rebase
/format

vdaas-ci · 2020-05-11T10:04:04Z

[REBASE] Rebase triggered by kpango for branch: feature/agent-ngt/add-storage-backup-functionality

vdaas-ci · 2020-05-11T10:04:08Z

[REBASE] Failed to rebase.

…ionality

kpango · 2020-05-12T00:57:27Z

@rinx could you please re-review? I revised them.
/format

vdaas-ci · 2020-05-12T00:58:39Z

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

Signed-off-by: vdaas-ci <[email protected]>

rinx

Others okay. 👍 Thanks for revising.

i'm going to investigate about filepath in mountPath

rinx · 2020-05-12T00:59:39Z

charts/vald/templates/agent/statefulset.yaml

+            {{- if .Values.agent.ngt.index_path }}
+            {{- if .Values.agent.persistentVolume.enabled }}
+            - name: {{ .Values.agent.name }}-pvc
+              mountPath: {{ .Values.agent.ngt.index_path }}


I'm not sure it's ok to use file path here.

I think its okay index_path means index dir, ngt core lib writes idx & grp files there also kvsdb writes same dir

rinx · 2020-05-12T01:03:33Z

/approve

vdaas-ci

[APPROVED] This PR is approved by rinx.

github-actions bot added area/agent area/helm area/internal area/makefile team/set SET team team/sre SRE team size/XL type/feature New feature priority/medium labels May 11, 2020

rinx reviewed May 11, 2020

View reviewed changes

[patch] add storage backup option to agent

5d3fd4f

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

kpango force-pushed the feature/agent-ngt/add-storage-backup-functionality branch from 41c2ba9 to 5d3fd4f Compare May 11, 2020 06:35

Yusuke Kato and others added 2 commits May 11, 2020 15:42

Update charts/vald/values.yaml

a1459c5

Co-authored-by: Rintaro Okamura <[email protected]>

fix

fcdf354

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

rinx reviewed May 11, 2020

View reviewed changes

charts/vald/templates/agent/statefulset.yaml Outdated Show resolved Hide resolved

rinx closed this May 11, 2020

rinx reopened this May 11, 2020

fix

a3ef683

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

Merge branch 'master' into feature/agent-ngt/add-storage-backup-funct…

819fa06

…ionality

🤖 Update license headers and formatting go codes

aa23c97

Signed-off-by: vdaas-ci <[email protected]>

rinx reviewed May 12, 2020

View reviewed changes

vdaas-ci approved these changes May 12, 2020

View reviewed changes

kpango merged commit f5ea3ef into master May 12, 2020

kpango deleted the feature/agent-ngt/add-storage-backup-functionality branch May 12, 2020 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[patch] add storage backup option to agent #367

[patch] add storage backup option to agent #367

kpango commented May 11, 2020

pull-assistant bot commented May 11, 2020 •

edited

Loading

rinx left a comment

rinx May 11, 2020

kpango May 11, 2020

rinx May 11, 2020

kpango May 11, 2020

kpango May 11, 2020

rinx May 11, 2020

kpango May 11, 2020

kpango commented May 11, 2020

codecov bot commented May 11, 2020 •

edited

Loading

rinx commented May 11, 2020

rinx commented May 11, 2020

kpango commented May 11, 2020

kpango commented May 11, 2020

vdaas-ci commented May 11, 2020

vdaas-ci commented May 11, 2020

kpango commented May 12, 2020

vdaas-ci commented May 12, 2020

rinx left a comment

rinx May 12, 2020

kpango May 12, 2020

rinx commented May 12, 2020

vdaas-ci left a comment

[patch] add storage backup option to agent #367

[patch] add storage backup option to agent #367

Conversation

kpango commented May 11, 2020

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

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

rinx 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

Choose a reason for hiding this comment

kpango commented May 11, 2020

codecov bot commented May 11, 2020 • edited Loading

Codecov Report

rinx commented May 11, 2020

rinx commented May 11, 2020

kpango commented May 11, 2020

kpango commented May 11, 2020

vdaas-ci commented May 11, 2020

vdaas-ci commented May 11, 2020

kpango commented May 12, 2020

vdaas-ci commented May 12, 2020

rinx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rinx commented May 12, 2020

vdaas-ci left a comment

Choose a reason for hiding this comment

pull-assistant bot commented May 11, 2020 •

edited

Loading

codecov bot commented May 11, 2020 •

edited

Loading