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

Increase Sentry retry to 30s from Sidecar Injector #7508

Merged
merged 1 commit into from Feb 6, 2024

Conversation

yaron2
Copy link
Member

@yaron2 yaron2 commented Feb 6, 2024

Follow up to #7507

Increases retries to 30s which is the maximum mutation request allowed duration when the K8s API server calls an admission webhook.

Also moves connection init to New to encapsulate the logic in the requester.

Signed-off-by: yaron2 <schneider.yaron@live.com>
@yaron2 yaron2 requested review from a team as code owners February 6, 2024 21:07
@@ -109,7 +111,7 @@ func (r *Requester) RequestCertificateFromSentry(ctx context.Context, namespace
Token: token,
Namespace: namespace,
TokenValidator: tokenValidator,
}, grpcRetry.WithMax(3), grpcRetry.WithPerRetryTimeout(time.Second*3))
}, grpcRetry.WithMax(10), grpcRetry.WithPerRetryTimeout(time.Second*3))
Copy link
Contributor

Choose a reason for hiding this comment

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

Then do we need to set a "WithMax"? Could we just abort on when the context was canceled, which happens automatically?

Suggested change
}, grpcRetry.WithMax(10), grpcRetry.WithPerRetryTimeout(time.Second*3))
}, grpcRetry.WithPerRetryTimeout(2*time.Second))

Copy link
Member Author

Choose a reason for hiding this comment

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

WithMax is required to enable retries. Only having WithPerRetryTimeout will not enable it. Context cancellation will not retry the request, it will serve as a 30s timeout for a single request.

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (afcf9d5) 62.17% compared to head (aa16085) 62.07%.

Additional details and impacted files
@@               Coverage Diff                @@
##           release-1.13    #7508      +/-   ##
================================================
- Coverage         62.17%   62.07%   -0.10%     
================================================
  Files               245      245              
  Lines             22336    22336              
================================================
- Hits              13887    13865      -22     
- Misses             7293     7316      +23     
+ Partials           1156     1155       -1     

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

