Open m0nstermind opened 4 years ago
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! 🍻
Please sign your commit with git commit -s --amend
, then force push it.
Thanks
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
LGTM. also cc/ @jim3ma
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.
Merging #1501 (9c4b244) into master (0639b49) will decrease coverage by
1.45%
. The diff coverage is46.66%
.
@@ 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%) |
:arrow_down: |
...t/core/downloader/p2p_downloader/p2p_downloader.go | 1.85% <0.00%> (-0.24%) |
:arrow_down: |
supernode/daemon/mgr/preheat/file_preaheater.go | 2.43% <ø> (-3.69%) |
:arrow_down: |
supernode/daemon/mgr/preheat/preheat_service.go | 0.00% <0.00%> (ø) |
|
supernode/daemon/mgr/cdn/manager.go | 22.55% <75.00%> (+1.69%) |
:arrow_up: |
...get/core/downloader/p2p_downloader/power_client.go | 53.12% <100.00%> (+0.29%) |
:arrow_up: |
supernode/daemon/mgr/task/manager.go | 29.26% <100.00%> (+0.69%) |
:arrow_up: |
supernode/server/metrics.go | 70.31% <100.00%> (+0.16%) |
:arrow_up: |
supernode/store/store.go | 44.44% <0.00%> (-9.73%) |
:arrow_down: |
supernode/daemon/mgr/cdn/cdn_util.go | 37.50% <0.00%> (-8.66%) |
:arrow_down: |
... 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.
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
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