Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

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

Merged
merged 1 commit into from
Sep 9, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/sota_tools/check.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ static size_t writeString(void *contents, size_t size, size_t nmemb, void *userp
return size * nmemb;
}

int CheckRefValid(const TreehubServer &treehub, const std::string &ref, RunMode mode, int max_curl_requests) {
int CheckRefValid(TreehubServer &treehub, const std::string &ref, RunMode mode, int max_curl_requests) {
// Check if the ref is present on treehub. The traditional use case is that it
// should be a commit object, but we allow walking the tree given any OSTree
// ref.
Expand Down
2 changes: 1 addition & 1 deletion src/sota_tools/check.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@
/**
*Check if the ref is present on the server and in targets.json
*/
int CheckRefValid(const TreehubServer& treehub, const std::string& ref, RunMode mode, int max_curl_requests);
int CheckRefValid(TreehubServer& treehub, const std::string& ref, RunMode mode, int max_curl_requests);

#endif
14 changes: 8 additions & 6 deletions src/sota_tools/garage_deploy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,18 +115,20 @@ int main(int argc, char **argv) {
if (mode == RunMode::kDefault) {
if (push_credentials.CanSignOffline()) {
bool ok = OfflineSignRepo(ServerCredentials(push_credentials.GetPathOnDisk()), name, commit, hardwareids);
return static_cast<int>(!ok);
if (ok) {
if (CheckRefValid(fetch_server, ostree_commit, mode, max_curl_requests) != EXIT_SUCCESS) {
LOG_FATAL << "Check if the ref is present on the server or in targets.json failed";
return EXIT_FAILURE;
}
} else {
return EXIT_FAILURE;
}
}
LOG_FATAL << "Online signing with garage-deploy is currently unsupported";
return EXIT_FAILURE;
} else {
LOG_INFO << "Dry run. Not attempting offline signing.";
}

if (CheckRefValid(fetch_server, ostree_commit, mode, max_curl_requests) != EXIT_SUCCESS) {
LOG_FATAL << "Check if the ref is present on the server or in targets.json failed";
return EXIT_FAILURE;
}
} catch (OSTreeCommitParseError &e) {
LOG_FATAL << e.what();
return EXIT_FAILURE;
Expand Down
4 changes: 2 additions & 2 deletions src/sota_tools/ostree_http_repo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

class OSTreeHttpRepo : public OSTreeRepo {
public:
explicit OSTreeHttpRepo(const TreehubServer* server) : server_(server) {}
explicit OSTreeHttpRepo(TreehubServer* server) : server_(server) {}
~OSTreeHttpRepo() override = default;

bool LooksValid() const override;
Expand All @@ -20,7 +20,7 @@ class OSTreeHttpRepo : public OSTreeRepo {
bool FetchObject(const boost::filesystem::path& path) const override;
static size_t curl_handle_write(void* buffer, size_t size, size_t nmemb, void* userp);

const TreehubServer* server_;
TreehubServer* server_;
const TemporaryDirectory root_;
};

Expand Down
30 changes: 16 additions & 14 deletions src/sota_tools/ostree_object.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <assert.h>

#include <sys/stat.h>
#include <cstring>
#include <iostream>

Expand All @@ -21,7 +22,7 @@ OSTreeObject::OSTreeObject(const OSTreeRepo &repo, const std::string &object_nam
refcount_(0),
is_on_server_(PresenceOnServer::kObjectStateUnknown),
curl_handle_(nullptr),
form_post_(nullptr) {
fd_(nullptr) {
if (!boost::filesystem::is_regular_file(file_path_)) {
throw std::runtime_error(file_path_.native() + " is not a valid OSTree repo.");
}
Expand Down Expand Up @@ -206,7 +207,7 @@ void OSTreeObject::MakeTestRequest(const TreehubServer &push_target, CURLM *curl
request_start_time_ = std::chrono::steady_clock::now();
}

void OSTreeObject::Upload(const TreehubServer &push_target, CURLM *curl_multi_handle, const RunMode mode) {
void OSTreeObject::Upload(TreehubServer &push_target, CURLM *curl_multi_handle, const RunMode mode) {
if (mode == RunMode::kDefault || mode == RunMode::kPushTree) {
LOG_INFO << "Uploading " << object_name_;
} else {
Expand All @@ -222,25 +223,27 @@ void OSTreeObject::Upload(const TreehubServer &push_target, CURLM *curl_multi_ha
}
curlEasySetoptWrapper(curl_handle_, CURLOPT_VERBOSE, get_curlopt_verbose());
current_operation_ = CurrentOp::kOstreeObjectUploading;
push_target.SetContentType("Content-Type: application/octet-stream");
push_target.InjectIntoCurl(Url(), curl_handle_);
curlEasySetoptWrapper(curl_handle_, CURLOPT_USERAGENT, Utils::getUserAgent());
curlEasySetoptWrapper(curl_handle_, CURLOPT_WRITEFUNCTION, &OSTreeObject::curl_handle_write);
curlEasySetoptWrapper(curl_handle_, CURLOPT_WRITEDATA, this);
http_response_.str(""); // Empty the response buffer

assert(form_post_ == nullptr);
struct curl_httppost *last_item = nullptr;
const CURLFORMcode form_err =
curl_formadd(&form_post_, &last_item, CURLFORM_COPYNAME, "file", CURLFORM_FILE, file_path_.c_str(), CURLFORM_END);
if (form_err != 0u) {
// Apparently there is not strerror for formadd.
LOG_ERROR << "curl_formadd error: " << form_err;
struct stat file_info {};
fd_ = fopen(file_path_.c_str(), "rb");
if (fd_ == nullptr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing an exception might be better.

throw std::runtime_error("could not open file to be uploaded");
} else {
if (stat(file_path_.c_str(), &file_info) < 0) {
throw std::runtime_error("Could not get file information");
}
}
curlEasySetoptWrapper(curl_handle_, CURLOPT_READDATA, fd_);
curlEasySetoptWrapper(curl_handle_, CURLOPT_POSTFIELDSIZE, file_info.st_size);
curlEasySetoptWrapper(curl_handle_, CURLOPT_POST, 1);
curlEasySetoptWrapper(curl_handle_, CURLOPT_HTTPPOST, form_post_);

curlEasySetoptWrapper(curl_handle_, CURLOPT_PRIVATE, this); // Used by ostree_object_from_curl
pattivacek marked this conversation as resolved.
Show resolved Hide resolved

const CURLMcode err = curl_multi_add_handle(curl_multi_handle, curl_handle_);
if (err != 0) {
LOG_ERROR << "curl_multi_add_handle error:" << curl_multi_strerror(err);
Expand Down Expand Up @@ -316,13 +319,11 @@ void OSTreeObject::CurlDone(CURLM *curl_multi_handle, RequestPool &pool) {
}

} else if (current_operation_ == CurrentOp::kOstreeObjectUploading) {
curl_formfree(form_post_);
form_post_ = nullptr;
// Sanity-check the handle's URL to make sure it contains the expected
// object hash.
if (url == nullptr || strstr(url, object_name_.c_str()) == nullptr) {
UploadError(pool, rescode);
} else if (rescode == 200) {
} else if (rescode == 204) {
LOG_TRACE << "OSTree upload successful";
is_on_server_ = PresenceOnServer::kObjectPresent;
last_operation_result_ = ServerResponse::kOk;
Expand All @@ -335,6 +336,7 @@ void OSTreeObject::CurlDone(CURLM *curl_multi_handle, RequestPool &pool) {
} else {
UploadError(pool, rescode);
}
fclose(fd_);
} else {
LOG_ERROR << "Unknown operation: " << static_cast<int>(current_operation_);
assert(0);
Expand Down
4 changes: 2 additions & 2 deletions src/sota_tools/ostree_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class OSTreeObject {
void MakeTestRequest(const TreehubServer& push_target, CURLM* curl_multi_handle);

/* Upload this object to the destination server. */
void Upload(const TreehubServer& push_target, CURLM* curl_multi_handle, RunMode mode);
void Upload(TreehubServer& push_target, CURLM* curl_multi_handle, RunMode mode);

/* Process a completed curl transaction (presence check or upload). */
void CurlDone(CURLM* curl_multi_handle, RequestPool& pool);
Expand Down Expand Up @@ -110,7 +110,7 @@ class OSTreeObject {

std::stringstream http_response_;
CURL* curl_handle_;
struct curl_httppost* form_post_;
FILE* fd_;
std::list<parentref> parents_;
std::list<OSTreeObject::ptr> children_;

Expand Down
10 changes: 1 addition & 9 deletions src/sota_tools/ostree_object_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ TEST(OstreeObject, UploadDryRun) {
EXPECT_EQ(object->is_on_server_, PresenceOnServer::kObjectPresent);
// This currently does not get reset.
EXPECT_EQ(object->current_operation_, CurrentOp::kOstreeObjectPresenceCheck);
// This should not get allocated.
EXPECT_EQ(object->form_post_, nullptr);
}

/* Detect curl misconfiguration.
Expand All @@ -128,10 +126,6 @@ TEST(OstreeObject, UploadFail) {
object->Upload(push_server, nullptr, RunMode::kDefault);
EXPECT_EQ(object->is_on_server_, PresenceOnServer::kObjectStateUnknown);
EXPECT_EQ(object->current_operation_, CurrentOp::kOstreeObjectUploading);
// This currently will get allocated and we need to free it.
EXPECT_NE(object->form_post_, nullptr);
curl_formfree(object->form_post_);
object->form_post_ = nullptr;
}

/* Upload missing OSTree objects to destination repository. */
Expand Down Expand Up @@ -182,10 +176,8 @@ TEST(OstreeObject, UploadSuccess) {
EXPECT_GE(h->refcount_, 1);
long rescode = 0; // NOLINT(google-runtime-int)
curl_easy_getinfo(h->curl_handle_, CURLINFO_RESPONSE_CODE, &rescode);
EXPECT_EQ(rescode, 200);
EXPECT_EQ(rescode, 204);

curl_formfree(h->form_post_);
h->form_post_ = nullptr;
curl_multi_remove_handle(multi, h->curl_handle_);
curl_easy_cleanup(h->curl_handle_);
h->curl_handle_ = nullptr;
Expand Down
2 changes: 1 addition & 1 deletion src/sota_tools/request_pool.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

#include "logging/logging.h"

RequestPool::RequestPool(const TreehubServer& server, const int max_curl_requests, const RunMode mode)
RequestPool::RequestPool(TreehubServer& server, const int max_curl_requests, const RunMode mode)
: rate_controller_(max_curl_requests), running_requests_(0), server_(server), mode_(mode), stopped_(false) {
curl_global_init(CURL_GLOBAL_DEFAULT);
multi_ = curl_multi_init();
Expand Down
4 changes: 2 additions & 2 deletions src/sota_tools/request_pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

class RequestPool {
public:
RequestPool(const TreehubServer& server, int max_curl_requests, RunMode mode);
RequestPool(TreehubServer& server, int max_curl_requests, RunMode mode);
~RequestPool();
void AddQuery(const OSTreeObject::ptr& request);
void AddUpload(const OSTreeObject::ptr& request);
Expand Down Expand Up @@ -42,7 +42,7 @@ class RequestPool {
RateController rate_controller_;
int running_requests_;
int total_requests_made_{0};
const TreehubServer& server_;
TreehubServer& server_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CURLM* multi_;
std::list<OSTreeObject::ptr> query_queue_;
std::list<OSTreeObject::ptr> upload_queue_;
Expand Down
16 changes: 14 additions & 2 deletions src/sota_tools/treehub_server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,30 @@ TreehubServer::TreehubServer() {
auth_header_.next = &force_header_;
force_header_contents_ = "x-ats-ostree-force: true";
force_header_.data = const_cast<char*>(force_header_contents_.c_str());
force_header_.next = nullptr;
force_header_.next = &content_type_header_;
content_type_header_.data = const_cast<char*>(content_type_header_contents_.c_str());
content_type_header_.next = nullptr;
}

void TreehubServer::SetToken(const string& token) {
assert(auth_header_.next == &force_header_);
assert(force_header_.next == nullptr);
assert(force_header_.next == &content_type_header_);
assert(content_type_header_.next == nullptr);

auth_header_contents_ = "Authorization: Bearer " + token;
auth_header_.data = const_cast<char*>(auth_header_contents_.c_str());
method_ = AuthMethod::kOauth2;
}

void TreehubServer::SetContentType(const string& content_type) {
assert(auth_header_.next == &force_header_);
assert(force_header_.next == &content_type_header_);
assert(content_type_header_.next == nullptr);

content_type_header_contents_ = content_type;
content_type_header_.data = const_cast<char*>(content_type_header_contents_.c_str());
}

void TreehubServer::SetCerts(const std::string& client_p12) {
method_ = AuthMethod::kTls;
client_p12_path_.PutContents(client_p12);
Expand Down
5 changes: 5 additions & 0 deletions src/sota_tools/treehub_server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class TreehubServer {
public:
TreehubServer();
void SetToken(const std::string &token);
void SetContentType(const std::string &content_type);
void SetCerts(const std::string &client_p12);
void SetAuthBasic(const std::string &username, const std::string &password);

Expand Down Expand Up @@ -39,6 +40,10 @@ class TreehubServer {
// Don't modify force_header_contents_ without updating the pointer in
// force_header_
std::string force_header_contents_;
struct curl_slist content_type_header_ {};
// Don't modify content_type_header_contents_ without updating the pointer in
// content_type_header_
std::string content_type_header_contents_;
};

// vim: set tabstop=2 shiftwidth=2 expandtab:
Expand Down
2 changes: 1 addition & 1 deletion tests/sota_tools/test-server-500
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class OstreeRepo(object):
if self.upload_count % 2 == 0:
self.did_return_500.set()
return 500
return 200
return 204

def query(self, name):
self.query_count += 1
Expand Down
2 changes: 1 addition & 1 deletion tests/sota_tools/test-server-500_after_20
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class OstreeRepo(object):
self.upload_count += 1
if self.upload_count >= 20:
return 500
return 200
return 204

def query(self, name):
return 404
Expand Down
2 changes: 1 addition & 1 deletion tests/sota_tools/test-server-error_every_10
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class OstreeRepo(object):
self.upload_count += 1
if self.upload_count % 10 == 0:
return status_code
return 200
return 204

def query(self, name):
return 404
Expand Down
10 changes: 10 additions & 0 deletions tests/sota_tools/treehub_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,16 @@ def do_POST(self):
self.send_response_only(200)
self.end_headers()
return
elif ctype == 'application/octet-stream':
Copy link
Collaborator

@mike-sul mike-sul Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@mike-sul mike-sul Aug 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

length = int(self.headers['content-length'])
body = self.rfile.read(length)
full_path = os.path.join(repo_path, self.path[1:])
os.system("mkdir -p %s" % os.path.dirname(full_path))
with open(full_path, "wb") as f:
f.write(body)
self.send_response_only(204)
self.end_headers()
return

self.send_response_only(400)
self.end_headers()
Expand Down