@yaron2 yaron2 merged commit 281068b into dapr:release-1.13 Feb 6, 2024
31 of 32 checks passed
@JoshVanL JoshVanL added this to the v1.13 milestone Feb 12, 2024
JoshVanL pushed a commit to JoshVanL/dapr that referenced this pull request Mar 6, 2024
Signed-off-by: yaron2 <schneider.yaron@live.com>
yaron2 added a commit that referenced this pull request Mar 6, 2024
* Make injector resilient to sentry unavailability (#7507)

* make injector resilient to sentry unavailability

Signed-off-by: yaron2 <schneider.yaron@live.com>

* remove redundant line

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* sentry retry up to 30s (#7508)

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Update contrib to 1.13.0-rc.3 (#7509)

* update contrib to 1.13.0-rc.2

Signed-off-by: yaron2 <schneider.yaron@live.com>

* update to rc.3

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Injector: add option to add `DAPR_HOST_IP` env var to daprd (#7511)

The `DAPR_HOST_IP` env var is used in various places in Dapr for a sidecar to know its own IP address, for example for service invocation or actor invocation.

When using the Dapr injector to add the daprd container, we can use the downstream APIs to add the `DAPR_HOST_IP` env var based on data from the controller

This option can be enabled by setting the Helm option `dapr_sidecar_injector.enableK8sDownwardAPIs=true`

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Fix state encryption regression + add integration test (#7517)

* fix state encryption regression + add intg test

Signed-off-by: yaron2 <schneider.yaron@live.com>

* linter

Signed-off-by: yaron2 <schneider.yaron@live.com>

* review feedback

Signed-off-by: yaron2 <schneider.yaron@live.com>

* linter

Signed-off-by: yaron2 <schneider.yaron@live.com>

* just use sha256, remove nolint:gosec

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Test Integration: speed up tests 10% (#7528)

* Test Integration: speed up tests 10%

Speed up integration tests by changing poll intervals
`100*time.Millisecond` to `10*time.Millisecond`.

~4.50m to ~4.20m

Signed-off-by: joshvanl <me@joshvanl.dev>

* Wait for operator healthz before exiting to ensure no exit error

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>

* Revert selfhosted disk loader to not respect namespace (#7527)

* Revert selfhosted disk loader to not respect namespace

Revert the selfhosted component disk loader to not respect the
namespace. This will allow the selfhosted disk loader to load components
from any namespace from file.

Fixes #7523

Signed-off-by: joshvanl <me@joshvanl.dev>

* Revert component disk loader behaviour to respect component namespace
when NAMESPACE env var is set.

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>

* [1.13] Add warning that Dapr state store encryption could lead to catastrophic failures (#7524)

* [1.13] Add warning that Dapr state store encryption could lead to catastrophic failures

If the same key is used to encrypt more than 2^32 values (ie. more than 2^32 "Save" operations), it can lead to the private keys being exposed.

See: #6027
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Changed per review feedback

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

---------

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>

* Add content-length to http channel (#7537)

* add content-length to http channel

Signed-off-by: yaron2 <schneider.yaron@live.com>

* update tests

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Updates components-contrib to 1.13.0-rc.4 (#7538)

* Updates components-contrib to 1.13.0-rc.3

Signed-off-by: joshvanl <me@joshvanl.dev>

* mod-tidy-all

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>

* Subscriptions: Fix panic when match rule is empty (#7539)

Prevents a panic resulting when a subscription route rule match
(`spec.routes.rules.match`) is empty. An empty match rule is valid and
is considered "default". Error occurs because of Go interface vs
implementation struct pointer nil check foot-gun.

Adds integration tests for both HTTP and gRPC subscribers.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Hot Reloading: don't watch files if not enabled (#7521)

* Hot Reloading: don't watch files if not enabled

Update hot reloading so that we don't setup file watchers on the
component directory if hot reloading is not enabled.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove `Close` paradigm from hot reloader

Signed-off-by: joshvanl <me@joshvanl.dev>

* Linting

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>

* Remove pubsub content-length test which has been removed from (#7550)

release-1.13

Signed-off-by: joshvanl <me@joshvanl.dev>

* Revert ApiLevel controlling vnodes back to context metadata (#7547)

* Revert ApiLevel controlling vnodes back to context metadata

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Lint

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Fixes after review

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Set back api level to 10, just for completeness purposes

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Don’t specify APILevelSpecify api level

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Apply suggestions from code review

Co-authored-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>

* Cleanup after review

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Small refactor

Signed-off-by: Elena Kolevska <elena@kolevska.com>

* Refactor multiple parameters into a request object for placement table dissemination

Signed-off-by: Elena Kolevska <elena@kolevska.com>

---------

Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Co-authored-by: Josh van Leeuwen <me@joshvanl.dev>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>

* Actor Reminders: Default JSON serialization. (#7548)

* Actor Reminders: Default JSON serialization.

To support downgrades to 1.12 from 1.13, this PR changes the reminder
serialization storage format back to JSON by default. This means a 1.12
actor reminder client can read reminders written by 1.13 actors.

1.13 will continue to understand both JSON and protobuf. Protobuf
serialization can be enabled with the `ActorReminderStorageProtobuf`
feature gate. The actor "API Level" has been changed back to 10.

Adds test to ensure the default serialization is JSON.

Signed-off-by: joshvanl <me@joshvanl.dev>

* Remove ActorReminderStorageProtobuf feature gate in favour of using API
level

Signed-off-by: joshvanl <me@joshvanl.dev>

* Fix api level tests

Signed-off-by: joshvanl <me@joshvanl.dev>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>

* Update contrib to 1.13.0-rc.6 (#7553)

* update contrib to 1.13.0-rc.5

Signed-off-by: yaron2 <schneider.yaron@live.com>

* update to rc.6

Signed-off-by: yaron2 <schneider.yaron@live.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>

* Updates components-contrib to 1.13.0-rc.7 (#7562)

Signed-off-by: joshvanl <me@joshvanl.dev>

* update contrib to 1.13.0-rc.8 (#7567)

* Add metadata in binding response even in case of error (#7572)

* Add binding metadata even in case of error.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* ADD IT.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* Fix lint.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

* [Release-1.13] Upgrade to contrib `v1.13.0-rc.10` (#7577)

* Upgrade to contrib 1.13.0-rc.9

Signed-off-by: Bernd Verst <github@bernd.dev>

* Use contrib 1.13.0-rc.10 for latest kafka sarama patch version

Signed-off-by: Bernd Verst <github@bernd.dev>

---------

Signed-off-by: Bernd Verst <github@bernd.dev>

* Fix for issue 7576 (#7581) (#7587)

Signed-off-by: Guido Spadotto <guido.spadotto@profesia.it>
Co-authored-by: Guido Spadotto <guido.spad8@gmail.com>
Co-authored-by: Guido Spadotto <guido.spadotto@profesia.it>

* Adds v1.13.0 release notes (#7586)

* Adds v1.13.0 release notes

Adds docs/release_notes/v1.13.0.md

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update docs/release_notes/v1.13.0.md

Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>
Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>

* Move Actor Reminder Performance higher in notes

Signed-off-by: joshvanl <me@joshvanl.dev>

* Update v1.13.0.md

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>

* chore: bump go to 1.21.8 and protobuf lib to 1.33.0 (#7591)

* ci: force go1.21.8 in workflows instead of go.mod

Signed-off-by: mikeee <hey@mike.ee>

* chore: bump google.golang.org/protobuf to 1.33.0

Signed-off-by: mikeee <hey@mike.ee>

* make modtidy-all

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: mikeee <hey@mike.ee>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>

---------

Signed-off-by: yaron2 <schneider.yaron@live.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: Elena Kolevska <elena@kolevska.com>
Signed-off-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Signed-off-by: Artur Souza <asouza.pro@gmail.com>
Signed-off-by: Bernd Verst <github@bernd.dev>
Signed-off-by: Guido Spadotto <guido.spadotto@profesia.it>
Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>
Signed-off-by: mikeee <hey@mike.ee>
Co-authored-by: Yaron Schneider <schneider.yaron@live.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Co-authored-by: Elena Kolevska <elena-kolevska@users.noreply.github.com>
Co-authored-by: Artur Souza <asouza.pro@gmail.com>
Co-authored-by: Bernd Verst <github@bernd.dev>
Co-authored-by: Guido Spadotto <guido.spad8@gmail.com>
Co-authored-by: Guido Spadotto <guido.spadotto@profesia.it>
Co-authored-by: Paul Yuknewicz <paulyuk@microsoft.com>
Co-authored-by: Mike Nguyen <hey@mike.ee>
JoshVanL pushed a commit to JoshVanL/dapr that referenced this pull request Mar 6, 2024
Signed-off-by: yaron2 <schneider.yaron@live.com>
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

3 participants