Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

Support of Roslyn C# Scripting #758

Closed
wants to merge 6 commits into from
Closed

Support of Roslyn C# Scripting #758

wants to merge 6 commits into from

Conversation

DTXMania
Copy link

Issue Ref: None

First at all, I'm not goot at English, sorry.

Description:

CSharpX (Roslyn C# Scripting) runtime supports C# script (.csx), not C# (.cs).

Excample

using Kubeless.CSharpX;

string Hello( Event ev, Context ctx )
{
    return "Hello world!";
}

using microsoft/aspnetcore:2.0

  • Roslyn 2.8.0
  • .NET Core 2.0.8

To add dependencies, add a .csproj file.

Please see samples for details.

Thanks

TODOs:

  • Ready to review
  • Automated Tests
  • Docs

@andresmgot
Copy link
Contributor

Thank you for submitting this PR! There is an ongoing effort to refactor the current .NET/csharp runtime here: #697. @allantargino Can you review this PR and check possible conflicts?

@allantargino
Copy link
Contributor

Hey @DTXMania ! That's a great approach to support .csx (like Azure Functions). I will review the PR and check that, @andresmgot. 👍

@DTXMania
Copy link
Author

Thanks
I think that .NET is a great environment, I hope we will have more scenes to use it.

@KyleGobel
Copy link

KyleGobel commented May 18, 2018

Hey, this is really cool.

One thing i'm noticing in this as well as the other dotnetcore runtime is the use of the full blown aspnet mvc pipeline.

I don't think any of that is needed for any of this, I feel like we could remove all those dependencies, skip the pipeline entirely and just use some simple middleware. Skipping the mvc pipeline would make this faster and the code a bit more simple in my opinion.

Small example doing everything in just the Startup.cs (could move this to a KublessMiddleware class though to keep it organized if we want).

public class Startup
{
    // constructor stuff ommitted

   public void ConfigureServices(IServiceCollection services)
   {
     // any services we'll need (probably not necessary)
   }

   public void Configure(IApplicationBuilder app, IHostingEnvironment env)
   {
       app.Use(next => ctx => { ctx.Request.EnableRewind(); return next(ctx); });
       app.Map("/healthz", builder =>
       {
           builder.Run(async context =>
           {
               await context.Response.WriteAsync("Ok");
           });
      });

     // this is static per container?  we could just do this once?
     // not sure if kubeless changes the env variables, or if we need to check them on 
     // every request but we could do this on the actual request if needed
     var runnerContext = new Context();
     app.Run(async (context) => 
     {
          var modulePath = $"/kubeless/{runnerContext.ModuleName}.csx";
          var functionCode = await File.ReadAllTextAsync(modulePath, context.RequestAborted);

         // you get the picture, rest of the code here
       
        httpContext.Response.StatusCode = 200;
        await httpContext.Response.WriteAsync(result, probablyALinkedCancellationToken);
     });
   }

Also, I believe in this one as well as the other dotnetcore one we're not using any of the requestaborted cancellation tokens?

I'm not sure if kubeless or the outside world will be cancelling the requests, but we can handle those cases, the HttpContext carries a cancellation token of RequestAborted for that purpose, we could link that with our function timeout token easy enough.

var timeoutToken = new CancellationTokenSource(TimeSpan.FromSeconds(timeoutDuration));
var requestAbortedToken = context.RequestAborted;

var linkedToken = CancellationTokenSource.CreateLinkedTokenSource(timeoutToken.Token, requestAbortedToken.Token);

// any async work could use linked token
 
await scriptResult = await CSharpScript.EvaluateAsync<string>(/* parama */, linkedToken.Token);

Just some food for thought.

Anyway great work, excited to see these.

DTXMania added 3 commits May 19, 2018 16:36
# Conflicts:
#	examples/Makefile
#	kubeless-non-rbac.jsonnet
#	pkg/langruntime/langruntime.go
@allantargino
Copy link
Contributor

Hi @KyleGobel,
This suggestion make a lot of sense. Removing the whole mvc pipeline and changing it for one or two middlewares would be better.
About the CancellationTokenSource, loved the idea. To be honest, I didn't know the HttpContext carries a cancellation token. It would be easy then to adapt the current code to use both tokens - since today we only use one for timeout.
Would you like to send a PR with these changes? We would appreciate your contribution on these optimizations!

@allantargino
Copy link
Contributor

allantargino commented May 19, 2018

Hey @DTXMania!

Your csx runtime is great :) but today they have most of the same base code for the current .NET core runtime. I was thinking if maybe we could use a single source code for both "runtimes", since they make lot of sense to be together.

The controller/middleware code from both of them are almost the same and implements all features (FUNC_PORT, FUNC_TIMEOUT , etc). Thinking about maintaining these 2 runtimes (specially when new features have to be developed), we would be duplicating code most of the times.

Also, the model layer for our users would be benefit if we use a single project. Today, we are have a Kubeless.Functions package published on nuget (Check this out). The models are basically the same you have:

public class Context
{
   public string ModuleName { get; }
   public string FunctionName { get; }
   public string FunctionPort { get; }
   public string Timeout { get; }
   public string Runtime { get; }
   public string MemoryLimit { get; }
}
public class Event
{
    public object Data { get; }
    public string EventId { get; }
    public string EventType { get; }
    public string EventTime { get; }
    public string EventNamespace { get; }
    public Extensions Extensions { get; }
}

So they could use the same nuget package and namespace:

using Kubeless.Functions;

To integrate that on the current runtime, you should move your execution code to an implementation of the IInvoker interface:

using Kubeless.Functions;
using System.Threading;

namespace Kubeless.Core.Interfaces
{
    public interface IInvoker
    {
        object Execute(IFunction function, CancellationTokenSource cancellationSource, Event kubelessEvent, Context kubelessContext);
    }
}

Today, the current implementation for .cs files is:

public object Execute(IFunction function, CancellationTokenSource cancellationSource, Event kubelessEvent, Context kubelessContext)
{
    // Gets references to function assembly
    var assembly = Assembly.Load(function.FunctionSettings.Assembly.Content);
    Type type = assembly.GetType(function.FunctionSettings.ModuleName);

    // Instantiates a new function
    object instance = Activator.CreateInstance(type);

    // Sets function timeout
    cancellationSource.CancelAfter(_functionTimeout);
    var cancellationToken = cancellationSource.Token;

    // Invoke function
    object functionOutput = null;
    var task = Task.Run(() =>
    {
        functionOutput = InvokeFunction(function, new object[] { kubelessEvent, kubelessContext }, type, instance);
    });

    // Wait for function execution. If the timeout is achived, the invoker exits
    task.Wait(cancellationToken);

    return functionOutput;
}

So in this proposal, you would need to wrapper the following code inside this interface:

var scriptResult = await CSharpScript.EvaluateAsync<string>(
					code,
					ScriptOptions.Default
						.WithReferences( new[] {
						typeof( Event ).Assembly,
						typeof( Context ).Assembly,
						} ),
					_globals,
					typeof( Globals ), linkedToken.Token );

You could named it somethink like CsxInvoker, and we could rename the current one to CsInvoker

@allantargino
Copy link
Contributor

allantargino commented May 19, 2018

On startup.cs of the current .NET Core runtime, we could use two approaches:

Approach 01: Build two different docker images, with a single invoker hardcoded per image:

  • Image for .cs:
[...]
            services.AddSingleton<IInvoker>(new CsInvoker(timeout * 1000)); // seconds
[...]
  • Image for .csx:
[...]
            services.AddSingleton<IInvoker>(new CsxInvoker(timeout * 1000)); // seconds
[...]

Approach 02: Build a single docker image, and dispatch the right Invoker at runtime:

  • Image for both .cs and .csx:
[...]
            //Something like:
            if(functionExtension==".cs")
                services.AddSingleton<IInvoker>(new CsInvoker(timeout * 1000)); // seconds
            else
                services.AddSingleton<IInvoker>(new CsxInvoker(timeout * 1000)); // seconds
[...]

Since both runtimes use .csproj to manage dependencies, they could use a single init image without problems. The current one for .NET Core simple does a dotnet restore and store the .dlls at /kubeless directory.

@allantargino
Copy link
Contributor

This is a really important decision to take guys!
I would vote to merge both into one, regarding the specific implementation you choice.
What do you think? @DTXMania @KyleGobel @fcatae @andresmgot

@allantargino
Copy link
Contributor

There are many performance changes we could perform together on the .NET Core runtime, but before that, this decision is key to the next steps and the roadmap.

{
public class Extensions
{
public HttpRequest Request { get; }
Copy link
Contributor

@allantargino allantargino May 19, 2018

Choose a reason for hiding this comment

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

Really great you implementation that adds HttpRequest, HttpContext and HttpResponse to Extensions 👍

@@ -0,0 +1,14 @@
#r "/kubeless/packages/newtonsoft.json/11.0.2/lib/netstandard2.0/Newtonsoft.Json.dll"
Copy link
Contributor

@allantargino allantargino May 19, 2018

Choose a reason for hiding this comment

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

We could ask the user to reference just the dll name and we could add the whole path. What do you think?

@@ -724,3 +724,28 @@ get-java-deps:
get-java-deps-verify:
kubeless function call get-java-deps --data '{"hello": "world"}'
kubectl logs -l function=get-java-deps | grep -q '.*Hello.*world! Current local time is:'

get-csharpx:
kubeless function deploy get-csharpx --runtime csharpx2.8.0 --from-file csharpx/helloget.csx --handler helloget.Hello
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you include tests with dependencies?

case strings.Contains(runtime, "csharpx"):
command = appendToCommand(command,
"export HOME="+installVolume.MountPath,
"dotnet restore --packages "+installVolume.MountPath+"/packages > /dev/termination-log 2>&1")
Copy link
Contributor

@allantargino allantargino May 19, 2018

Choose a reason for hiding this comment

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

I had many problems regarding using a non-root user inside Kubernetes on microsoft/aspnetcore-build:2.0 image. Were you able to make this base image to work?
This image is using a user with uid=1000 (instead of the default root user). Since it is the first time this 1000 user is using dotnet restore command, it will create a .nuget folder on the home directory of this user. The problem is that the home folder for this user is / - which it doesn't have any permissions to create directories there.
To overcome this barrier, I previously create the directory and move it. Check the Dockerfile:

FROM microsoft/aspnetcore-build:2.0.0

WORKDIR /app

COPY kubeless.csproj .

RUN dotnet restore

RUN mv /root/.dotnet /root/.nuget /

RUN chown 1000 /.dotnet/ /.nuget/

// evaluate function
await Console.Out.WriteLineAsync( $"{DateTime.Now}: Evaluate: MOD_NAME:{_globals.KubelessContext.ModuleName}, FUNC_HANDLER:{_globals.KubelessContext.FunctionName}" );

var scriptResult = await CSharpScript.EvaluateAsync<string>(
Copy link
Contributor

@allantargino allantargino May 19, 2018

Choose a reason for hiding this comment

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

This is beautiful 👍
Support both .csx and .cs really would open the doors for people who is both developing traditional applications or migrating their scripts/Azure Functions!

@KyleGobel
Copy link

Casting my vote I would say using one docker image, and determine the IInvoker instance at runtime.

To me this seems like the easier approach, especially if we can get it to something like your example (just an if statement). I like the idea of it being controlled by the runtime itself, versus some other area in kubeless.

Unless there are some advantages to the multiple docker image approach that i'm not seeing

@DTXMania
Copy link
Author

Thank you for many reviews!

hmm...
It seemes like CSharpX is unnecessary....

we can replace entirely .csx by .cs.
There is no reason to distinguish between .csx and .cs in Kubeless.

@KyleGobel
Copy link

KyleGobel commented May 20, 2018

There is no reason to distinguish between .csx and .cs in Kubeless.

Can you clarify this?

I think it's still necessary (well...nice to have), I think @allantargino was just proposing that this functionality be pulled into the single dotnetcore runtime.

I thought the Roslyn scripting stuff (typically .csx extension) supports a couple other things that a standard c# class library project doesn't?

  • loading references via, #r, loading other files with #load (not actually sure if this is supported out of box or we have to add capabilities for this).
  • the ability to write just functions, similar to python or node.
  • lower barrier to entry and using C# in kubeless (less code required by end user)
  • azure functions uses something very similar making migration to and from easier. (I believe they also use the .csx extension) EDIT: apparently Azure support both, scripting files and class library projects (Azure Function Class Libraries)

Also looking towards future (maybe not for kubeless but at least for myself with a custom runtime), adding some ability to add custom directives (nuget reference loading as a simple example)

#nuget "Newtonsoft.Json"
using Newtonsoft.Json;

string MyFunction(Event e, Context c) {
    return JsonConvert.SerializeObject(e);
}

Essentially have the runtime doing some simple string parsing to find #nuget (or whatever) directives and dynamically downloading package and loading/referencing them.

While yes the extension of the file doesn't really matter, detecting the extension is a good way to see if the runtime needs to compile the script or new up/call a method in an assembly. There's value to having both paths in my opinion.

@andresmgot
Copy link
Contributor

I am closing this PR as stale. If someone is interested on continuing this effort please check the new documentation at https://github.com/kubeless/runtimes/blob/master/DEVELOPER_GUIDE.md. We are accepting runtimes for the incubator state :)

@andresmgot andresmgot closed this Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants