feat: add base agent #12

WoodenMaiden · 2023-03-28T11:30:33Z

Hello!

This PR aims to add the agent, whose job is:

To execute commands the api provides
To send back the standard output & standard error output to the agent.

Resolves #5 & #6

WoodenMaiden · 2023-03-28T16:39:18Z

Hey @sameo 👋, we did some cleanup on our branch.

You might notice some things:

@Alexis-Bernard's commits are signed with @GridexX and my emails as he forgot to add the -s flag and did not have time to amend the commits himself.
Commits from c12f4af to 6517574 belong to PR Feature: add base architecture #9. You should merge this first.

sameo · 2023-04-04T07:49:39Z

Hey @sameo 👋, we did some cleanup on our branch.

You might notice some things:

@Alexis-Bernard's commits are signed with @GridexX and my emails as he forgot to add the -s flag and did not have time to amend the commits himself.

Commits from c12f4af to 6517574 belong to PR Feature: add base architecture #9. You should merge this first.

I merged #9 now.

For me to review this PR, I need you to:

Rebase it on top of main
Clean your git history up. It's not important to me (the reviewer), or to anyone else looking at this repo git history to know that you fix some formatting issues with e210ec9, for example. Structure your git history into commits that show how you logically implemented the feature this PR is bringing.

GridexX · 2023-04-09T14:06:47Z

Hello @sameo We both have Rebase it on top of main and clean our git history up without the fix and chore commits.

The PR is ready to be reviewed 😄

sameo

This PR is made of 14 commits, that show you put a lot of work and time on it. As a teacher, I appreciate that and I can see there's been a lot of fixups, feature additions, removal, files deleted and removed, etc.
Future users of your code will not care at all about it though. They may want to understand what you were trying to do, and you have the opportunity to tell them a nice and logical story about your work. Something like:

Hey, I am going to implement a serverless agent, running on a guest and talking to a host. Here is my AP describing the requests, responses and status messages of my protocol. That my first commit.
When I receive a request from the host, I will run a serverless workload, step by step. I will use a workspace for that where I will run the commands I got from the host request, and sent the output of those commands back to the host. This is my second commit.
The agent can be configured through a config file. I'm describing it on my 3rd commit.
Now that we have all the pieces together, I can actually implement my main function. It uses all the APIs I defined in my first 3 commits to implement the main lambdo agent flow.

Please tell me that kind of story.

sameo · 2023-04-13T07:10:01Z

agent/lib/src/external_api/model.rs

+#[derive(Deserialize, Serialize, Debug, PartialEq, Eq)]
+pub enum Type {
+    Status,
+    Request,
+    Response,
+}
+
+#[derive(Deserialize, Serialize, Debug, PartialEq, Eq)]
+pub enum Code {
+    Run,
+    Ok,
+}


This model could be improved in many ways, but basically a more scalable and efficient one would look like:

use serde::{Deserialize, Serialize}; #[repr(u16)] pub enum MessageType { Status, Request, Response, } #[derive(Deserialize, Serialize, Debug)] pub enum StatusCode { Ok, Failed, Foo, Bar, } #[derive(Deserialize, Serialize, Debug)] pub struct Step { } #[derive(Deserialize, Serialize, Debug)] pub struct File { } #[derive(Deserialize, Serialize, Debug)] pub enum MessagePayload { Status { status: StatusCode }, Request { id: String, files: Vec<File>, steps: Vec<Step>}, // Response, } pub struct MessageHeader { r#type: MessageType, length: u16, } pub struct AgentMessage { header: MessageHeader, payload: MessagePayload, }

Then:

let mut header = [0u16; 2]; serialport.read(port, &mut header); let message_type = header[0]; let message_length = header[1]; // Read message_length bytes from the serial port // Depending on message_type, deserialize into the correct payload

With that approach, you have one single type of message where the payload differs. And the header is short and describes the message type. You can then read anything from your serial port, and not assume you're only going to receive one kind of message.

