remove vcache for vald agent due to vcache delete timing control failure and time ordered concurrent vector queue called vqueue #1028

kpango · 2021-02-19T20:53:28Z

Description:

We are currently experiencing a bug where the Vald agent is unable to ensure the integrity of vector Delete and Insert when performing high frequency Upsert and Delete in a multi-threaded environment.
The Vald Agent was using Vector Cache = vcache to reduce the overhead of calling FFI inline in user requests, and also to aggregate FFI calls during CreateIndex.

However, since vcache uses a sync.Map-like implementation and does not support ordered loops, depending on the timing, the old vcache may not be deleted and may be loaded during the next CreateIndex.
In addition, Insert Vector Cache (IVC) and Delete Vector Cache (DVC) compare the timestamps of execution plans with the same UUID and delete unnecessary caches to avoid duplication of FFIs.
I believe that this reciprocal cache deletion logic further expands the garbage data and the Delete and Insert operations were not executed.
To solve this problem, we need to removed the vcache and added a cache layer to ensure orderliness.

So, I implemented TOCVQ (Time Ordered Concurrent Vector Queue) to avoid this problem, always calculating the execution plan from the head of the queue (operating on the old time axis) and calling FFI calls to strongly guarantee the consistency of the results.

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.16
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.12.3

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.

vdaas-ci · 2021-02-19T20:53:48Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - replace the PR body by changelog details
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master
🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

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

codecov · 2021-02-19T21:11:09Z

Codecov Report

Merging #1028 (facbd8d) into master (f12c2b6) will decrease coverage by 0.59%.
The diff coverage is 0.74%.

@@            Coverage Diff             @@
##           master    #1028      +/-   ##
==========================================
- Coverage   15.01%   14.42%   -0.60%     
==========================================
  Files         497      495       -2     
  Lines       28703    28398     -305     
==========================================
- Hits         4310     4096     -214     
+ Misses      24123    24046      -77     
+ Partials      270      256      -14

Impacted Files	Coverage Δ
...ternal/observability/metrics/agent/core/ngt/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/handler/grpc/handler.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/service/ngt.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/service/vqueue/option.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/service/vqueue/queue.go	`0.00% <0.00%> (ø)`
internal/config/ngt.go	`100.00% <100.00%> (ø)`
internal/worker/worker.go	`83.33% <0.00%> (ø)`
internal/net/dialer.go
internal/net/option.go
internal/net/net.go
... and 4 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 f12c2b6...78997d6. Read the comment docs.

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

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

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

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

…ure and time ordered concurrent vector queue called vqueue Signed-off-by: kpango <kpango@vdaas.org>

Signed-off-by: kpango <kpango@vdaas.org>

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

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

…che to vqueue Signed-off-by: kpango <kpango@vdaas.org>

Signed-off-by: kpango <kpango@vdaas.org>

…ontrol-failure-and-add-concurrent-queue

Signed-off-by: kpango <kpango@vdaas.org>

pkg/discoverer/k8s/service/option_test.go

…ontrol-failure-and-add-concurrent-queue

github-actions · 2021-03-02T10:59:13Z

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

+	WithInsertBufferSize(100),
+}
+
+func WithErrGroup(eg errgroup.Group) Option {


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

github-actions · 2021-03-02T10:59:13Z

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

+	}
+}
+
+func WithInsertBufferSize(size int) Option {


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

github-actions · 2021-03-02T10:59:13Z

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

+	}
+}
+
+func WithDeleteBufferSize(size int) Option {


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

github-actions · 2021-03-02T10:59:13Z

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

+	}
+}
+
+func WithInsertBufferPoolSize(size int) Option {


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

github-actions · 2021-03-02T10:59:13Z

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

+	}
+}
+
+func WithDeleteBufferPoolSize(size int) Option {


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

github-actions · 2021-03-02T10:59:14Z

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

+	})
+	dup := make(map[string]bool, len(uii)/2)
+	dl := make([]int, 0, len(uii)/2)
+	for i, idx := range uii {


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

github-actions · 2021-03-02T10:59:14Z

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

+			dup[idx.uuid] = true
+		}
+	}
+	sort.Sort(sort.Reverse(sort.IntSlice(dl)))


