components-contrib icon indicating copy to clipboard operation
components-contrib copied to clipboard

feat: file based naming resolver

Open beiwei30 opened this issue 3 years ago • 15 comments

Description

Provide a file based naming resolver to replace mDNS so that dapr can address each other either on the same host or via shared file system, which would be more stable and friendly for development.

The default directory to keep naming informations is: $HOME/.dapr/naming, and it can be customized in Dapr's config.yaml:

apiVersion: dapr.io/v1alpha1
kind: Configuration
metadata:
  name: appconfig
spec:
  nameResolution:
    component: "file"
    configuration:
      dir: "/var/tmp/dapr-naming"

Pro and con of this solution:

  • Pro: Since the name resolving is file based, it means that it can not only work in slim mode, but also in standalone mode, since the directory to keep the naming informations can be anywhere as long as it's accessible to Dapr's instances.
  • Con: Since there has no lifecycle mechanism for components, this solution doesn't have chance to remove (unregister) its naming info from the file when Dapr instance goes away. It may not a big deal in development stage, but in the worst case user needs to know where the directory locates and delete the files manually.

Issue reference

This PR will close: dapr/dapr#3256

Checklist

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

  • [x] Code compiles correctly
  • [x] Created/updated tests
  • [ ] Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

beiwei30 avatar Dec 14 '21 07:12 beiwei30

@daixiang0 can you please review this?

yaron2 avatar Dec 16 '21 17:12 yaron2

@yaron2 sure.

daixiang0 avatar Dec 17 '21 00:12 daixiang0

ping @beiwei30

yaron2 avatar Dec 27 '21 05:12 yaron2

ping @beiwei30

sorry for late response, will follow up the review comments today.

beiwei30 avatar Jan 12 '22 06:01 beiwei30

@daixiang0 @artursouza I have updated this PR, pls. take a look again.

beiwei30 avatar Jan 12 '22 09:01 beiwei30

@beiwei30 Please resolve the conflicts.

Taction avatar Feb 23 '22 14:02 Taction

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

dapr-bot avatar Mar 25 '22 14:03 dapr-bot

keepalive

yaron2 avatar Mar 25 '22 17:03 yaron2

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

dapr-bot avatar Apr 24 '22 17:04 dapr-bot

@yaron2 @daixiang0 would you mind to review it again? I resolved the conflict today.

beiwei30 avatar Apr 25 '22 07:04 beiwei30

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

dapr-bot avatar Jul 07 '22 06:07 dapr-bot

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

dapr-bot avatar Jul 14 '22 06:07 dapr-bot

@beiwei30 can you please run go mod tidy and push the changes? Thanks!

yaron2 avatar Aug 10 '22 17:08 yaron2

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

dapr-bot avatar Sep 22 '22 04:09 dapr-bot

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

dapr-bot avatar Sep 29 '22 04:09 dapr-bot

Codecov Report

Merging #1380 (a12728c) into master (a69b9b9) will decrease coverage by 1.04%. The diff coverage is 59.16%.

:exclamation: Current head a12728c differs from pull request most recent head e50d2b6. Consider uploading reports for the commit e50d2b6 to get more accurate results

@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
- Coverage   37.62%   36.57%   -1.05%     
==========================================
  Files         192      167      -25     
  Lines       23982    15640    -8342     
==========================================
- Hits         9023     5721    -3302     
+ Misses      14193     9272    -4921     
+ Partials      766      647     -119     
Impacted Files Coverage Δ
nameresolution/file/file.go 59.16% <59.16%> (ø)
metadata/duration.go 0.00% <0.00%> (-59.62%) :arrow_down:
bindings/zeebe/command/deploy_process.go 12.50% <0.00%> (-11.31%) :arrow_down:
bindings/alicloud/rocketmq/settings.go 22.22% <0.00%> (-9.78%) :arrow_down:
bindings/azure/cosmosdb/cosmosdb.go 18.00% <0.00%> (-7.69%) :arrow_down:
bindings/alicloud/nacos/settings.go 33.33% <0.00%> (-6.67%) :arrow_down:
bindings/alicloud/dingtalk/webhook/settings.go 33.33% <0.00%> (-6.67%) :arrow_down:
bindings/mqtt/mqtt.go 25.00% <0.00%> (-5.64%) :arrow_down:
bindings/azure/blobstorage/blobstorage.go 15.09% <0.00%> (-3.81%) :arrow_down:
lock/redis/standalone.go 49.33% <0.00%> (-3.66%) :arrow_down:
... and 191 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Nov 17 '22 20:11 codecov[bot]

@beiwei30 - When the app becomes unhealthy, there is lot of manual activity involved -

  1. User should manually delete the name resolution file and lock file.
  2. All the app instances should be restarted to create a new file with updated name-resolution information for all app instances.[Since we have a common name resolution information for all apps with same id]

Also having 777 file permissions allows anyone to write to the nameresolution file which is a security issue, since the consuming app can be misguided to connect to any endpoint.

To complete this solution, there should be a cleanup of the nameresolution information. when file is chosen as name resolution - dapr cli can periodically check if all the apps as running and clean up the nameresolution information to be accurate - thoughts? @mukundansundar @berndverst

akhilac1 avatar Dec 12 '22 11:12 akhilac1

@beiwei30 - When the app becomes unhealthy, there is lot of manual activity involved -

  1. User should manually delete the name resolution file and lock file.
  2. All the app instances should be restarted to create a new file with updated name-resolution information for all app instances.[Since we have a common name resolution information for all apps with same id]

Also having 777 file permissions allows anyone to write to the nameresolution file which is a security issue, since the consuming app can be misguided to connect to any endpoint.

To complete this solution, there should be a cleanup of the nameresolution information. when file is chosen as name resolution - dapr cli can periodically check if all the apps as running and clean up the nameresolution information to be accurate - thoughts? @mukundansundar @berndverst

I think we need someone to create a design document for the overall design / experience including the CLI, and the various edge cases before we should proceed here.

This feature is useful, but there seem to be plenty of gaps.

berndverst avatar Sep 08 '23 21:09 berndverst

@berndverst and I synced yesterday, and I agree a file-based nameresolver should happen in 1.13, but IMHO this should be based on SQLIte.

Using a single SQLite database will already solve problems such as concurrent access by multiple sidecars (locking, etc). A single file is also easier to handle and to garbage-collect. Lastly, SQLite is also generally faster than the filesystem for these kinds of things.

ItalyPaleAle avatar Sep 12 '23 17:09 ItalyPaleAle