I am not asking you to move to that model, because we're only a few days away from the deadline. But please address my other comments.

sameo · 2023-04-13T07:37:40Z

agent/lib/src/external_api/model.rs

+pub struct FileModel {
+    pub filename: String,
+    pub content: String,
+}
+


What is a FileModel for?

FileModel Represents the files that the API sends us as defined in this message.

I'll document this

sameo · 2023-04-13T07:38:49Z

agent/lib/src/external_api/model.rs

+    pub files: Vec<FileModel>,
+    pub steps: Vec<RequestStep>,
+}


How do the steps relate to the files?

The steps field in the api's requests list all commands to run in order to execute the code. For instance in a NodeJS Project we would have a npm ci to install dependencies, followed by a node dist/index.js, in this example the steps relates to the files because a package.json is needed to install the dependencies.

sameo · 2023-04-13T07:59:00Z

agent/lib/src/external_api/model.rs

@@ -0,0 +1,139 @@
+use serde::{Deserialize, Serialize};


On the name of that file: There's only one API, and that's the one between the agent and the host. So please rename to api/model.rs

This is not addressed yet.

agent/lib/src/external_api/model.rs

sameo · 2023-04-13T08:13:36Z

agent/lib/src/external_api/service.rs

+        // Flush the serial port
+        self.serial_port
+            .flush()
+            .map_err(|e| anyhow!("Failed to flush serial port: {}", e))?;


What do we do with the flushed bytes? Maybe there were some pending bytes from the host?

I didn't think about that. I'm deleting this

sameo · 2023-04-13T08:15:45Z

agent/lib/src/external_api/service.rs

+            .write_all(buf)
+            .map_err(|e| anyhow!("Failed to write to serial port: {}", e))?;
+
+        // In order to still be readable by ``readline`` on the other side, we add a carriage return


If the other side decides to use readline to read the agent's output, it's none of the agent's business. You should not add unspecified data to your protocol.

Normally I would absolutely agree with you.
However since the API uses BufReader::read_line they need a carriage return, else the API just blocks meaning that we have to send it.

I know this looks ridicoulous, I tried to tell them to stick to what we have defined.

sameo · 2023-04-13T08:19:46Z

agent/lib/src/internal_api/service.rs

+    fn random_usize(max: usize) -> usize {
+        let mut f = File::open("/dev/urandom").unwrap();
+        let mut buf = [0u8; 1];
+        f.read_exact(&mut buf).unwrap();
+        let value = buf[0] as usize;
+
+        if value < max {
+            max
+        } else {
+            value % max
+        }
+    }
+


https://docs.rs/rand/latest/rand/

I didnt use it as I wasn't aware of the [dev-dependencies] section. Replaced 👍

sameo · 2023-04-13T08:23:58Z

agent/lib/src/external_api/model.rs

+pub struct CodeEntry {
+    pub files: Vec<FileModel>,
+    pub script: Vec<String>, // All commands to execute at startup
+}


Is CodeEntry the entry point for running some piece of code?

This is a legacy piece of code that is not used anymore, replaced by RequestData. Thank you for noticing it

sameo · 2023-04-13T08:25:24Z

agent/lib/src/external_api/model.rs