🚫 [golangci] _{reported by reviewdog 🐶}
expressions should not be cuddled with blocks (wsl)

github-actions · 2021-03-02T10:59:14Z

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

+func (v *vqueue) RangePopInsert(ctx context.Context, f func(uuid string, vector []float32) bool) {
+	if v.finalizing.Load().(bool) {
+		for !v.finalizing.Load().(bool) {
+			time.Sleep(time.Millisecond * 100)


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

github-actions · 2021-03-02T10:59:14Z

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

+//
+
+// Package vqueue manages the vector cache layer for reducing FFI overhead for fast Agent processing.
+package vqueue


🚫 [golangci] _{reported by reviewdog 🐶}
package should be vqueue_test instead of vqueue (testpackage)

github-actions · 2021-03-02T10:59:15Z

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

+//
+
+// Package vqueue manages the vector cache layer for reducing FFI overhead for fast Agent processing.
+package vqueue


🚫 [golangci] _{reported by reviewdog 🐶}
package should be vqueue_test instead of vqueue (testpackage)

pkg/agent/core/ngt/handler/grpc/handler.go

Signed-off-by: kpango <kpango@vdaas.org>

rinx · 2021-03-03T03:01:40Z

Seems okay to merge but please ask to the others.

kpango · 2021-03-03T03:02:34Z

@vankichi @hlts2 @kevindiu @datelier can you please review this PR?

Signed-off-by: kpango <kpango@vdaas.org>

kevindiu · 2021-03-03T07:19:59Z

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

+	log.Info("create index delete phase finished")
+	n.gc()
+	log.Info("create index insert phase started")
+	n.vq.RangePopInsert(ctx, func(uuid string, vector []float32) bool {


I am not sure if it is correct:
The current logic is process all the delete request first, and insert request later

For example we have 2 request, reqA and reqB
reqA: insert vecA request, time t+0
reqB: delete vecA request, time t+10

If they are processing the same vector, even the reqB comes later then reqA, the final result will be the vecA is inserted, but maybe it is not correct.

Am I correct?

that's why I remove the Insert Queue here.
https://github.com/vdaas/vald/pull/1028/files#diff-d8459a99ea4f8ec96e01405d5f55171070363986154ad61e269c7911dbfd91a9R304-R327

I see... I think it is good.

hlts2

LGTM

kevindiu

LGTM

…urrent-queue Signed-off-by: kpango <kpango@vdaas.org>

github-actions bot added area/agent/core area/internal team/set SET team size/XXXL type/bug Something isn't working priority/medium labels Feb 19, 2021

github-actions bot reviewed Feb 19, 2021

View reviewed changes

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

kpango force-pushed the bugfix/agent/remove-vcache-due-to-timing-control-failure-and-add-concurrent-queue branch 2 times, most recently from 4dcc4a1 to 2d94bcb Compare February 19, 2021 21:08

github-actions bot added size/XXL and removed size/XXL labels Feb 19, 2021

github-actions bot reviewed Feb 19, 2021

View reviewed changes

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

github-actions bot added size/XXL and removed size/XXL labels Feb 20, 2021

kpango force-pushed the bugfix/agent/remove-vcache-due-to-timing-control-failure-and-add-concurrent-queue branch from 3e44b48 to 051e19d Compare February 21, 2021 16:10

github-actions bot added size/XXL and removed size/XXL labels Feb 21, 2021

github-actions bot reviewed Feb 21, 2021

View reviewed changes

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

github-actions bot added area/helm area/makefile team/sre SRE team type/ci size/XXL and removed size/XXL labels Feb 27, 2021

github-actions bot reviewed Feb 27, 2021

View reviewed changes

remove vcache for vald agent due to vcache delete timing control fail…

f4a5911

…ure and time ordered concurrent vector queue called vqueue Signed-off-by: kpango <kpango@vdaas.org>

kpango force-pushed the bugfix/agent/remove-vcache-due-to-timing-control-failure-and-add-concurrent-queue branch from e364393 to f4a5911 Compare February 27, 2021 21:01

github-actions bot added the size/XXL label Feb 27, 2021

add queue duplicated removal logic comment

82f0c36

Signed-off-by: kpango <kpango@vdaas.org>

github-actions bot reviewed Mar 1, 2021

View reviewed changes

kpango added 4 commits March 1, 2021 17:17

change VCacheLen method name to VQueueLen and change metrics name vca…

70ec88f

…che to vqueue Signed-off-by: kpango <kpango@vdaas.org>

fix failed test

f9dc344

Signed-off-by: kpango <kpango@vdaas.org>

Merge branch 'master' into bugfix/agent/remove-vcache-due-to-timing-c…

428bd6a

…ontrol-failure-and-add-concurrent-queue

add channel buffer length metrics

160fa3a

Signed-off-by: kpango <kpango@vdaas.org>

github-actions bot added the area/discoverer label Mar 1, 2021

github-actions bot reviewed Mar 1, 2021

View reviewed changes

Merge branch 'master' into bugfix/agent/remove-vcache-due-to-timing-c…

227d472

…ontrol-failure-and-add-concurrent-queue

github-actions bot reviewed Mar 2, 2021

View reviewed changes

kpango requested review from vankichi, datelier, hlts2 and kevindiu March 2, 2021 13:39

rinx reviewed Mar 3, 2021

View reviewed changes

pkg/agent/core/ngt/handler/grpc/handler.go Outdated Show resolved Hide resolved

add delete queue buffer to uncommited index response

ec0b1e6

Signed-off-by: kpango <kpango@vdaas.org>

replace go-hostpool org monzo to kpango as a temporary workaround

facbd8d

Signed-off-by: kpango <kpango@vdaas.org>

vankichi previously approved these changes Mar 3, 2021

View reviewed changes

kevindiu reviewed Mar 3, 2021

View reviewed changes

hlts2 previously approved these changes Mar 3, 2021

View reviewed changes

kevindiu previously approved these changes Mar 3, 2021

View reviewed changes

bugfix/agent/remove-vcache-due-to-timing-control-failure-and-add-conc…

78997d6

…urrent-queue Signed-off-by: kpango <kpango@vdaas.org>

kpango dismissed stale reviews from kevindiu, hlts2, and vankichi via 78997d6 March 3, 2021 08:19

kpango merged commit 972f77d into master Mar 3, 2021

kpango deleted the bugfix/agent/remove-vcache-due-to-timing-control-failure-and-add-concurrent-queue branch March 3, 2021 08:19

vdaas-ci mentioned this pull request Mar 17, 2021

[patch] Release v1.0.4 #1087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove vcache for vald agent due to vcache delete timing control failure and time ordered concurrent vector queue called vqueue #1028

remove vcache for vald agent due to vcache delete timing control failure and time ordered concurrent vector queue called vqueue #1028

kpango commented Feb 19, 2021

vdaas-ci commented Feb 19, 2021

codecov bot commented Feb 19, 2021 •

edited

Loading

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

github-actions bot Mar 2, 2021

rinx commented Mar 3, 2021

kpango commented Mar 3, 2021

kevindiu Mar 3, 2021 •

edited

Loading

kpango Mar 3, 2021

kevindiu Mar 3, 2021

hlts2 left a comment

kevindiu left a comment

remove vcache for vald agent due to vcache delete timing control failure and time ordered concurrent vector queue called vqueue #1028

remove vcache for vald agent due to vcache delete timing control failure and time ordered concurrent vector queue called vqueue #1028

Conversation

kpango commented Feb 19, 2021

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Feb 19, 2021

codecov bot commented Feb 19, 2021 • edited Loading

Codecov Report

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

github-actions bot Mar 2, 2021

Choose a reason for hiding this comment

rinx commented Mar 3, 2021

kpango commented Mar 3, 2021

kevindiu Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

kpango Mar 3, 2021

Choose a reason for hiding this comment

kevindiu Mar 3, 2021

Choose a reason for hiding this comment

hlts2 left a comment

Choose a reason for hiding this comment

kevindiu left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 19, 2021 •

edited

Loading

kevindiu Mar 3, 2021 •

edited

Loading