fix/ota-2183/Enable streaming of garage-push/deploy uploads #1305

Zee314159 · 2019-08-20T12:17:18Z

Draft code.

codecov-io · 2019-08-20T13:47:39Z

Codecov Report

Merging #1305 into master will decrease coverage by 0.02%.
The diff coverage is 84.37%.

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
- Coverage   79.72%   79.69%   -0.03%     
==========================================
  Files         177      177              
  Lines       10475    10487      +12     
==========================================
+ Hits         8351     8358       +7     
- Misses       2124     2129       +5

Impacted Files	Coverage Δ
src/sota_tools/request_pool.h	`100% <ø> (ø)`	⬆️
src/sota_tools/treehub_server.h	`100% <ø> (ø)`	⬆️
src/sota_tools/ostree_object.h	`100% <ø> (ø)`	⬆️
src/sota_tools/request_pool.cc	`87.75% <100%> (ø)`	⬆️
src/sota_tools/treehub_server.cc	`100% <100%> (ø)`	⬆️
src/sota_tools/ostree_http_repo.h	`100% <100%> (ø)`	⬆️
src/sota_tools/check.cc	`61.42% <100%> (-2.86%)`	⬇️
src/sota_tools/garage_deploy.cc	`88.31% <25%> (-1.3%)`	⬇️
src/sota_tools/ostree_object.cc	`94.21% <84.61%> (-0.37%)`	⬇️
... and 3 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 38f8239...ee671d5. Read the comment docs.

Zee314159 · 2019-08-21T15:51:22Z

I manually tested, using ‘t_deploy’,the output looks it didn’t work. Content_type is multipart and x-www-form-urlencoded.Maybe I was using the wrong test, or the code I added didn’t work somehow.

pattivacek · 2019-08-22T07:48:34Z

Content_type is multipart and x-www-form-urlencoded.

It's possible that there is some other way you have to specify it, perhaps forcing the header value doesn't actually stick.

However, I suspect a bigger problem is that you probably don't want to set that from the RequestPool. You only want it for uploads, so OSTreeObject::Upload in src/sota_tools/ostree_object.cc is probably a safer place to do it.

mike-sul · 2019-08-23T06:39:01Z

You only want it for uploads, so OSTreeObject::Upload in src/sota_tools/ostree_object.cc

It will require updating treehub_server.py accordingly.
Also, I am wondering what does Out of band upload described in advancedtelematic/treehub#70 exactly mean and whether there is any benefit in that upload mode for us ? My understanding is that it will help to bypass one of the OTA connect internal services (treehub ?) and stream directly to S3.

pattivacek · 2019-08-23T07:38:24Z

It will require updating treehub_server.py accordingly.

Ah, I hadn't considered that. You might be right; the tests may need to get updated once we have it working correctly with the real backend.

Also, I am wondering what does Out of band upload described in advancedtelematic/treehub#70 exactly mean and whether there is any benefit in that upload mode for us ? My understanding is that it will help to bypass one of the OTA connect internal services (treehub ?) and stream directly to S3.

To be clear, the "out of band upload" is out of scope for the current task that Liping is working on. But it's still worth understanding, and we may want to support it eventually, although I think it is somewhat more work. See https://saeljira.it.here.com/browse/OTA-1813 for some additional detail. Basically, the backend could tell us to upload directly to S3 and bypass treehub completely. The advantage is reduced load on our server and fewer points of failure.

eu-siemann · 2019-08-27T16:29:26Z

src/sota_tools/ostree_object.h

