Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

bugfix: CDN == "source" does not request pieces from source #1501

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

m0nstermind
Copy link

by dfget having suprnodes was configured with explicily defined port

Ⅰ. Describe what this PR did

When supernode listens on non default port ( e.g. other than 80 for http ) the CDN Pattern: source ( to request pieces from source url ) does not work. dfget try to load pieces for the first time from supernode and fail, because supernode does not have them.

The source of this is in comparation performed by dfget at power_client.go, which compares piece destination ip, received by downloader to the url of supernode PowerClient is initialized with ( which comes in form ip:port ). In case of default port value, it is omitted from configration and this check passes and always fails for non default port values.
The PR fixes this to compare ip to ip.

Ⅱ. Does this pull request fix one issue?

none

Ⅲ. Why don't you add test cases (unit test/integration test)?

The fix is trivial

Ⅳ. Describe how to verify it

Prepare supernode.yaml with

base:
  cdnPattern: source
  listenPort: 5002

run supernode without running nginx

run dfget with
dfget --node 127.0.0.1:5002 http://source/url/to/get

dfget will fail to download any pieces from source url, trying to get it from 127.0.0.1:8001 ( the default download port of supernode )

Ⅴ. Special notes for reviews

none

@pouchrobot
Copy link
Collaborator

We found this is your first time to contribute to Dragonfly, @m0nstermind
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/dragonflyoss/Dragonfly/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

…rnode was configured with explicily defined port

Signed-off-by: Oleg Anastasyev <oa@odnoklassniki.ru>
@jim3ma
Copy link
Member

jim3ma commented Nov 3, 2020

Please sign your commit with git commit -s --amend, then force push it.
Thanks

@m0nstermind
Copy link
Author

BTW unit test failed with
image_preheater_test.go:38: Get https://registry.cn-zhangjiakou.aliyuncs.com/v2/acs/alpine/manifests/3.6: net/http: TLS handshake timeout

which, i believe is not about this PR, but due to connectivity problems with registry.cn-zhangjiakou.aliyuncs.com

@starnop
Copy link
Contributor

starnop commented Dec 11, 2020

LGTM.
also cc/ @jim3ma

@starnop
Copy link
Contributor

starnop commented Dec 11, 2020

BTW unit test failed with
image_preheater_test.go:38: Get https://registry.cn-zhangjiakou.aliyuncs.com/v2/acs/alpine/manifests/3.6: net/http: TLS handshake timeout

which, i believe is not about this PR, but due to connectivity problems with registry.cn-zhangjiakou.aliyuncs.com

I have rerun the CI.

@codecov-io
Copy link

Codecov Report

Merging #1501 (9c4b244) into master (0639b49) will decrease coverage by 1.45%.
The diff coverage is 46.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   50.76%   49.31%   -1.46%     
==========================================
  Files         145      145              
  Lines        9557     8381    -1176     
==========================================
- Hits         4852     4133     -719     
+ Misses       4309     3861     -448     
+ Partials      396      387       -9     
Impacted Files Coverage Δ
...et/core/downloader/p2p_downloader/client_writer.go 8.47% <0.00%> (-0.06%) ⬇️
...t/core/downloader/p2p_downloader/p2p_downloader.go 1.85% <0.00%> (-0.24%) ⬇️
supernode/daemon/mgr/preheat/file_preaheater.go 2.43% <ø> (-3.69%) ⬇️
supernode/daemon/mgr/preheat/preheat_service.go 0.00% <0.00%> (ø)
supernode/daemon/mgr/cdn/manager.go 22.55% <75.00%> (+1.69%) ⬆️
...get/core/downloader/p2p_downloader/power_client.go 53.12% <100.00%> (+0.29%) ⬆️
supernode/daemon/mgr/task/manager.go 29.26% <100.00%> (+0.69%) ⬆️
supernode/server/metrics.go 70.31% <100.00%> (+0.16%) ⬆️
supernode/store/store.go 44.44% <0.00%> (-9.73%) ⬇️
supernode/daemon/mgr/cdn/cdn_util.go 37.50% <0.00%> (-8.66%) ⬇️
... and 143 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 731059a...9c4b244. Read the comment docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants