Add Rust API for Storage Channel (#77) #137

michael-kernel-sanders · 2019-07-08T13:04:18Z

This change is

tiziano88 · 2019-07-08T13:09:49Z

oak/server/oak_node.cc

@@ -318,6 +320,13 @@ grpc::Status OakNode::ProcessModuleInvocation(grpc::GenericServerContext* contex
  channels_[LOGGING_CHANNEL_HANDLE] = std::move(logging_channel);
  LOG(INFO) << "Created logging channel";

+  // TODO: Move to initialization method.


I expect this to end up being a new type of (pseudo-)node:

oak/oak/proto/manager.proto

Lines 38 to 42 in 4523eed

oneof node_type {

LogNode log_node = 2;

GrpcServerNode grpc_server_node = 3;

WebAssemblyNode web_assembly_node = 4;

}

tiziano88 · 2019-07-08T13:13:41Z

oak/proto/storage.proto

 }

 message RollbackResponse {
 }

 // Wrapper message used to send requests via the StorageChannel interface.
 message StorageOperationRequest {
+  // The name of the storage on which to perform the operation.
+  // The StorageChannel generates a name-based (version 5) UUID from this value.


Not sure I follow this, is the UUID deterministically generated from the name? What purpose does it serve?

Updated comment.

tiziano88 · 2019-07-08T13:16:08Z

rust/oak/src/lib.rs

@@ -132,3 +137,65 @@ pub extern "C" fn oak_handle_grpc_call() {
        }
    });
 }
+
+pub fn execute_storage_operation(


I would suggest moving the storage-related stuff in its own module to start with (so it can be invoked as oak::storage::execute_storage_operation or similar, unless previously imported)

googlebot · 2019-08-12T11:51:30Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-12T12:35:55Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tiziano88

Thanks, adding @daviddrysdale to look at the channel stuff more closely

tiziano88 · 2019-08-22T12:15:38Z

oak/proto/oak_api.proto

I think STORAGE_IN and STORAGE_OUT make more sense here, both for consistency, and also so they don't suggest that one is for reading and the other for writing data to storage.

tiziano88 · 2019-08-22T12:26:50Z

oak/server/oak_node.cc

@@ -183,7 +186,15 @@ static void RunModule(wabt::interp::Environment* env, wabt::interp::DefinedModul
  }
 }

-OakNode::OakNode() : Service() {}
+OakNode::OakNode() : Service() {
+  std::unique_ptr<ChannelHalf> storage_read_channel =


Should this be around

oak/oak/server/oak_node.cc

Lines 188 to 263 in 7d75624

std::unique_ptr<OakNode> OakNode::Create(const std::string& module) {

LOG(INFO) << "Creating Oak Node";

std::unique_ptr<OakNode> node = absl::WrapUnique(new OakNode());

node->InitEnvironment(&node->env_);

LOG(INFO) << "Host func count: " << node->env_.GetFuncCount();

wabt::Errors errors;

LOG(INFO) << "Reading module";

wabt::Result result = ReadModule(module, &node->env_, &errors, &node->module_);

if (!wabt::Succeeded(result)) {

LOG(WARNING) << "Could not read module: " << result;

LOG(WARNING) << "Errors: " << wabt::FormatErrorsToString(errors, wabt::Location::Type::Binary);

return nullptr;

}

LOG(INFO) << "Reading module done";

if (!CheckModuleExports(&node->env_, node->module_)) {

LOG(WARNING) << "Failed to validate module";

return nullptr;

}

// Create a logging channel for the module.

{

std::shared_ptr<MessageChannel> channel = std::make_shared<MessageChannel>();

node->channel_halves_[ChannelHandle::LOGGING] =

absl::make_unique<MessageChannelWriteHalf>(channel);

LOG(INFO) << "Created logging channel " << ChannelHandle::LOGGING;

// Spawn a thread that reads and logs messages on this channel forever.

std::thread t([channel] {

std::unique_ptr<MessageChannelReadHalf> read_chan =

absl::make_unique<MessageChannelReadHalf>(channel);

while (true) {

ReadResult result = read_chan->BlockingRead(INT_MAX);

if (result.required_size > 0) {

LOG(ERROR) << "Message size too large: " << result.required_size;

return;

}

LOG(INFO) << "LOG: " << std::string(result.data->data(), result.data->size());

}

});

// TODO: join() instead when we have node termination

t.detach();

}

// Create the channels needed for gRPC interactions.

{

// Incoming request channel: keep the write half in C++, but map the read

// half to a well-known channel handle.

std::shared_ptr<MessageChannel> channel = std::make_shared<MessageChannel>();

node->channel_halves_[ChannelHandle::GRPC_IN] =

absl::make_unique<MessageChannelReadHalf>(channel);

node->req_half_ = absl::make_unique<MessageChannelWriteHalf>(channel);

LOG(INFO) << "Created gRPC input channel: " << ChannelHandle::GRPC_IN;

}

{

// Outgoing response channel: keep the read half in C++, but map the write

// half to a well-known channel handle.

std::shared_ptr<MessageChannel> channel = std::make_shared<MessageChannel>();

node->channel_halves_[ChannelHandle::GRPC_OUT] =

absl::make_unique<MessageChannelWriteHalf>(channel);

node->rsp_half_ = absl::make_unique<MessageChannelReadHalf>(channel);

LOG(INFO) << "Created gRPC output channel: " << ChannelHandle::GRPC_IN;

}

// Spin up a per-node Wasm thread to run forever; the Node object must

// outlast this thread.

LOG(INFO) << "Executing module oak_main";

std::thread t([&node] { RunModule(&node->env_, node->module_); });

// TODO: join() instead when we have node termination

t.detach();

LOG(INFO) << "Started module execution thread";

return node;

}

?

cc @daviddrysdale

Yeah, keeping all of the well-known channel creation stuff together makes sense, either here or in CreateNode (and I'd probably go for the latter because we don't want to start a logging thread until we're sure the Node instance is good to go).

tiziano88 · 2019-08-22T12:29:11Z

oak/server/storage/storage_read_channel.h

+namespace oak {
+
+// A channel implementation that reads from a StorageService.
+class StorageReadChannel final : public ChannelHalf {


Should this extend

oak/oak/server/channel.h

Line 84 in 7d75624

class MessageChannel : public ChannelHalf {

instead?

BTW, @daviddrysdale maybe channel_halves_ should keep track of MessageChannel instance rather than ChannelHalf? I guess we expect all channels to provide async methods for reading / writing?

At the moment we've got a single (per-Node) numbering space for channel handles, regardless of direction, so channel_halves_ maps to a channel half rather than a channel. (To put it another way: when we get as far as creating channels at runtime, the creator will get two numbers to refer to the new channel, one for read and one for write.)

More generally, I'm idly wondering if the whole storage channel thing could be replaced with a couple of MessageChannel instances that connect to some C++ code (i.e. a pseudo-node, to use the terminology from manager.proto).

But I'm not yet ramped up enough on the storage design to know if that's a sensible or a silly idea…

daviddrysdale · 2019-08-22T13:20:00Z

oak/server/oak_node.cc

@@ -183,7 +186,15 @@ static void RunModule(wabt::interp::Environment* env, wabt::interp::DefinedModul
  }
 }

-OakNode::OakNode() : Service() {}
+OakNode::OakNode() : Service() {
+  std::unique_ptr<ChannelHalf> storage_read_channel =


Yeah, keeping all of the well-known channel creation stuff together makes sense, either here or in CreateNode (and I'd probably go for the latter because we don't want to start a logging thread until we're sure the Node instance is good to go).

daviddrysdale · 2019-08-22T13:25:03Z

oak/server/storage/storage_manager.h

+
+ private:
+  // The serialized StorageOperationResponse message set by WriteResponseData.
+  std::string operation_response_data_;


I wonder if this class be replaced by an instance of MessageChannel ?

We can probably follow up with a refactoring... will discuss offline.

daviddrysdale · 2019-08-22T13:26:11Z

oak/server/storage/storage_manager.h

+  StorageManager() {}
+
+  std::unique_ptr<Message> ReadResponseData(uint32_t size) {
+    absl::Span<const char> response_data = operation_response_view_.subspan(0, size);


This seems like it's aligned more with the previous stream-based channels, rather than the current message-based approach?

+1, @michael-kernel-sanders do you plan to fix this in this PR or to follow up later? If the latter, perhaps add a TODO somewhere?

Added TODO.

daviddrysdale · 2019-08-22T13:32:26Z

oak/proto/oak_api.proto

+  STORAGE_READ = 4;
+  STORAGE_WRITE = 5;


Update README.md to describe the new well-known channel handles?

daviddrysdale · 2019-08-22T13:33:38Z

oak/proto/storage.proto

 }

 message RollbackResponse {
 }

 // Wrapper message used to send requests via the StorageChannel interface.
 message StorageOperationRequest {
+  // The name of the storage on which to perform the operation.
+  // The StorageChannel generates a deterministic name-based (version 5) UUID
+  // from this value and the Oak Module to use as the storage identifier.


(I didn't really follow this, but that might be lack of background)

I'm also still not entirely sure what this is used for or whether it's necessary / nice to have (and why).

In what sense does the StorageChannel generate something from this value and the Oak Module? Do you mean the name of the module? or the hash of the code? or something else?

daviddrysdale · 2019-08-22T13:42:36Z

oak/server/storage/storage_read_channel.h

+namespace oak {
+
+// A channel implementation that reads from a StorageService.
+class StorageReadChannel final : public ChannelHalf {


At the moment we've got a single (per-Node) numbering space for channel handles, regardless of direction, so channel_halves_ maps to a channel half rather than a channel. (To put it another way: when we get as far as creating channels at runtime, the creator will get two numbers to refer to the new channel, one for read and one for write.)

More generally, I'm idly wondering if the whole storage channel thing could be replaced with a couple of MessageChannel instances that connect to some C++ code (i.e. a pseudo-node, to use the terminology from manager.proto).

But I'm not yet ramped up enough on the storage design to know if that's a sensible or a silly idea…

daviddrysdale · 2019-08-22T13:43:03Z

oak/proto/oak_api.proto

+  STORAGE_IN = 4;
+  STORAGE_OUT = 5;


Update README.md to describe the new well-known channel handles?

tiziano88 · 2019-08-28T11:40:23Z

oak/proto/storage.proto

@@ -77,7 +77,9 @@ message RollbackResponse {
 message StorageOperationRequest {
  // The name of the storage on which to perform the operation.


I still think this is quite vague, what is a "storage" and what defines its name? Maybe this should point to some high level comment? Or this proto message should contain more information on how the storage works? e.g. "a storage backend is subdivided into different namespaces, whose values are UUIDs derived from a user-provided string... if multiple mutually distrusting clients end up choosing the same name then $X happens" etc. . Maybe this is already present in the code, so you can just add a link to it?

Attempted to clarify.

Separated the storage channel and service messages.

tiziano88

Thanks Michael and sorry for the delay!

michael-kernel-sanders requested review from tiziano88 and daviddrysdale July 8, 2019 13:04

googlebot added the cla: yes label Jul 8, 2019

tiziano88 reviewed Jul 8, 2019

View reviewed changes

michael-kernel-sanders force-pushed the mks_storage_module_api branch from a8f0a8d to c713556 Compare July 29, 2019 12:21

googlebot added cla: no and removed cla: yes labels Aug 12, 2019

michael-kernel-sanders force-pushed the mks_storage_module_api branch from 2b96408 to c1689a0 Compare August 12, 2019 12:35

googlebot added cla: yes and removed cla: no labels Aug 12, 2019

michael-kernel-sanders force-pushed the mks_storage_module_api branch 2 times, most recently from 6ffd8b8 to 06226e7 Compare August 13, 2019 13:36

michael-kernel-sanders force-pushed the mks_storage_module_api branch 2 times, most recently from acc5323 to 5ec2cdc Compare August 22, 2019 10:10

tiziano88 reviewed Aug 22, 2019

View reviewed changes

daviddrysdale reviewed Aug 22, 2019

View reviewed changes

tiziano88 reviewed Aug 28, 2019

View reviewed changes

michael-kernel-sanders force-pushed the mks_storage_module_api branch 2 times, most recently from 411a11d to 43d3731 Compare August 30, 2019 11:33

michael-kernel-sanders added 9 commits August 30, 2019 14:10

Add Rust API for Storage Channel (project-oak#77)

44b8dca

Convert to ChannelHalf interface

de1228b

Fix proto import path

6ed5523

Fix build, update Channel implementation again

b5535b7

Fix examples

5c6b7b9

Fix merge/rebase

2560413

Fix storage channel handles

db8ca16

Address review comment

48b309b

Address review comments

1ae69f7

michael-kernel-sanders added 8 commits August 30, 2019 14:10

Document storage channels

5b63ea1

Add missing storage module

bdcd2c4

Address review comments

3a56c6e

Clarify Storage service description

5b75131

Separate the storage channel and service messages

7c4a918

Fix storage module to build

53acae5

Move Storage channels to OakRuntime

21c55af

Add storage_channel.rs

aac3301

michael-kernel-sanders force-pushed the mks_storage_module_api branch from 2e976f0 to aac3301 Compare August 30, 2019 13:13

michael-kernel-sanders added 3 commits August 30, 2019 14:25

Fix clippy errors

1d2cb84

Fix return

93208c2

Fix clippy errors redux

3d401a9

tiziano88 approved these changes Sep 3, 2019

View reviewed changes

michael-kernel-sanders merged commit 9574991 into project-oak:master Sep 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rust API for Storage Channel (#77) #137

Add Rust API for Storage Channel (#77) #137

michael-kernel-sanders commented Jul 8, 2019 •

edited by tiziano88

Loading

tiziano88 Jul 8, 2019

tiziano88 Jul 8, 2019

michael-kernel-sanders Aug 22, 2019

tiziano88 Jul 8, 2019

michael-kernel-sanders Aug 22, 2019

googlebot commented Aug 12, 2019

googlebot commented Aug 12, 2019

tiziano88 left a comment

tiziano88 Aug 22, 2019

michael-kernel-sanders Aug 22, 2019

tiziano88 Aug 22, 2019

daviddrysdale Aug 22, 2019

michael-kernel-sanders Aug 22, 2019

tiziano88 Aug 22, 2019

daviddrysdale Aug 22, 2019

daviddrysdale Aug 22, 2019

daviddrysdale Aug 22, 2019

michael-kernel-sanders Aug 22, 2019

daviddrysdale Aug 22, 2019

tiziano88 Aug 27, 2019

michael-kernel-sanders Aug 28, 2019

daviddrysdale Aug 22, 2019

michael-kernel-sanders Aug 22, 2019

daviddrysdale Aug 22, 2019

tiziano88 Aug 27, 2019

tiziano88 Aug 28, 2019

michael-kernel-sanders Aug 28, 2019

daviddrysdale Aug 22, 2019

daviddrysdale Aug 22, 2019

tiziano88 Aug 28, 2019

michael-kernel-sanders Aug 28, 2019

michael-kernel-sanders Aug 29, 2019

tiziano88 left a comment

	oneof node_type {
	LogNode log_node = 2;
	GrpcServerNode grpc_server_node = 3;
	WebAssemblyNode web_assembly_node = 4;
	}

	std::unique_ptr<OakNode> OakNode::Create(const std::string& module) {
	LOG(INFO) << "Creating Oak Node";

	std::unique_ptr<OakNode> node = absl::WrapUnique(new OakNode());
	node->InitEnvironment(&node->env_);
	LOG(INFO) << "Host func count: " << node->env_.GetFuncCount();

	wabt::Errors errors;
	LOG(INFO) << "Reading module";
	wabt::Result result = ReadModule(module, &node->env_, &errors, &node->module_);
	if (!wabt::Succeeded(result)) {
	LOG(WARNING) << "Could not read module: " << result;
	LOG(WARNING) << "Errors: " << wabt::FormatErrorsToString(errors, wabt::Location::Type::Binary);
	return nullptr;
	}

	LOG(INFO) << "Reading module done";
	if (!CheckModuleExports(&node->env_, node->module_)) {
	LOG(WARNING) << "Failed to validate module";
	return nullptr;
	}

	// Create a logging channel for the module.
	{
	std::shared_ptr<MessageChannel> channel = std::make_shared<MessageChannel>();
	node->channel_halves_[ChannelHandle::LOGGING] =
	absl::make_unique<MessageChannelWriteHalf>(channel);
	LOG(INFO) << "Created logging channel " << ChannelHandle::LOGGING;

	// Spawn a thread that reads and logs messages on this channel forever.
	std::thread t([channel] {
	std::unique_ptr<MessageChannelReadHalf> read_chan =
	absl::make_unique<MessageChannelReadHalf>(channel);
	while (true) {
	ReadResult result = read_chan->BlockingRead(INT_MAX);
	if (result.required_size > 0) {
	LOG(ERROR) << "Message size too large: " << result.required_size;
	return;
	}
	LOG(INFO) << "LOG: " << std::string(result.data->data(), result.data->size());
	}
	});
	// TODO: join() instead when we have node termination
	t.detach();
	}

	// Create the channels needed for gRPC interactions.
	{
	// Incoming request channel: keep the write half in C++, but map the read
	// half to a well-known channel handle.
	std::shared_ptr<MessageChannel> channel = std::make_shared<MessageChannel>();
	node->channel_halves_[ChannelHandle::GRPC_IN] =
	absl::make_unique<MessageChannelReadHalf>(channel);
	node->req_half_ = absl::make_unique<MessageChannelWriteHalf>(channel);
	LOG(INFO) << "Created gRPC input channel: " << ChannelHandle::GRPC_IN;
	}
	{
	// Outgoing response channel: keep the read half in C++, but map the write
	// half to a well-known channel handle.
	std::shared_ptr<MessageChannel> channel = std::make_shared<MessageChannel>();
	node->channel_halves_[ChannelHandle::GRPC_OUT] =
	absl::make_unique<MessageChannelWriteHalf>(channel);
	node->rsp_half_ = absl::make_unique<MessageChannelReadHalf>(channel);
	LOG(INFO) << "Created gRPC output channel: " << ChannelHandle::GRPC_IN;
	}

	// Spin up a per-node Wasm thread to run forever; the Node object must
	// outlast this thread.
	LOG(INFO) << "Executing module oak_main";
	std::thread t([&node] { RunModule(&node->env_, node->module_); });
	// TODO: join() instead when we have node termination
	t.detach();
	LOG(INFO) << "Started module execution thread";

	return node;
	}

		@@ -77,7 +77,9 @@ message RollbackResponse {
		message StorageOperationRequest {
		// The name of the storage on which to perform the operation.

Add Rust API for Storage Channel (#77) #137

Add Rust API for Storage Channel (#77) #137

Conversation

michael-kernel-sanders commented Jul 8, 2019 • edited by tiziano88 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

googlebot commented Aug 12, 2019

googlebot commented Aug 12, 2019

tiziano88 left a 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

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

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

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

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

Choose a reason for hiding this comment

tiziano88 left a comment

Choose a reason for hiding this comment

michael-kernel-sanders commented Jul 8, 2019 •

edited by tiziano88

Loading