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

DistributeLock API: standardize err msgs #7631

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

moonorange
Copy link

@moonorange moonorange commented Mar 15, 2024

Description

This is the code to standardize many of the DistributedLock api errors to use the newly merged kit pkg for standardizing errors around the richer error model based on this proposal: https://github.com/dapr/proposals/blob/main/0009-BCIRS-error-handling-codes.md.

Issue reference

#7617

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • Integration tests passing
  • End-to-end tests passing
  • Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
  • Specification has been updated / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
  • Provided sample for the feature / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

moonorange and others added 2 commits March 15, 2024 19:34
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
@moonorange moonorange force-pushed the feat-standardize-err-msgs-lock-api branch from 72bbe4a to 496f4ae Compare March 15, 2024 14:26
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
@moonorange moonorange force-pushed the feat-standardize-err-msgs-lock-api branch from fae465a to a3328e2 Compare March 19, 2024 07:07
@moonorange
Copy link
Author

moonorange commented Mar 19, 2024

Hi @cicoyle, I'd appreciate early feedback to ensure I'm on the right track. I've implemented new lock error models, replaced existing errors with them, and fixed unit tests.

I believe the next steps involve implementing integration tests to validate the newly created errors
However, I'm encountering difficulty in composing them, as I'm unable to locate a pluggable lock component interface similar to the one found for pubsub at the following links:

https://github.com/moonorange/dapr/blob/a3328e2752b7114defd897af39d1206a1312f366/tests/integration/framework/process/pubsub/component.go#L38-L45

https://github.com/moonorange/dapr/blob/a3328e2752b7114defd897af39d1206a1312f366/pkg/proto/components/v1/pubsub.pb.go#L202-L207

Could I please receive guidance on how to proceed with this task?


if a.compStore.LocksLen() == 0 {
err = messages.ErrLockStoresNotConfigured
err = apiErrors.NotConfigured(req.GetStoreName(), lockType+" store", nil, codes.FailedPrecondition, http.StatusInternalServerError, "ERR_LOCK_STORE_NOT_CONFIGURED", kitErrors.CodePrefixLock+kitErrors.CodeNotConfigured)
Copy link
Author

Choose a reason for hiding this comment

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

The component type should be "lock," but in this context, "lock store" is more appropriate. so, I included the word "store."

a.logger.Debug(err)
return nil, err
}

// 2. find lock component
store, ok := a.compStore.GetLock(req.GetStoreName())
if !ok {
err = messages.ErrLockStoreNotFound.WithFormat(req.GetStoreName())
err = apiErrors.NotFound(req.GetStoreName(), lockType+" store", nil, codes.InvalidArgument, http.StatusNotFound, "ERR_LOCK_STORE_NOT_FOUND", kitErrors.CodePrefixLock+kitErrors.CodeNotFound)
Copy link
Author

Choose a reason for hiding this comment

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

Ditto

msg,
"ERR_RESOURCE_ID_EMPTY",
).
WithErrorInfo(kitErrors.CodePrefixLock+PostFixIDEmpty, nil).
Copy link
Author

Choose a reason for hiding this comment

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

I'm uncertain about how to retrieve metadata from "TryLockRequest", so I've set it to nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

There aught to be some metadata that a user will want. Upon a quick glance in that file where you are defining the error, it looks like req has:

	req.GetStoreName()
	req.GetResourceId()
	req.GetLockOwner()

You could populate metadata, a map[string]string and populate some of those fields.

@cicoyle
Copy link
Contributor

cicoyle commented Mar 19, 2024

Hi @cicoyle, I'd appreciate early feedback to ensure I'm on the right track. I've implemented new lock error models, replaced existing errors with them, and fixed unit tests.

I believe the next steps involve implementing integration tests to validate the newly created errors However, I'm encountering difficulty in composing them, as I'm unable to locate a pluggable lock component interface similar to the one found for pubsub at the following links:

https://github.com/moonorange/dapr/blob/a3328e2752b7114defd897af39d1206a1312f366/tests/integration/framework/process/pubsub/component.go#L38-L45

https://github.com/moonorange/dapr/blob/a3328e2752b7114defd897af39d1206a1312f366/pkg/proto/components/v1/pubsub.pb.go#L202-L207

Could I please receive guidance on how to proceed with this task?

You are correct, integration tests are next, and your file structure/funcs look to be on the right track. You might not need a pluggable component to trigger your errs, but it looks like you could use the redis lock component for this api. Basically, you'll need a pluggable component to force the error if its not a super easy one to trigger in the code path without some tweaking. For pubsub, I used the pluggable component to force the errors, see this line where I forced the outbox err. You might need to do something similar.

Signed-off-by: moonorange <monoma632@gmail.com>
@moonorange
Copy link
Author

moonorange commented Mar 22, 2024

You are correct, integration tests are next, and your file structure/funcs look to be on the right track. You might not need a pluggable component to trigger your errs, but it looks like you could use the redis lock component for this api. Basically, you'll need a pluggable component to force the error if its not a super easy one to trigger in the code path without some tweaking. For pubsub, I used the pluggable component to force the errors, see this line where I forced the outbox err. You might need to do something similar.

@cicoyle Thank you for the pointer! I tried writing some integration tests for gRPC using redis lock component.
22e2f00

Comment on lines +54 to +58
- name: redisHost
value: localhost:6379
- name: redisPassword
value: ""
`))
Copy link
Author

Choose a reason for hiding this comment

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

The configurations were successful when tested locally, but I'm uncertain if they'll function properly in the CI environment. If they don't, I'm unsure about the correct values to assign to these parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @moonorange , do you need a Redis instance, specifically, or just any key-value store? Would the default in-memory store work? Check out these tests for example: https://github.com/dapr/dapr/blob/master/tests/integration/suite/daprd/state/grpc/errors.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, we're working with a lockstore here, not a state store. So basically, we should check if we have a default in-memory lockstore, if we do, it will have been used in integration tests already. If not, we're gonna have to create a pluggable component, as illustrated in the file above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks we only have a redis lock for now: https://github.com/dapr/components-contrib/tree/main/lock.
You can simulate one by creating your own pluggable component, and simulating the Redis behaviour.
For reference:

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshVanL, I wonder if it would make more sense to create an in-memory lock component in components-contrib? Do you see a scenario where it might be useful to users?

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshVanL what do you think about the comment above, creating an in-memory lock component?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, though it would be my preference to use a file based lock.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Josh!
@moonorange first of all, thank you once again for the great work! The next step before we can merge this PR is getting the integration tests right. As you noted yourself, this is not straightforward because we don't run Redis in the CI for integration tests, so we have to be creative!

I believe the easiest way would be to create a new file-based lock component in the components-contrib repo. In order to run in the CI without any external dependencies it needs to be file-based (so it can be shared across apps).

I think this can be a really fun exercise! You would basically have to look at the redis-based lock component and duplicate its functionality using files as locks. Once that's done, you can set your local dapr to pull from your updated components-contrib by modifying dapr/go.mod to point to it (scroll down to the line that does a go replace for components contrib, uncomment it and change it to the path to your local components-contrib repo).
And then, you can set the test component lock store to your new file-based lockstore.

Do you feel up for the challenge? :) We're always here to help out if you get stuck!

Copy link
Author

Choose a reason for hiding this comment

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

@elena-kolevska
Thank you for the pointer. I'm totally down for this challenge!

I'll start by checking out how the Redis-based lock component works and then figure out how to replicate it using files. If I hit any bumps, I'll definitely reach out for a hand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing! I'm happy to hear that.
It would be great if we could get this in the next release (1.14). Code freeze for it is the 11th of June.

moonorange and others added 11 commits March 26, 2024 18:25
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
@moonorange moonorange marked this pull request as ready for review March 26, 2024 14:01
@moonorange moonorange requested review from a team as code owners March 26, 2024 14:01
@moonorange moonorange marked this pull request as draft March 26, 2024 14:06
moonorange and others added 5 commits March 27, 2024 15:28
…r models

Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
…range/dapr into feat-standardize-err-msgs-lock-api
Signed-off-by: moonorange <monoma632@gmail.com>
@moonorange moonorange marked this pull request as ready for review March 30, 2024 05:43
moonorange and others added 3 commits March 30, 2024 17:09
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
@moonorange
Copy link
Author

@cicoyle Hi, could I get a review for this PR?

Signed-off-by: moonorange <monoma632@gmail.com>
@yaron2
Copy link
Member

yaron2 commented Apr 3, 2024

cc @cicoyle

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 61.81%. Comparing base (091a204) to head (586cfc1).
Report is 5 commits behind head on master.

Current head 586cfc1 differs from pull request most recent head 67bef77

Please upload reports for the commit 67bef77 to get more accurate results.

Files Patch % Lines
pkg/api/universal/lock.go 0.00% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7631      +/-   ##
==========================================
+ Coverage   61.39%   61.81%   +0.42%     
==========================================
  Files         265      245      -20     
  Lines       22609    22425     -184     
==========================================
- Hits        13880    13863      -17     
+ Misses       7579     7400     -179     
- Partials     1150     1162      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@cicoyle cicoyle left a comment

Choose a reason for hiding this comment

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

Really awesome job on this! 🚀 Minor tweaks are needed to ensure we don't break users relying on the legacy tags and http/grpc codes. Make sure to update the tests once you update those values.

@@ -0,0 +1,95 @@
/*
Copyright 2022 The Dapr Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2022 The Dapr Authors
Copyright 2024 The Dapr Authors

Looks to be off to a good start 🚀

http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implieh.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implieh.
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

Copy link
Member

Choose a reason for hiding this comment

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

I think the origin one should be fix as well, we need a right license.

@cicoyle could you help check it?

@@ -0,0 +1,19 @@
/*
Copyright 2023 The Dapr Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2023 The Dapr Authors
Copyright 2024 The Dapr Authors

@@ -0,0 +1,38 @@
/*
Copyright 2023 The Dapr Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright 2023 The Dapr Authors
Copyright 2024 The Dapr Authors

http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implieh.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implieh.
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

const (
ErrInfoType = "type.googleapis.com/google.rpc.ErrorInfo"
ResourceInfoType = "type.googleapis.com/google.rpc.ResourceInfo"
BadRequestType = "type.googleapis.com/google.rpc.BadRequest"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being used?

ErrInfoType = "type.googleapis.com/google.rpc.ErrorInfo"
ResourceInfoType = "type.googleapis.com/google.rpc.ResourceInfo"
BadRequestType = "type.googleapis.com/google.rpc.BadRequest"
HelpType = "type.googleapis.com/google.rpc.Help"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here - Is this being used?

ResourceInfoType = "type.googleapis.com/google.rpc.ResourceInfo"
BadRequestType = "type.googleapis.com/google.rpc.BadRequest"
HelpType = "type.googleapis.com/google.rpc.Help"
LockStoreName = "lockstore"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this inside the Run(), but before & outside all the t.Runs().

require.NoError(t, err)

require.Equal(t, "application/json", resp.Header.Get("Content-Type"))
require.Equal(t, http.StatusNotFound, resp.StatusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

In looking at the predefined.go I believe this should be the legacy http code of: http.StatusBadRequest. By changing this it might break existing users, we want to keep the logic as is, even if it's not correct. You can add a // TODO: change this to be http.StatusNotFound in 2 releases since it would be a breaking change to the errors/lock.go

l.skipResourceInfo = true
return l.build(
codes.InvalidArgument,
http.StatusNotFound,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
http.StatusNotFound,
http.StatusBadRequest, // TODO: change in 2 releases to http.StatusNotFound

moonorange and others added 7 commits April 5, 2024 21:03
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Signed-off-by: moonorange <monoma632@gmail.com>
Copy link
Author

@moonorange moonorange left a comment

Choose a reason for hiding this comment

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

@cicoyle Thank you for the thorough review!
I have addressed all the reviewed points.

I'm still encountering an issue with setting up Redis in the integration test on the CI environment.
Any guidance on this would be greatly appreciated.
https://github.com/dapr/dapr/pull/7631/files#r1535379267

Here's what I believe the necessary steps are:

Set up Redis in .github/workflows/dapr.yml, similar to .github/workflows/kind-e2e.yaml.
Access Redis on the integration CI environment using this host and port: dapr-redis-master.dapr-tests.svc.cluster.local:6379.

Change dapr.yml to something like this

integration-tests:
        name: Integration tests
        needs: lint
        runs-on: '${{ matrix.os }}'
        strategy:
            fail-fast: false
            matrix:
                include:
                    - os: ubuntu-latest
                      target_os: linux
                      target_arch: amd64
                    - os: windows-2022
                      target_os: windows
                      target_arch: amd64
                      windows_version: ltsc2022
                    - os: macOS-latest
                      target_os: darwin
                      target_arch: amd64
        env:
            GOOS: '${{ matrix.target_os }}'
            GOARCH: '${{ matrix.target_arch }}'
            GOPROXY: 'https://proxy.golang.org'
            TEST_OUTPUT_FILE_PREFIX: 'test_report'
        steps:
            - name: Check out code into the Go module directory
              uses: actions/checkout@v4
            - name: Set up Go
              id: setup-go
              uses: actions/setup-go@v5
              with:
                  go-version-file: 'go.mod'
            - name: Setup Redis # Add this step to setup redis on integration-test
              run: |
                  make setup-test-env-redis
            - name: Override DAPR_HOST_IP for MacOS
              if: matrix.target_os == 'darwin'
              run: |
                  echo "DAPR_HOST_IP=127.0.0.1" >>${GITHUB_ENV}
            - name: Run make test-integration-parallel
              run: make test-integration-parallel

Then change the redis host to this

dapr-redis-master.dapr-tests.svc.cluster.local:6379

@@ -2499,6 +2499,7 @@ func TestV1Alpha1DistributedLock(t *testing.T) {

// assert
assert.Nil(t, resp.JSONBody)
assert.Contains(t, string(resp.RawBody), "ERR_MALFORMED_REQUEST")
Copy link
Author

Choose a reason for hiding this comment

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

Add this line to ensure that I don't make any breaking changes to the error tag.

@@ -2517,6 +2518,7 @@ func TestV1Alpha1DistributedLock(t *testing.T) {

// assert
assert.Nil(t, resp.JSONBody)
assert.Contains(t, string(resp.RawBody), "ERR_MALFORMED_REQUEST")
Copy link
Author

Choose a reason for hiding this comment

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

same here

@@ -2534,6 +2536,7 @@ func TestV1Alpha1DistributedLock(t *testing.T) {

// assert
assert.Nil(t, resp.JSONBody)
assert.Contains(t, string(resp.RawBody), "ERR_MALFORMED_REQUEST")
Copy link
Author

Choose a reason for hiding this comment

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

same

@moonorange moonorange mentioned this pull request May 20, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants