[CLOUDSTACK-9772] Template: perform a HEAD request to check file size from a URL#1934
Conversation
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-473 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-805)
|
|
@marcaurele there are some marvin tests in extract and delete template, could you please have a look? |
| // Get the size of a file from URL response header. | ||
| public static Long getRemoteSize(String url) { | ||
| Long remoteSize = (long)0; | ||
| public static long getRemoteSize(String url) { |
There was a problem hiding this comment.
@marcaurele I think this needs to be changed back to Long, or else the method cannot be found as it is called with type Long.
There was a problem hiding this comment.
@remibergsma if you look at all the calls to getRemoteSize, they all expect a long. Moreover the result is neither null but a long, 0 or an exception, so it's best to return a long directly IMO.
|
@borisstoyanov Dunno what's wrong with the failed tests. Its' failing at https://github.com/apache/cloudstack/blob/master/plugins/storage/image/default/src/org/apache/cloudstack/storage/datastore/driver/CloudStackImageStoreDriverImpl.java#L83. The log says that the ssvm endpoint was not ready. We're running that fix in production since a week and haven't hit a problem yet. |
|
OK @marcaurele, if you're 100% sure it works as expected it could be that we need to redesign the test itself, I guess it needs deeper investigation of the failure itself. |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-815)
|
borisstoyanov
left a comment
borisstoyanov
left a comment
There was a problem hiding this comment.
LGTM, based on latests tests results and code review
|
What about signed S3 urls that people may supply? If you want to register a template and have it download from a S3 bucket, you need to specify a signed url to provide the authentication. But that signature is valid for either GET or HEAD requests, not both. When you use S3 as secondary storage, you automatically get a GET pre-signed url from CloudStack that you cannot use to register it as a new template. Something to think about.. See: http://stackoverflow.com/questions/15717230/pre-signing-amazon-s3-urls-for-both-head-and-get-verbs |
|
@remibergsma Good point, I was aware of that difference, which I think doesn't help to make systems reliable. |
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no): https://www.dropbox.com/sh/r2si930m8xxzavs/AAAzNrnoF1fC3auFrvsKo_8-a?dl=0 Failed tests:
Skipped tests: Passed test suits: |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1297 |
|
@marcaurele are you happy to further testing/merging, or you think this can break use-cases for people using URLs where HEAD may not be supported by hosting server? |
|
@rhtyd I will amend the code to support the case with S3 pre-signed URL which will potentially fail on a HEAD operation. It will try to perform a HEAD operation and if it does not work, it tries to do a normal GET. I'll ping you when it's updated. |
marcaurele
force-pushed
the
fix/CLOUDSTACK-9772
branch
5 times, most recently
from
920edd3 to
9ff4866
Compare
|
@rhtyd ready on my side with the fallback on a GET request if the HEAD fails |
|
Code LGTM With one side-note though. I still see a lot of webservers not sending Content-Length nor Content-Type (not used here) |
|
@wido ok, then I'll improve it to calculate the content size in case of a GET request based on the input stream. |
marcaurele
force-pushed
the
fix/CLOUDSTACK-9772
branch
from
9ff4866 to
493f2f6
Compare
|
@wido improvement added on |
|
Update on this? @marcaurele |
|
@rhtyd I did the code update regarding @wido comment. So now it tries to download using the HEAD request and reads the information. If that does not work, for example because the URL is a pre-signed S3 download one, it performs a GET and tries to read the content length from the header. If it is not found, it will read the length out of the downloaded content. |
| return details; | ||
| } | ||
|
|
||
| public static void main(String[] args) { |
yadvr
left a comment
yadvr
left a comment
There was a problem hiding this comment.
Left a minor to-do task, otherwise LGTM subject to testing.
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Signed-off-by: Marc-Aurèle Brothier <m@brothier.org>
marcaurele
force-pushed
the
fix/CLOUDSTACK-9772
branch
from
493f2f6 to
33ce66c
Compare
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1439 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1843)
|
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1477 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1905)
|
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1483 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1918)
|
|
Test LGTM. Merging this based on three code lgtm and test results. Thanks. |
7 participants
Signed-off-by: Marc-Aurèle Brothier m@brothier.org