Tweaked invoker and added tests #540

aryans1204 · 2024-10-30T14:01:08Z

Summary

A small summary of the requirements (in one/two sentences).

Implementation Notes ⚒️

Briefly outline the overall technical solution. If necessary, identify talking points where the reviewer's attention should be drawn to.

External Dependencies 🍀

Breaking API Changes ⚠️

Simply specify none (N/A) if not applicable.

leokondrashov · 2024-10-31T06:28:02Z

pkg/driver/clients/grpc_client.go

Split it into two files: gprc_invoker and grpc_vswarm_invoker. Also there is a weird indentation going on, please run through gofmt.

There are still issues with indentations. There is an additional space on each line. Please run it through gofmt.

I think there is no point in touching this file at all. You swapped a couple of lines and renamed one of the imports, nothing substantial.

pkg/driver/clients/grpc_client_test.go

pkg/driver/clients/vhivemetadata.go

leokondrashov · 2024-10-31T06:38:22Z

pkg/driver/clients/grpc_client_test.go

+	newinvoker := CreateInvoker(cfgSwarm, nil, nil)
+
+	start = time.Now()
+	success, record = newinvoker.Invoke(&testFunction, &testRuntimeSpecs)


This should fail. We deploy the same gRPC server as runs in the invitro images. It should fail to correctly respond to vSwarm invoker.

pkg/driver/clients/grpc_client_test.go

leokondrashov · 2024-10-31T06:42:41Z

I don't see the tests for the driver/clients actually running in the CI. Can you please check the workflow?

Also, there is a linter error, please fix.

leokondrashov · 2024-11-08T08:41:41Z

Please squash your commits, sign off, and rebase the branch on current main.

leokondrashov · 2024-11-08T08:45:56Z

pkg/driver/clients/invoker.go

@@ -18,7 +18,12 @@ func CreateInvoker(cfg *config.LoaderConfiguration, announceDoneExe *sync.WaitGr
 		return newAWSLambdaInvoker(announceDoneExe)
 	case "Dirigent", "Dirigent-RPS":
 		if cfg.InvokeProtocol == "grpc" {
-			return newGRPCInvoker(cfg)
+			if !cfg.VSwarm {


You put it into Dirigent mode, the intended place should be knative.

aryans1204 · 2024-11-13T16:39:29Z

Hi, I've rebased on main and squashed and signed off my commits. I've also fixed the Dirigent mode to Knative mode. Please check

leokondrashov

Please rebase and fix the indentation in pkg/driver/clients/grpc_client.go

leokondrashov · 2024-11-15T10:54:52Z

server/trace-func-go/trace_func.go

@@ -62,5 +62,5 @@ func main() {
 		log.Infof("Function type: EMPTY\n")
 	}

-	standard.StartGRPCServer("", serverPort, functionType, *zipkin)
+	standard.StartGRPCServer("", serverPort, functionType, *zipkin, false)


Can you extract the vSwarm from here? I don't like that we are introducing this vSwarm greeter into our Invitro workload. Since we are creating it just for testing, we don't need to reuse all of the features of the workload that we have (tracing, handling SIGTERM); you can just put this mock server into test file instead. Or if you can, you can try to reuse the relay server from vSwarm, not to duplicate the code.

Fixed this, fixed the formatting as well and rebased on main. Please review again.

Thanks

I don't think you did what I was asking for.

Since we use StartVSwarmGRPCServer only in tests, we don't need anything from it except the ability to accept and respond with VSwarm protobufs. So, you can significantly slim it down by removing everything unnecessary for the simplest gRPC server (signal handling, TraceFunctionExecution, tracing).

leokondrashov

Please fix the failing linter test and look at the comment about test VSwarm server once again.

leokondrashov · 2024-12-04T12:52:58Z

pkg/driver/clients/grpc_client.go

-	if strings.Contains(strings.ToLower(i.cfg.Platform), "dirigent") {
-		dialOptions = append(dialOptions, grpc.WithAuthority(function.Name)) // Dirigent specific
-	}


Why did you remove that?

I don't see a reason to change this file at all. Can you clarify why do you need that?

Seems like this is a bad merge. Added the lines again.

leokondrashov · 2024-12-04T12:56:29Z

pkg/driver/clients/grpc_client_test.go

+}
+func TestVSwarmClientWithServerBatchWorkload(t *testing.T) {
+	logrus.SetLevel(logrus.TraceLevel)
+	err := os.Setenv("ITERATIONS_MULTIPLIER", "225")


I don't think this is needed for any VSwarm functions.

Removed this test

leokondrashov · 2024-12-04T13:05:32Z

server/trace-func-go/trace_func.go

@@ -62,5 +62,5 @@ func main() {
 		log.Infof("Function type: EMPTY\n")
 	}

-	standard.StartGRPCServer("", serverPort, functionType, *zipkin)
+	standard.StartGRPCServer("", serverPort, functionType, *zipkin, false)


I don't think you did what I was asking for.

Since we use StartVSwarmGRPCServer only in tests, we don't need anything from it except the ability to accept and respond with VSwarm protobufs. So, you can significantly slim it down by removing everything unnecessary for the simplest gRPC server (signal handling, TraceFunctionExecution, tracing).

Signed-off-by: aryans1204 <[email protected]> Add VHIVE_REPO and LOADER_REPO to setup script config Signed-off-by: Mohsen Ghasemi <[email protected]> Added tests and created new vSwarm invoker Fixed linting Fixed VSwarm Invoker for Knative mode Committer: aryans1204 [email protected] Fixed VSwarm Invoker for Knative mode Added new invoker for vSwarm functions and added tests Signed-off-by: aryans1204 <[email protected]> Fixed formatting under grpc_client.go Signed-off-by: aryans1204 <[email protected]> Fixed driver tests Signed-off-by: aryans1204 <[email protected]> Fixed linting and slimmed VSwarm tests Signed-off-by: aryans1204 <[email protected]> Fixed linting errors Signed-off-by: aryans1204 <[email protected]>

leokondrashov · 2025-01-02T02:24:44Z

pkg/driver/clients/grpc_client.go

I think there is no point in touching this file at all. You swapped a couple of lines and renamed one of the imports, nothing substantial.

leokondrashov · 2025-01-02T02:36:24Z

pkg/workload/standard/workload.go

+type vSwarmServer struct {
+	helloworld.UnimplementedGreeterServer
+}
+func (s *vSwarmServer) SayHello(_ context.Context, req *helloworld.HelloRequest) (*helloworld.HelloReply, error) {


This is used only for tests. Please remove everything except the return statement in the body and move this together with StartVSwarmGRPCServer to the respective test file.

Please re-read previous comments, I've mentioned slimming down this part and moving to the test file several times already.

leokondrashov · 2025-01-02T02:40:54Z

pkg/driver/clients/grpc_vswarm_client.go

+	record.Instance = extractInstanceName(response.GetMessage())
+	if strings.HasPrefix(response.GetMessage(), "FAILURE - mem_alloc") {
+		record.MemoryAllocationTimeout = true
+	} else {
+		record.ActualMemoryUsage = common.Kib2Mib(0)
+	}


Does this make sense with the messages that the relay sends? Here, we rely on our workload to fill the message with instance ID or failure message. Does the relay do the same? I doubt that

leokondrashov · 2025-01-02T02:52:53Z

pkg/driver/clients/grpc_client_test.go

+
+}
+func TestVSwarmClientUnreachable(t *testing.T) {


The blank line should be between the closing brace and new function, not inside the previous function. Please format carefully and look at all of your changes for similar inconsistent formatting.

leokondrashov reviewed Oct 31, 2024

View reviewed changes

aryans1204 requested a review from leokondrashov November 7, 2024 10:32

leokondrashov reviewed Nov 8, 2024

View reviewed changes

aryans1204 force-pushed the new-vswarm-invoker branch 2 times, most recently from 39cdf02 to 4c372fb Compare November 13, 2024 16:38

aryans1204 requested a review from leokondrashov November 13, 2024 16:39

leokondrashov reviewed Nov 15, 2024

View reviewed changes

aryans1204 force-pushed the new-vswarm-invoker branch 2 times, most recently from 197fbbc to af79127 Compare November 30, 2024 01:51

aryans1204 requested a review from leokondrashov November 30, 2024 01:52

aryans1204 force-pushed the new-vswarm-invoker branch from af79127 to df997aa Compare December 1, 2024 10:58

leokondrashov reviewed Dec 4, 2024

View reviewed changes

aryans1204 force-pushed the new-vswarm-invoker branch 3 times, most recently from c447b94 to 46a4c3b Compare December 13, 2024 12:19

aryans1204 requested a review from leokondrashov December 13, 2024 12:21

aryans1204 force-pushed the new-vswarm-invoker branch from 46a4c3b to f3b274d Compare December 24, 2024 05:32

leokondrashov reviewed Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweaked invoker and added tests #540

Tweaked invoker and added tests #540

aryans1204 commented Oct 30, 2024

leokondrashov Oct 31, 2024

leokondrashov Nov 8, 2024

leokondrashov Jan 2, 2025

leokondrashov Oct 31, 2024

leokondrashov commented Oct 31, 2024

leokondrashov commented Nov 8, 2024

leokondrashov Nov 8, 2024

aryans1204 commented Nov 13, 2024

leokondrashov left a comment

leokondrashov Nov 15, 2024

aryans1204 Nov 30, 2024

leokondrashov Dec 4, 2024

leokondrashov left a comment

leokondrashov Dec 4, 2024

aryans1204 Dec 13, 2024

leokondrashov Dec 4, 2024

aryans1204 Dec 13, 2024

leokondrashov Dec 4, 2024

leokondrashov Jan 2, 2025

leokondrashov Jan 2, 2025

leokondrashov Jan 2, 2025

leokondrashov Jan 2, 2025

Tweaked invoker and added tests #540

Are you sure you want to change the base?

Tweaked invoker and added tests #540

Conversation

aryans1204 commented Oct 30, 2024

Summary

Implementation Notes ⚒️

External Dependencies 🍀

Breaking API Changes ⚠️

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

leokondrashov commented Oct 31, 2024

leokondrashov commented Nov 8, 2024

Choose a reason for hiding this comment

aryans1204 commented Nov 13, 2024

leokondrashov 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

leokondrashov 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