+#[derive(Deserialize, Serialize, Debug, PartialEq, Eq)]
+pub struct FileModel {
+    pub filename: String,
+    pub content: String,


Is content the content of the file at filename? Is filename a path? or just a name?

It is a path relative to the workspace directory in internal::service

WoodenMaiden · 2023-04-13T22:07:20Z

Here are the fixes, I hope these are okay

sameo · 2023-04-16T14:38:51Z

Please rebase this PR on top of main. Do not try to merge main back into it. Let me know if you need help with this.

sameo

As explained, do not merge main into this PR, rebase on top of it instead.
Also, the lumper submodule update is confusing, to say the least.

And on this PR commits:

79bab13 is a bag of random fixes. Please have each of them merged into the commit it's fixing.
Describe what the commit is doing. For example, commit f322737 says:

feat: basic structures, code exec and RW 

* feat: define structures
* feat: add read & write from serial method
* feat: create workspace method internal api
* test(external_api): parse json and check delimiter
* feat: read both stderr and stdout
* chore: testing InternalApi.create_workspace()

But does not tell me anything about what this commit is doing and how it's doing it.

sameo · 2023-04-14T05:11:37Z

agent/lib/src/external_api/model.rs

@@ -0,0 +1,139 @@
+use serde::{Deserialize, Serialize};


This is not addressed yet.

sameo · 2023-04-14T05:21:22Z

agent/lib/src/api/model.rs

+}
+
+/// Serializes an Option<String> as a String by returning an empty string if the Option is None
+fn serialize_optionnal_string<S>(value: &Option<String>, serializer: S) -> Result<S::Ok, S::Error>


nit:

Suggested change

fn serialize_optionnal_string<S>(value: &Option<String>, serializer: S) -> Result<S::Ok, S::Error>

fn serialize_optional_string<S>(value: &Option<String>, serializer: S) -> Result<S::Ok, S::Error>

This still needs to be fixed.

sameo · 2023-04-16T15:05:02Z

agent/lib/src/api/model.rs

+        result: i32,
+        stdout: Option<String>,
+        stderr: String,
+        enable_output: bool,


You should not need that if stdoutis an Option

I am not sure to understand what you mean here.

You know if output is enabled or not by looking at stdout:

if let Some(r.stdout) { // output is enabled } else { // output is disabled }

So you no longer need the enable_output field.

sameo · 2023-04-16T15:09:50Z

agent/lib/src/api/model.rs

+pub enum Code {
+    /// Represents a request to run the code or a response to such request
+    Run,
+    /// Indicates that your status is that you are ok and ready to communicate


Suggested change

/// Indicates that your status is that you are ok and ready to communicate

/// Agent is ready to communicate.

Please rename it to Readyat least.

sameo · 2023-04-16T15:16:39Z

agent/lib/src/api/model.rs

+/// Identifies the type of the message
+#[derive(Deserialize, Serialize, Debug, PartialEq, Eq)]
+pub enum Type {
+    /// Status message to indicate that the agent is ready
+    Status,
+    /// Request message
+    Request,
+    /// Response message answering to a request message
+    Response,
+}


I don't think Typeis checked or used anywhere, except for decorating your messages with that information. As explained in #12 (comment), you will have to create a new structure (as opposed to extending a payload enum) for any new message that you'd want to extend your protocol with. So you don't need this Type enum and you should get rid of it.

sameo · 2023-04-16T15:17:18Z

agent/lib/src/api/model.rs

+    /// Command that was run
+    pub command: String,
+    /// Exit code of the command
+    pub result: i32,


Suggested change

pub result: i32,

pub exit_code: i32,

sameo · 2023-04-16T15:17:40Z

agent/lib/src/api/model.rs

+    pub stderr: String,
+    /// Whether the stdout should be returned or not
+    #[serde(alias = "enableOutput")]
+    pub enable_output: bool,


Again, I dont think this is needed.

sameo · 2023-04-16T15:31:28Z

src/config.rs

@@ -0,0 +1,116 @@
+use anyhow::Result;


lumper is a submodule. Where are those files coming from?

Signed-off-by: iverly <[email protected]>

Signed-off-by: GridexX <[email protected]>

GridexX · 2023-04-17T16:21:51Z

@sameo @WoodenMaiden
I completely rewrite the commits history, and we add your changes with Yann.
4 commits are from @iverly, because they are on our main branch and the PR where the changes were made need to be merged first 👍

sameo

A couple nits and we should be able to merge that one.
The commit messages are still empty, fwiw.

sameo · 2023-04-18T06:16:55Z

agent/lib/src/api/model.rs

+}
+
+/// Serializes an Option<String> as a String by returning an empty string if the Option is None
+fn serialize_optionnal_string<S>(value: &Option<String>, serializer: S) -> Result<S::Ok, S::Error>


This still needs to be fixed.

sameo · 2023-04-18T06:18:36Z

agent/Cargo.toml

@@ -14,8 +14,12 @@ serde = { version = "1.0.152", features = ["derive"] }
 serde_json = "1.0.93"
 serde_yaml = "0.9"
 clap = { version="4.1.6", features=["derive"] }
+unshare = "0.7.0"


Where is this crate used?

WoodenMaiden force-pushed the feature/add-base-agent branch 4 times, most recently from 609d2d0 to 00c221f Compare March 28, 2023 16:33

WoodenMaiden marked this pull request as ready for review March 28, 2023 16:34

WoodenMaiden force-pushed the feature/add-base-agent branch from 00c221f to 1574ed5 Compare March 29, 2023 15:47

GridexX force-pushed the feature/add-base-agent branch 5 times, most recently from 9aff95a to 658d128 Compare April 9, 2023 14:04

WoodenMaiden force-pushed the feature/add-base-agent branch 2 times, most recently from 1a0fcaf to 09220bf Compare April 12, 2023 18:38

sameo requested changes Apr 13, 2023

View reviewed changes

WoodenMaiden force-pushed the feature/add-base-agent branch from 09220bf to 79bab13 Compare April 13, 2023 22:04

sameo requested changes Apr 16, 2023

View reviewed changes

WoodenMaiden force-pushed the feature/add-base-agent branch from 1ab2274 to 2de3333 Compare April 17, 2023 11:12

iverly added 4 commits April 17, 2023 13:47

feat: add default logger & anyhow errors

beb4618

Signed-off-by: iverly <[email protected]>

feat: add clap and parse input

7f54aca

Signed-off-by: iverly <[email protected]>

feat: add config structure & load from path

aee324c

Signed-off-by: iverly <[email protected]>

chore: update lumper to the latest version

137ff5e

Signed-off-by: iverly <[email protected]>

WoodenMaiden force-pushed the feature/add-base-agent branch from 2de3333 to 2ee2be9 Compare April 17, 2023 12:04

GridexX added 3 commits April 17, 2023 18:02

feat(agent): implement api, send status message

3084b7e

Signed-off-by: GridexX <[email protected]>

feat(agent): add configuration and construct api

5303046

Signed-off-by: GridexX <[email protected]>

feat(agent): create workspace and launch code

6fa890b

Signed-off-by: GridexX <[email protected]>

GridexX force-pushed the feature/add-base-agent branch from 2ee2be9 to 6fa890b Compare April 17, 2023 16:18

sameo reviewed Apr 18, 2023

View reviewed changes

iverly deleted the feature/add-base-agent branch April 25, 2023 17:11

	fn serialize_optionnal_string<S>(value: &Option<String>, serializer: S) -> Result<S::Ok, S::Error>
	fn serialize_optional_string<S>(value: &Option<String>, serializer: S) -> Result<S::Ok, S::Error>

	/// Indicates that your status is that you are ok and ready to communicate
	/// Agent is ready to communicate.

feat: add base agent #12

Are you sure you want to change the base?

feat: add base agent #12

Conversation

WoodenMaiden commented Mar 28, 2023

WoodenMaiden commented Mar 28, 2023 • edited Loading

sameo commented Apr 4, 2023

GridexX commented Apr 9, 2023

sameo left a comment • 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

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

WoodenMaiden commented Apr 13, 2023

sameo commented Apr 16, 2023

sameo 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

GridexX commented Apr 17, 2023

sameo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WoodenMaiden commented Mar 28, 2023 •

edited

Loading

sameo left a comment •

edited

Loading