@@ -111,6 +111,7 @@ class OSTreeObject {
  std::stringstream http_response_;
  CURL* curl_handle_;
  struct curl_httppost* form_post_;
+  curl_slist* headers;


We should initialize this to nullptr. Otherwise, it could be anything and will segfault in destructor. This should fix some of the failing tests in CI, I think.

eu-siemann · 2019-08-27T16:34:52Z

src/sota_tools/ostree_object.cc

@@ -241,6 +245,10 @@ void OSTreeObject::Upload(const TreehubServer &push_target, CURLM *curl_multi_ha

  curlEasySetoptWrapper(curl_handle_, CURLOPT_PRIVATE, this);  // Used by ostree_object_from_curl

+  headers = nullptr;


If we call Upload() multiple times this will be a leak. Or is it not possible?

mike-sul · 2019-08-28T07:42:58Z

tests/sota_tools/treehub_server.py

@@ -79,6 +79,17 @@ def do_POST(self):
            self.send_response_only(200)
            self.end_headers()
            return
+        elif ctype == 'application/octet-stream':


In case of 'application/octet-stream' we can read the posted file/body just from the socket buffer, so something like that should work

elif ctype == 'application/octet-stream': length = int(self.headers['content-length']) body = self.rfile.read(length) with open("body.dat", "wb") as f: f.write(body) self.send_response_only(200) self.end_headers()

But, if the given implementation works then it's ok - this is just a test tool

Thanks Mike, I'll try your way. I was also just gonna try this:

import cgitb; cgitb.enable()

form = cgi.FieldStorage()
fileitem = form['filename']
if fileitem.filename:
with open("body.dat", "wb") as f:
f.write(fileitem.file.read())

Tested Mike's way, t_deploy output:

diff: /tmp/aktualizr-951f-8561-2d1e-5c0a/d5ea-2beb-dir/objects/: No such file or directory
/home/liping/aktualizr/src/sota_tools/deploy_test.cc:32: Failure

Interesting, I've just checked and it works for me, I missed an ending return statement at the end of the code snippet

Yes, I added the return statement. maybe we can see how it work out In CI test?

Oh, I think, I understand where is a problem. It's not enough just set a header that indicates a type of transferring data, actual data should be transferred according to the declared way. In this particular case we set 'application/octet-stream' header but actually transfer data as 'multipart/form-data', see OSTreeObject::Upload() code below setting the headers.

mike-sul · 2019-08-28T08:00:14Z

src/sota_tools/ostree_object.cc

@@ -241,6 +246,10 @@ void OSTreeObject::Upload(const TreehubServer &push_target, CURLM *curl_multi_ha

  curlEasySetoptWrapper(curl_handle_, CURLOPT_PRIVATE, this);  // Used by ostree_object_from_curl

+  assert(headers_ == nullptr);


If OSTreeObject::Upload is called more than one for the same class instance then it won't work. I suppose, in our case it's called only once so it shouldn't be a problem, although, perhaps makes sense to set the headers in ctor ?

Zee314159 · 2019-08-29T14:40:30Z

All the tests passed except 'OstreeObject_test.UploadFail". II'll look into it tomorrow.

Zee314159 · 2019-08-30T07:54:50Z

I am not sure if I see where this fd is closed in case if the file is successfully opened.

I closed the file after the reading is done, but somehow it will cause a failing of the test. Any suggestion where to close the file?

Zee314159 · 2019-09-02T05:02:31Z

See this PR: #1244 , looks there left an unsolved issue.

mike-sul · 2019-09-02T06:35:25Z

src/sota_tools/ostree_object.cc

-    LOG_ERROR << "curl_formadd error: " << form_err;
+  struct stat file_info {};
+  fd_ = fopen(file_path_.c_str(), "rb");
+  if (fd_ == nullptr) {


As far as I understand, if a file opening or getting its stat fails the code control flow will end up at line #333 ?
I am just wondering if throwing an exception (like at line #229) will work in this case, at first glance it won't work (just thinking loudly :))

Throwing an exception might be better.

src/sota_tools/ostree_object.cc

mike-sul · 2019-09-02T08:32:30Z

@patrickvacek As far as I understand some of the tests triggered by the CI pipelines do use garag-push/deploy against the prod/SIT treehub ?

pattivacek · 2019-09-02T11:40:14Z

As far as I understand some of the tests triggered by the CI pipelines do use garag-push/deploy against the prod/SIT treehub ?

Yes, that is correct. I don't think we actually push anything, though, I think it's just testing authentication. I might be misremembering.

mike-sul · 2019-09-02T11:54:47Z

As far as I understand some of the tests triggered by the CI pipelines do use garag-push/deploy against the prod/SIT treehub ?

Yes, that is correct. I don't think we actually push anything, though, I think it's just testing authentication. I might be misremembering.

I suppose, it would be great to test this change against the real treehub, at least manually.

pattivacek · 2019-09-02T12:24:11Z

I suppose, it would be great to test this change against the real treehub, at least manually.

Absolutely, this is a requirement. Manual confirmation is enough if we have tests that effectively simulate the real backend.

Set header from ostree_object Fix headers initialize bug Uploade data as 'octet-stream' Fix bug fail ostree_object_test Close fd and not return right away Fix legacy issue of garage-deploy Recieve http/204 instead of http/200 Fix headers overwriting bug Signed-off-by: Zee314159 <[email protected]>

pattivacek · 2019-09-09T14:13:50Z

src/sota_tools/request_pool.h

@@ -42,7 +42,7 @@ class RequestPool {
  RateController rate_controller_;
  int running_requests_;
  int total_requests_made_{0};
-  const TreehubServer& server_;
+  TreehubServer& server_;


It's too bad we lose this const-ness. It doesn't seem so critical, though, and I don't have a better solution available yet, though, so maybe it's fine.

Yes, I feel sorry for the loss too. I have tried.

pattivacek

Tested against SIT and it works! The const issue is pretty minor and we can always try to restore it another time.

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch 2 times, most recently from 9f3e6db to 321413c Compare August 20, 2019 12:27

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch 3 times, most recently from 9cf5fa2 to ac6abde Compare August 27, 2019 07:03

eu-siemann reviewed Aug 27, 2019

View reviewed changes

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch 2 times, most recently from b4831ca to f9cfd21 Compare August 28, 2019 04:42

mike-sul reviewed Aug 28, 2019

View reviewed changes

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch 2 times, most recently from 04a1608 to 50cd80b Compare August 29, 2019 14:32

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch 4 times, most recently from 70b8a8a to 915e9cd Compare August 30, 2019 05:58

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch 3 times, most recently from abc53fc to 12f26e1 Compare September 2, 2019 04:56

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch from 12f26e1 to 10fa383 Compare September 2, 2019 06:12

mike-sul reviewed Sep 2, 2019

View reviewed changes

pattivacek reviewed Sep 2, 2019

View reviewed changes

src/sota_tools/ostree_object.cc Show resolved Hide resolved

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch 12 times, most recently from 06c1722 to 4523a85 Compare September 7, 2019 05:47

Zee314159 force-pushed the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch from 4523a85 to ee671d5 Compare September 9, 2019 01:39

pattivacek reviewed Sep 9, 2019

View reviewed changes

pattivacek approved these changes Sep 9, 2019

View reviewed changes

pattivacek merged commit d6e45d2 into master Sep 9, 2019

pattivacek deleted the fix/ota-2183/enable-streaming-of-garage-tool-uploads branch September 9, 2019 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/ota-2183/Enable streaming of garage-push/deploy uploads #1305

fix/ota-2183/Enable streaming of garage-push/deploy uploads #1305

Zee314159 commented Aug 20, 2019

codecov-io commented Aug 20, 2019 •

edited

Loading

Zee314159 commented Aug 21, 2019

pattivacek commented Aug 22, 2019

mike-sul commented Aug 23, 2019

pattivacek commented Aug 23, 2019

eu-siemann Aug 27, 2019

eu-siemann Aug 27, 2019

mike-sul Aug 28, 2019 •

edited

Loading

Zee314159 Aug 28, 2019

Zee314159 Aug 28, 2019

mike-sul Aug 28, 2019

Zee314159 Aug 28, 2019

mike-sul Aug 28, 2019 •

edited

Loading

mike-sul Aug 28, 2019

Zee314159 commented Aug 29, 2019 •

edited

Loading

Zee314159 commented Aug 30, 2019

Zee314159 commented Sep 2, 2019

mike-sul Sep 2, 2019

Zee314159 Sep 2, 2019

mike-sul commented Sep 2, 2019

pattivacek commented Sep 2, 2019

mike-sul commented Sep 2, 2019

pattivacek commented Sep 2, 2019

pattivacek Sep 9, 2019

Zee314159 Sep 9, 2019

pattivacek left a comment

		@@ -241,6 +245,10 @@ void OSTreeObject::Upload(const TreehubServer &push_target, CURLM *curl_multi_ha

		curlEasySetoptWrapper(curl_handle_, CURLOPT_PRIVATE, this); // Used by ostree_object_from_curl

		headers = nullptr;

		@@ -241,6 +246,10 @@ void OSTreeObject::Upload(const TreehubServer &push_target, CURLM *curl_multi_ha

		curlEasySetoptWrapper(curl_handle_, CURLOPT_PRIVATE, this); // Used by ostree_object_from_curl

		assert(headers_ == nullptr);

fix/ota-2183/Enable streaming of garage-push/deploy uploads #1305

fix/ota-2183/Enable streaming of garage-push/deploy uploads #1305

Conversation

Zee314159 commented Aug 20, 2019

codecov-io commented Aug 20, 2019 • edited Loading

Codecov Report

Zee314159 commented Aug 21, 2019

pattivacek commented Aug 22, 2019

mike-sul commented Aug 23, 2019

pattivacek commented Aug 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zee314159 commented Aug 29, 2019 • edited Loading

Zee314159 commented Aug 30, 2019

Zee314159 commented Sep 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul commented Sep 2, 2019

pattivacek commented Sep 2, 2019

mike-sul commented Sep 2, 2019

pattivacek commented Sep 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattivacek left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 20, 2019 •

edited

Loading

mike-sul Aug 28, 2019 •

edited

Loading

mike-sul Aug 28, 2019 •

edited

Loading

Zee314159 commented Aug 29, 2019 •

edited

Loading