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

Add ballerina runtime support #788

Merged
merged 10 commits into from
Jun 6, 2018
Merged

Conversation

anuruddhal
Copy link
Contributor

@anuruddhal anuruddhal commented May 31, 2018

Issue Ref: #744

Description:
Add Ballerina runtime support to kubeless.

[PR Description]
This PR contains implementation, examples, and integration tests for ballerina runtime.

TODOs:

  • Ready to review
  • Automated Tests
  • Docs

@anuruddhal anuruddhal changed the title Add ballerina runtime Add ballerina runtime support May 31, 2018
@sebgoa
Copy link
Contributor

sebgoa commented May 31, 2018

A short documentation would be great.

@anuruddhal
Copy link
Contributor Author

Added documentation with commit dad7d86.

@anuruddhal
Copy link
Contributor Author

anuruddhal commented Jun 4, 2018

@sebgoa
Ballerina has Prometheus compatible endpoint to collect metrics. Will Kubless be able to collect data if I enable it?
https://ballerina.io/learn/how-to-observe-ballerina-code/

@sebgoa
Copy link
Contributor

sebgoa commented Jun 4, 2018

@anuruddhal if you check the python runtime you will see that we use a prometheus client to collect metrics:

https://github.com/kubeless/kubeless/blob/master/docker/runtime/python/kubeless.py#L20

So yes, if you collect metrics in ballerina and expose them via a /metrics route kubeless will be able to use it for auto-scaling.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

thank you for the PR @anuruddhal, great job! I sent a few comments.

Regarding the Prometheus integration we can do like Ruby and PHP, add the Golang proxy to redirect requests (see the Dockerfile and the Makefile build task). That way we can easily get timeout handling (that I think is missing from the current implementation) and Prometheus statistics. This proxy redirect incoming requests to localhost:8090 so that port 8090 should be hardcoded in the ballerina runtime.

kubeless:Event event = {};

//Read environment variables and set them to context
context.function_name = config:getAsString("FUNC_HANDLER");
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 move the context initialization outside the handler? So it doesn't need to execute this code every time the function is called.

docs/runtimes.md Outdated

Ballerina functions should import the package `kubeless`. This [package](../docker/runtime/ballerina/kubeless/kubeless.bal) contains two types `Event` and `Context`.

When using the Ballerina runtime, it is possible to provide the `ballerina.conf` file. The values in conf file is available for the function.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't use the flag --dependencies for other thing than installing external dependencies. Since ballerina doesn't require a different file to do so I think it's better to just ignore this flag. If people wants to use a configuration file they can bundle their files in a zip that will be uncompressed.

{
"name": "ballerina0.970.1",
"version": "0.970.1",
"runtimeImage": "kubeless/ballerina:0.970.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

push the images to a temporary repository just to do the first testing, we can push them later on to the kubeless repository.

@@ -240,6 +240,8 @@ func (l *Langruntimes) GetBuildContainer(runtime, depsChecksum string, env []v1.
case strings.Contains(runtime, "java"):
command = appendToCommand(command,
"mv /kubeless/pom.xml /kubeless/function-pom.xml")
case strings.Contains(runtime, "ballerina"):
command = appendToCommand(command, "ls /kubeless/ballerina.conf")
Copy link
Contributor

@andresmgot andresmgot Jun 4, 2018

Choose a reason for hiding this comment

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

I would throw an error here to make it clear that ballerina doesn't require this file. something like:

return v1.Container{}, fmt.Errorf("Ballerina does not require a dependencies file")

@@ -309,6 +311,16 @@ func (l *Langruntimes) GetCompilationContainer(runtime, funcName string, install
"cp /kubeless/*.java /kubeless/function/src/main/java/io/kubeless/ && " +
"cp /kubeless/function-pom.xml /kubeless/function/pom.xml 2>/dev/null || true && " +
"mvn package > /dev/termination-log 2>&1 && mvn install > /dev/termination-log 2>&1"
case strings.Contains(runtime, "ballerina"):
command = fmt.Sprintf(
"mkdir -p /kubeless/kubeless/ && mkdir -p /kubeless/func/ && "+
Copy link
Contributor

Choose a reason for hiding this comment

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

mkdir -p accepts more than one argument: mkdir -p /kubeless/kubeless/ /kubeless/func/

@anuruddhal anuruddhal force-pushed the master branch 2 times, most recently from 76ca944 to aefb48e Compare June 5, 2018 08:30
@@ -315,6 +315,7 @@ func (l *Langruntimes) GetCompilationContainer(runtime, funcName string, install
command = fmt.Sprintf(
"mkdir -p /kubeless/kubeless/ /kubeless/func/ && "+
"cp -r /ballerina/files/kubeless/*.bal /kubeless/kubeless/ && "+
"ls -altr && "+
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this was for debug?

@anuruddhal anuruddhal force-pushed the master branch 5 times, most recently from a0e1053 to e74f3a7 Compare June 6, 2018 07:24
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

This looks good to me now, thank you again for the contribution!

@andresmgot andresmgot merged commit b06c647 into vmware-archive:master Jun 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.

3 participants