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

Bug fix for gateway #963

Merged
merged 19 commits into from
Feb 2, 2021
Merged

Bug fix for gateway #963

merged 19 commits into from
Feb 2, 2021

Conversation

kpango
Copy link
Collaborator

@kpango kpango commented Jan 29, 2021

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

Description:

bug fix of gateway for v1 release

Related Issue:

How Has This Been Tested?:

Environment:

  • Go Version: 1.15.7
  • Docker Version: 20.10.0-rc2
  • Kubernetes Version: 1.20.2
  • NGT Version: 1.13.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.

Signed-off-by: kpango <kpango@vdaas.org>
@kpango kpango changed the title V1.0.0 Stable Release [major] V1.0.0 Stable Release Jan 29, 2021
@vdaas-ci
Copy link
Collaborator

[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
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@kpango
Copy link
Collaborator Author

kpango commented Jan 29, 2021

/changelog

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Jan 29, 2021

[CHANGELOG] Please edit the following lines.

New Feature

  1. The Vald-Gateway has been divided into four Gateways (LB, Meta, Backup and Filter).
  2. API returns correct status and detailed error message.
  3. StreamAPI returns detailed error status payload for each chunked requests were failed
  4. improved vald network component performance
  5. improved observability for each function
  6. structured helm chart for managers and gateways
  7. Filter gateway is now GA, Users can issue requests with extracted feature vectors inline for search/insert/update/upsert requests.
  8. gRPC proto files are now Versioned for future changes
  9. users can access Vald agent using vald api client.
  10. internal/client packages are now GA, each vald component can use it for effective connection management and easily to send reqest.
  11. Trace & logger grpc Interceptor

Breaking Changes

  1. API interface changed.
  2. Helm Chart changed.
  3. Vald-gateway will be deprecated in a future version.
  4. k8s controller-runtime updated internal/k8s controller is now new context interface
  5. Vald Agent now uses same API definition as vald v1 gateway interface.

PRs

  • Remove actions/cache to improve workflow speed / Refactoring docker-build workflows (#957)
  • add filter gateway (#948)
  • Add Zap logger and access log interceptor (#944)
  • Add test case for internal/errors/mysql.go (#918)
  • Fix invalid data loading code in example (#949)
  • Add option to use networking.k8s.io/v1beta1 ingresses (#945)
    (bugfix gateway-lb nil pointer panic due to nil filter configuration  #943))
  • update rbac.authorization.k8s.io v1beta1 to v1 (#942)
  • Update Grafana dashboards (#940)
  • Add a gRPC interceptor for embedding payloads into trace spans (#900)
  • add goleak.IgnoreCurrent option for Parallel testing (#941)
  • update go modules (#939)
  • 🤖 Automatically update PULL_REQUEST_TEMPLATE and ISSUE_TEMPLATE (#937)
  • ⚡ Revise build command for multiplatforms (#890)
  • update dependencies (#935)
  • E2E deploy test: rewrite in go tests (#814)
  • create unit test guideline (#869)
  • feature/apis/change grpc error object (#934)
  • Add test case for internal/errors/http.go (#908)
  • 🤖 Update license headers / Format Go codes and YAML files (#933)
  • revise kubelinter config / Add securityContext section to Helm chart (#833)
  • os.free nil pointer failure in ngt cgo due to create index hang up (#930)
  • change grpc bidi-stream error handling and change grpc API interface (#928)
  • fix unclosed string literal in Dockerfile's ARG MAINTAINER (#923)
  • Revise building workflow of ci and dev containers (#922)
  • bugfix add nil check for grpc connection pool objects in grpc/client.go (#921)
  • remove unneccessary pr-tag definition from chart (#920)
  • 📝 Fix typo in gateway-vald configmap template (#919)
  • change docker base image PRIMARY_TAG name from nightly to latest (#917)
  • 💚 Use vdaas-ci token for making commits (#895)
  • Vald V1 New Design APIs (#826)
  • 🤖 Automatically update k8s manifests (#914)

kevindiu
kevindiu previously approved these changes Jan 29, 2021
Copy link
Contributor

@kevindiu kevindiu left a comment

Choose a reason for hiding this comment

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

LGTM.
Happy birthday 👏

hlts2
hlts2 previously approved these changes Jan 29, 2021
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rinx
Copy link
Contributor

rinx commented Jan 29, 2021

I'm not sure but the several dependencies are updated in this PR. Is that okay?

@kpango
Copy link
Collaborator Author

kpango commented Jan 29, 2021

@rinx I think it's okay, because each dependency are small update and I checked this build on my local cluster it works fine.

If you interest please check below each one looks not breaking changes and just update small things.
apache/cassandra-gocql-driver@9e3bb6c...14aa133
census-ecosystem/opencensus-go-exporter-stackdriver@v0.13.4...v0.13.5
https://github.com/aws/aws-sdk-go/releases/tag/v1.37.0
lucasb-eyer/go-colorful@v1.0.3...v1.2.0

rinx
rinx previously approved these changes Jan 29, 2021
@kpango
Copy link
Collaborator Author

kpango commented Jan 29, 2021

@rinx seems e2e test failed to deploy mysql.
Recently kubectl command may not find the resource even if you run a watch request immediately after deployment!
So I recently added the sleep command for scylla and it works.

example
https://github.com/vdaas/vald/blob/master/Makefile.d/k8s.mk#L261
https://github.com/vdaas/vald/blob/master/Makefile.d/k8s.mk#L282

should I add sleep command for each wait for command?

@rinx
Copy link
Contributor

rinx commented Jan 29, 2021

yes, please.

@kpango
Copy link
Collaborator Author

kpango commented Jan 29, 2021

okay, I'll do it.

Signed-off-by: kpango <kpango@vdaas.org>
@kpango kpango dismissed stale reviews from rinx, hlts2, and kevindiu via 9711da3 January 29, 2021 07:36
@rinx
Copy link
Contributor

rinx commented Jan 29, 2021

Maybe E2E tests for deploying with Cassandra requires changes like this.
442e82d

Signed-off-by: kpango <kpango@vdaas.org>
@kpango
Copy link
Collaborator Author

kpango commented Jan 29, 2021

@rinx thank you for your attention, I updated .github/helm/values/values-scylla.yaml

pkg/gateway/backup/handler/grpc/handler.go Show resolved Hide resolved
pkg/gateway/backup/handler/grpc/handler.go Show resolved Hide resolved
pkg/gateway/backup/handler/grpc/handler.go Show resolved Hide resolved
pkg/gateway/backup/handler/grpc/handler.go Show resolved Hide resolved
pkg/gateway/backup/handler/grpc/handler.go Show resolved Hide resolved
for _, req := range reqs.GetRequests() {
removeList = append(removeList, &payload.Remove_Request{
rmr.Requests = append(rmr.Requests, &payload.Remove_Request{
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 🐶
Config, XXX_NoUnkeyedLiteral, XXX_unrecognized, XXX_sizecache are missing in Remove_Request (exhaustivestruct)

vec.Config.SkipStrictExistCheck = true
}
ids = append(ids, vec.GetVector().GetId())
ireqs = append(ireqs, &payload.Insert_Request{
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 🐶
XXX_NoUnkeyedLiteral, XXX_unrecognized, XXX_sizecache are missing in Insert_Request (exhaustivestruct)

ids = append(ids, vec.GetVector().GetId())
ireqs = append(ireqs, &payload.Insert_Request{
Vector: vec.GetVector(),
Config: &payload.Insert_Config{
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 🐶
XXX_NoUnkeyedLiteral, XXX_unrecognized, XXX_sizecache are missing in Insert_Config (exhaustivestruct)

}
res, err = s.MultiInsert(ctx, &payload.Insert_MultiRequest{
log.Debugf("uuids %v were removed from %v for MultiUpdate it will execute MultiInsert soon, see detailt %#v", ids, locs.GetLocations(), locs)
locs, err = s.MultiInsert(ctx, &payload.Insert_MultiRequest{
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 🐶
XXX_NoUnkeyedLiteral, XXX_unrecognized, XXX_sizecache are missing in Insert_MultiRequest (exhaustivestruct)

@@ -783,35 +1423,61 @@ func (s *server) GetObject(ctx context.Context, req *payload.Object_VectorReques
}
}()
mvec, err := s.backup.GetVector(ctx, req.GetId().GetId())
if err == nil && mvec != nil {
return &payload.Object_Vector{
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 🐶
XXX_NoUnkeyedLiteral, XXX_unrecognized, XXX_sizecache are missing in Object_Vector (exhaustivestruct)

@@ -37,3 +40,16 @@ type (
ResourceInfo = errdetails.ResourceInfo
RetryInfo = errdetails.RetryInfo
)

const (
ValdResourceOwner = "vdaas.org vald team <vald@vdaas.org>"
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 const ValdResourceOwner should have comment (or a comment on this block) or be unexported (golint)

ValdGRPCResourceTypePrefix = "github.com/vdaas/vald/apis/grpc/v1"
)

func Serialize(obj interface{}) string {
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 Serialize should have comment or be unexported (golint)

}
if data != nil {
eg.Go(safety.RecoverWithoutPanicFunc(func() (err error) {
ctx, sspan := trace.StartSpan(ctx, fmt.Sprintf("%s/BidirectionalStream/stream-%020d", apiName, atomic.AddUint64(&cnt, 1)))
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 🐶
line is 127 characters (lll)

Signed-off-by: kpango <kpango@vdaas.org>
if span != nil {
span.SetStatus(trace.StatusCodeNotFound(err.Error()))
}
return nil, status.WrapWithNotFound(fmt.Sprintf("Exists API meta %s's uuid not found", meta.GetId()), err, meta.GetId(), info.Get())
return nil, 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 statements should not be cuddled if block has more than two lines (wsl)

if span != nil {
span.SetStatus(trace.StatusCodeInternal(err.Error()))
}
log.Error(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 🐶
expressions should not be cuddled with blocks (wsl)

if span != nil {
span.SetStatus(trace.StatusCodeInternal(err.Error()))
}
log.Error(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 🐶
expressions should not be cuddled with blocks (wsl)

}
return nil, err
}
ids = append(ids, meta)
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 🐶
append only allowed to cuddle with appended value (wsl)

@@ -84,26 +98,39 @@ func BidirectionalStream(ctx context.Context, stream ServerStream,
data := newData()
err = stream.RecvMsg(data)
if err != nil {
if err == io.EOF || errors.Is(err, io.EOF) {
return finalize()
if err != io.EOF && !errors.Is(err, io.EOF) {
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 🐶
comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)

Signed-off-by: kpango <kpango@vdaas.org>
Signed-off-by: kpango <kpango@vdaas.org>
}
return loc, nil
}

func (s *server) StreamInsert(stream vald.Insert_StreamInsertServer) error {
func (s *server) StreamInsert(stream vald.Insert_StreamInsertServer) (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 🐶
635-683 lines are duplicate of pkg/gateway/meta/handler/grpc/handler.go:911-959 (dupl)

}
return res, nil
}

func (s *server) StreamUpdate(stream vald.Update_StreamUpdateServer) error {
func (s *server) StreamUpdate(stream vald.Update_StreamUpdateServer) (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 🐶
911-959 lines are duplicate of pkg/gateway/meta/handler/grpc/handler.go:1118-1166 (dupl)

}
return loc, nil
}

func (s *server) StreamUpsert(stream vald.Upsert_StreamUpsertServer) error {
func (s *server) StreamUpsert(stream vald.Upsert_StreamUpsertServer) (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 🐶
1118-1166 lines are duplicate of pkg/gateway/meta/handler/grpc/handler.go:1371-1419 (dupl)

}
return loc, nil
}

func (s *server) StreamRemove(stream vald.Remove_StreamRemoveServer) error {
func (s *server) StreamRemove(stream vald.Remove_StreamRemoveServer) (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 🐶
1371-1419 lines are duplicate of pkg/gateway/meta/handler/grpc/handler.go:1546-1594 (dupl)

}
vec.Id = meta
return vec, nil
}

func (s *server) StreamGetObject(stream vald.Object_StreamGetObjectServer) error {
func (s *server) StreamGetObject(stream vald.Object_StreamGetObjectServer) (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 🐶
1546-1594 lines are duplicate of pkg/gateway/meta/handler/grpc/handler.go:635-683 (dupl)

}
return loc, nil
}

func (s *server) StreamUpsert(stream vald.Upsert_StreamUpsertServer) error {
func (s *server) StreamUpsert(stream vald.Upsert_StreamUpsertServer) (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 🐶
1352-1400 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:1611-1659 (dupl)

}
return locs, nil
}

func (s *server) StreamRemove(stream vald.Remove_StreamRemoveServer) error {
func (s *server) StreamRemove(stream vald.Remove_StreamRemoveServer) (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 🐶
1611-1659 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:1796-1844 (dupl)

}
return vec, nil
}

func (s *server) StreamGetObject(stream vald.Object_StreamGetObjectServer) error {
func (s *server) StreamGetObject(stream vald.Object_StreamGetObjectServer) (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 🐶
1796-1844 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:819-867 (dupl)

return nil, err
}

if !req.GetConfig().GetSkipStrictExistCheck() {
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 🐶
1019-1045 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:1187-1213 (dupl)

return nil, err
}
uuid := vec.GetVector().GetId()
if !vec.GetConfig().GetSkipStrictExistCheck() {
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 🐶
1187-1213 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:1019-1045 (dupl)

Signed-off-by: kpango <kpango@vdaas.org>
Signed-off-by: kpango <kpango@vdaas.org>
@kpango kpango force-pushed the release/v1/v1.0.0-stable-release branch from 4370141 to 02850eb Compare February 2, 2021 04:45
"github.com/gogo/protobuf/types"
)

type Any = types.Any
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 type Any should have comment or be unexported (golint)


type Any = types.Any

func UnmarshalAny(any *Any, pb proto.Message) 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 UnmarshalAny should have comment or be unexported (golint)

return fmt.Sprintf("code: %d,\tmessage: %s,\terror: %v,details: %v", e.Code, e.Message, e.Error, e.Details)
}

func DecodeErrorDetails(objs ...interface{}) (es []*ErrorDetails) {
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 🐶
Function 'DecodeErrorDetails' has too many statements (47 > 40) (funlen)

return string(b)
}

func DecodeDetail(detail *types.Any) (data interface{}, 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 🐶
Function 'DecodeDetail' has too many statements (58 > 40) (funlen)

)

var (
debugInfoMessageName = new(DebugInfo).XXX_MessageName()
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 🐶
debugInfoMessageName is a global variable (gochecknoglobals)

preconditionFailureViolationMessageName = new(PreconditionFailureViolation).XXX_MessageName()
helpMessageName = new(Help).XXX_MessageName()
helpLinkMessageName = new(HelpLink).XXX_MessageName()
quotaFailureMessageName = new(QuotaFailure).XXX_MessageName()
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 🐶
quotaFailureMessageName is a global variable (gochecknoglobals)

helpMessageName = new(Help).XXX_MessageName()
helpLinkMessageName = new(HelpLink).XXX_MessageName()
quotaFailureMessageName = new(QuotaFailure).XXX_MessageName()
quotaFailureViolationMessageName = new(QuotaFailureViolation).XXX_MessageName()
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 🐶
quotaFailureViolationMessageName is a global variable (gochecknoglobals)

helpLinkMessageName = new(HelpLink).XXX_MessageName()
quotaFailureMessageName = new(QuotaFailure).XXX_MessageName()
quotaFailureViolationMessageName = new(QuotaFailureViolation).XXX_MessageName()
requestInfoMessageName = new(RequestInfo).XXX_MessageName()
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 🐶
requestInfoMessageName is a global variable (gochecknoglobals)

quotaFailureMessageName = new(QuotaFailure).XXX_MessageName()
quotaFailureViolationMessageName = new(QuotaFailureViolation).XXX_MessageName()
requestInfoMessageName = new(RequestInfo).XXX_MessageName()
resourceInfoMessageName = new(ResourceInfo).XXX_MessageName()
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 🐶
resourceInfoMessageName is a global variable (gochecknoglobals)

quotaFailureViolationMessageName = new(QuotaFailureViolation).XXX_MessageName()
requestInfoMessageName = new(RequestInfo).XXX_MessageName()
resourceInfoMessageName = new(ResourceInfo).XXX_MessageName()
retryInfoMessageName = new(RetryInfo).XXX_MessageName()
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 🐶
retryInfoMessageName is a global variable (gochecknoglobals)

@kpango kpango merged commit 660684a into master Feb 2, 2021
@kpango kpango deleted the release/v1/v1.0.0-stable-release branch February 2, 2021 05:24
@vdaas-ci vdaas-ci mentioned this pull request Feb 2, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants