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

Jump to bottom

[patch] bugfix: do not create metadata file when create/append flag is not set for the agent/agent-sidecar #904

Merged

kpango merged 15 commits into master from bugfix/agent-sidecar/empty-metadata-file-opened

Jan 12, 2021

Contributor

kevindiu commented Dec 28, 2020 •

edited

Loading

Description:

This PR fix a bug when the agent sidecar try to open the metadata.json file and if the file is not exists, the empty file will be created and causing agent failed to start.

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.15.2
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.12.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.

Collaborator

vdaas-ci commented Dec 28, 2020

[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

github-actions bot added area/internal size/S type/bug priority/medium labels

kpango previously approved these changes

View reviewed changes

Collaborator

kpango left a comment

SGTM. please check it by e2e testing.

codecov bot commented Dec 28, 2020 •

edited

Loading

Codecov Report

Merging #904 (318ba84) into master (c4df6ef) will increase coverage by 0.04%.
The diff coverage is 8.00%.

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
+ Coverage   19.19%   19.23%   +0.04%     
==========================================
  Files         422      423       +1     
  Lines       19548    19622      +74     
==========================================
+ Hits         3752     3775      +23     
- Misses      15586    15631      +45     
- Partials      210      216       +6

Impacted Files	Coverage Δ
pkg/agent/core/ngt/service/ngt.go	`0.00% <0.00%> (ø)`
internal/file/file.go	`71.42% <50.00%> (+11.42%)`	⬆️
internal/db/storage/blob/s3/reader/reader.go	`65.51% <0.00%> (-32.19%)`	⬇️
internal/errgroup/group.go	`92.42% <0.00%> (-1.52%)`	⬇️
internal/net/net.go	`85.18% <0.00%> (ø)`
internal/worker/worker.go	`82.29% <0.00%> (+1.04%)`	⬆️
internal/worker/queue.go	`98.33% <0.00%> (+3.33%)`	⬆️

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 c1cfcd7...b61f5ce. Read the comment docs.

Collaborator

kpango commented Dec 28, 2020

please check internal testing error.

kevindiu dismissed kpango’s stale review via

c43d44f

December 28, 2020 06:31

github-actions bot added the area/agent/core label

github-actions bot reviewed

View reviewed changes

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

@@ @@ -170,12 +170,26 @@ func (n *ngt) initNGT(opts ...core.Option) (err error) { @@
               		n.core, err = core.New(opts...)
               		return err
               	}
+              	if os.IsPermission(err) {

Contributor

github-actions bot Dec 28, 2020

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

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

@@ @@ -170,12 +170,26 @@ func (n *ngt) initNGT(opts ...core.Option) (err error) { @@
               		n.core, err = core.New(opts...)
               		return err
               	}
+              	if os.IsPermission(err) {
+              		log.Debugf("no permission for index path,\tpath: %s,\terr: %v", n.path, err)
+              		return err

Contributor

github-actions bot Dec 28, 2020

[golangci] _{reported by reviewdog 🐶}
return with no blank line before (nlreturn)

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

kpango reviewed

View reviewed changes

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

               	log.Debugf("load index from %s", n.path)
               	agentMetadata, err := metadata.Load(filepath.Join(n.path, metadata.AgentMetadataFileName))
               	if err != nil {
               		log.Warnf("cannot read metadata from %s: %s", metadata.AgentMetadataFileName, err)

Collaborator

kpango Dec 28, 2020

I think we should check metadata.Load's err is os.IsNotExists or not, if err is os.ErrNotExists we can check kvsdb exsitance.

github-actions bot added size/M and removed size/S labels

github-actions bot reviewed

View reviewed changes

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

kevindiu changed the title ~~[bugfix] do not create file when create/append flag is not set~~ [WIP] [bugfix] do not create file when create/append flag is not set

kevindiu changed the title ~~[WIP] [bugfix] do not create file when create/append flag is not set~~ [WIP] [bugfix] do not create metadata file when create/append flag is not set for the agent/agent-sidecar

github-actions bot reviewed

View reviewed changes

pkg/agent/internal/metadata/metadata.go

-              func Load(path string) (*Metadata, error) {
+              func Load(path string) (meta *Metadata, err error) {
+              	var fi os.FileInfo
+              	if fi, err = os.Stat(path); err != nil {

Contributor

github-actions bot Dec 28, 2020

[golangci] _{reported by reviewdog 🐶}
if statements should only be cuddled with assignments used in the if statement itself (wsl)

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

kevindiu requested review from kpango and rinx

January 5, 2021 01:07

Contributor Author

kevindiu commented Jan 5, 2021

/rebase

Collaborator

vdaas-ci commented Jan 5, 2021

[REBASE] Rebase triggered by kevindiu for branch: bugfix/agent-sidecar/empty-metadata-file-opened

kevindiu added 8 commits

January 5, 2021 01:15


          bugfix: do not create file when create/append flag is not set

cd13f4a


          add logging message & check file permission before execution of vald …

0b0c694

…agent


          check metadata exist before handling kvsdb file

fdf47f9


          handle metadata length 0 in agent sidecar

86e39a1


          add debug log

1b997da


          handle metadata & kvsdb file empty

5a7b4b1


          fix compilation error

3d4f19d


          fix nil pointer panic

48cf3d8

kevindiu added 3 commits

January 5, 2021 01:15


          remove all files

2f22049


          remove unnecessory logs

a1c7f01


          refactor bugfix logic

83835bb

github-actions bot force-pushed the bugfix/agent-sidecar/empty-metadata-file-opened branch from ee2cfc2 to 83835bb Compare

January 5, 2021 01:15

kevindiu changed the title ~~[WIP] [bugfix] do not create metadata file when create/append flag is not set for the agent/agent-sidecar~~ [bugfix] do not create metadata file when create/append flag is not set for the agent/agent-sidecar

kevindiu commented

View reviewed changes

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


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

3f0e5cf

rinx reviewed

View reviewed changes

internal/file/file.go Show resolved Hide resolved

kevindiu changed the title ~~[bugfix] do not create metadata file when create/append flag is not set for the agent/agent-sidecar~~ [patch] bugfix: do not create metadata file when create/append flag is not set for the agent/agent-sidecar


          add test case to open file

ec21933

github-actions bot added the team/set label

github-actions bot reviewed

View reviewed changes

pkg/agent/internal/metadata/metadata.go

@@ @@ -38,20 +39,27 @@ type NGT struct { @@
               	IndexCount uint64 `json:"index_count" yaml:"index_count"`
               }
-              func Load(path string) (*Metadata, error) {
+              func Load(path string) (meta *Metadata, err error) {

Contributor

github-actions bot Jan 7, 2021

[golangci] _{reported by reviewdog 🐶}
exported function Load should have comment or be unexported (golint)

rinx previously approved these changes

View reviewed changes

Contributor

rinx left a comment

LGTM 👍


          fix failed test

kevindiu dismissed rinx’s stale review via

January 7, 2021 02:53

github-actions bot added size/L and removed size/M labels

kpango approved these changes

View reviewed changes

Collaborator

kpango left a comment

LGTM


          Merge branch 'master' into bugfix/agent-sidecar/empty-metadata-file-o…

b61f5ce

…pened

kpango merged commit 1823203 into master

kpango deleted the bugfix/agent-sidecar/empty-metadata-file-opened branch

January 12, 2021 02:03

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment