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.

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

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

Merged
merged 15 commits into from
Jan 12, 2021
Merged
4 changes: 3 additions & 1 deletion internal/core/ngt/ngt.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"unsafe"

"github.com/vdaas/vald/internal/errors"
"github.com/vdaas/vald/internal/log"
)

type (
Expand Down Expand Up @@ -220,7 +221,8 @@ func (n *ngt) loadOptions(opts ...Option) (err error) {

func (n *ngt) create() (err error) {
if fileExists(n.idxPath) {
if err = os.Remove(n.idxPath); err != nil {
log.Infof("index path exists, will remove the directory. path: %s", n.idxPath)
if err = os.RemoveAll(n.idxPath); err != nil {
return err
}
}
Expand Down
1 change: 1 addition & 0 deletions internal/errors/meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ package errors

var (
ErrInvalidMetaDataConfig = New("invalid metadata config")
ErrMetadataFileEmpty = New("metadata file empty")
)
8 changes: 5 additions & 3 deletions internal/file/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,11 @@ func Open(path string, flg int, perm os.FileMode) (*os.File, error) {
}
}

file, err = os.Create(path)
if err != nil {
return nil, err
if flg&(os.O_CREATE|os.O_APPEND) > 0 {
file, err = os.Create(path)
if err != nil {
return nil, err
}
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
}

if file != nil {
Expand Down
41 changes: 41 additions & 0 deletions internal/file/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package file

import (
"os"
"syscall"
"testing"

"github.com/vdaas/vald/internal/errors"
Expand Down Expand Up @@ -138,6 +139,46 @@ func TestOpen(t *testing.T) {
err: errors.ErrPathNotSpecified,
},
},

{
name: "returns (nil, error) when file does not exists and flag is not CREATE or APPEND",
args: args{
path: "dummy",
flg: os.O_RDONLY,
perm: os.ModeDir,
},
want: want{
want: nil,
err: &os.PathError{
Op: "open",
Path: "dummy",
Err: func() error {
_, err := syscall.Open("dummy", syscall.O_RDONLY|syscall.O_CLOEXEC, 0)
return err
}(),
},
},
},

{
name: "returns (nil, error) when the folder does not exists and flag is not CREATE or APPEND",
args: args{
path: "dummy/dummy",
flg: os.O_RDONLY,
perm: os.ModeDir,
},
want: want{
want: nil,
err: &os.PathError{
Op: "open",
Path: "dummy/dummy",
Err: func() error {
_, err := syscall.Open("dummy/", syscall.O_RDONLY|syscall.O_CLOEXEC, 0)
return err
}(),
},
},
},
}

for _, test := range tests {
Expand Down
31 changes: 29 additions & 2 deletions pkg/agent/core/ngt/service/ngt.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,17 +166,43 @@ func New(cfg *config.NGT, opts ...Option) (nn NGT, err error) {
}

func (n *ngt) initNGT(opts ...core.Option) (err error) {
if _, err = os.Stat(n.path); os.IsNotExist(err) || n.inMem {
if n.inMem {
log.Debug("vald agent starts with in-memory mode")
n.core, err = core.New(opts...)
return err
}

_, err = os.Stat(n.path)
if os.IsNotExist(err) {
log.Debugf("index file not exists,\tpath: %s,\terr: %v", n.path, err)
n.core, err = core.New(opts...)
return err
}
if os.IsPermission(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

log.Debugf("no permission for index path,\tpath: %s,\terr: %v", n.path, err)
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

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)
}
if os.IsNotExist(err) || agentMetadata == nil || agentMetadata.NGT == nil || agentMetadata.NGT.IndexCount == 0 {
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
log.Warnf("cannot read metadata from %s: %v", metadata.AgentMetadataFileName, err)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if fi, err := os.Stat(filepath.Join(n.path, kvsFileName)); os.IsNotExist(err) || fi.Size() == 0 {
log.Warn("kvsdb file is not exist")
n.core, err = core.New(opts...)
return err
}

if os.IsPermission(err) {
log.Debugf("no permission for kvsdb file,\tpath: %s,\terr: %v", filepath.Join(n.path, kvsFileName), err)
return err
kevindiu marked this conversation as resolved.
Show resolved Hide resolved
}
}

var timeout time.Duration
if agentMetadata != nil && agentMetadata.NGT != nil {
Expand All @@ -191,7 +217,7 @@ func (n *ngt) initNGT(opts ...core.Option) (err error) {
),
)
} else {
log.Debugf("cannot inspect the backup index size. starting to load.")
log.Debugf("cannot inspect the backup index size. starting to load default value.")
timeout = time.Duration(math.Min(float64(n.minLit), float64(n.maxLit)))
}

Expand Down Expand Up @@ -270,6 +296,7 @@ func (n *ngt) loadKVS() error {
m := make(map[string]uint32)
err = gob.NewDecoder(f).Decode(&m)
if err != nil {
log.Errorf("error decoding kvsdb file,\terr: %v", err)
return err
}

Expand Down
14 changes: 11 additions & 3 deletions pkg/agent/internal/metadata/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"os"

"github.com/vdaas/vald/internal/encoding/json"
"github.com/vdaas/vald/internal/errors"
"github.com/vdaas/vald/internal/file"
)

Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

var fi os.FileInfo
if fi, err = os.Stat(path); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return nil, err
}
if fi.Size() == 0 {
return nil, errors.ErrMetadataFileEmpty
}

f, err := file.Open(path, os.O_RDONLY|os.O_SYNC, os.ModePerm)
if err != nil {
return nil, err
}
defer f.Close()

var meta Metadata
err = json.Decode(f, &meta)
if err != nil && err != io.EOF {
return nil, err
}

return &meta, nil
return meta, nil
}

func Store(path string, meta *Metadata) error {
Expand Down