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

PHP runtime #581

Merged
merged 36 commits into from
Feb 15, 2018
Merged

Conversation

paolomainardi
Copy link
Contributor

@paolomainardi paolomainardi commented Feb 2, 2018

Issue Ref: None

Description:

PHP runtime implementation, still a working in progress.

[PR Description]

TODOs:

  • Make php controller to handle function timeout (maybe using something like pthreads.org)
  • Ready to review
  • Automated Tests
  • Docs

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 working on this @paolomainardi ! I sent a few early comments.

}

/**
* Heltz route.
Copy link
Contributor

Choose a reason for hiding this comment

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

small typo: Healtz route

Copy link
Contributor

Choose a reason for hiding this comment

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

healthz even

function foo() {
print "hello world";
}

Copy link
Contributor

@andresmgot andresmgot Feb 5, 2018

Choose a reason for hiding this comment

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

we should have another example in which the data given in the request is used somehow.

Also you should add in the Makefile of the examples/ folder tasks to deploy and verify these examples. Note that there should be tasks there to verify that setting a custom port and timeouts work as expected.

Those tests will be executed in travis if you add them in the file ./tests/integration-tests.bats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks @andresmgot, i'm going to add it in the next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andresmgot added a test with data and added to integration-tests.bats

@rakesh-garimella
Copy link
Contributor

You would need to modify the langruntime.go and langruntime_test.go(see [PR #575 ]) We have moved the information about init images into kubeless-config.

@paolomainardi
Copy link
Contributor Author

ok @rakesh-garimella

@paolomainardi paolomainardi changed the title WIP: PHP runtime PHP runtime Feb 6, 2018
get-php-deps:
kubeless function deploy get-php-deps --trigger-http --runtime php7.1 --handler hellowithdeps.foo --from-file php/hellowithdeps.php --dependencies php/composer.json
echo "curl localhost:8080/api/v1/proxy/namespaces/default/services/get-php-deps/"

get-php-deps-verify:
kubeless function call get-php-deps | egrep "hello world"
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of printing hello world for the test you can check the logs of the pod:

kubectl logs -l function=get-php-deps | egrep ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right, let me change it.

@paolomainardi
Copy link
Contributor Author

@andresmgot the MR can now be evaluated

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 @paolomainardi ! I just sent a few comments/questions.

{
try {
$this->app->any('/', [$this, 'root']);
$this->app->any('/healtz', [$this, 'healtz']);
Copy link
Contributor

Choose a reason for hiding this comment

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

the path is /healthz the h is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks

@@ -0,0 +1,17 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

is this file needed? (it seems for debugging)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed and removed


post-php-verify:
kubeless function call post-php --data '{"it-s": "alive"}'| egrep "it.*alive"

Copy link
Contributor

Choose a reason for hiding this comment

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

we need one last test to check that the timeout is working as expected. Something like:

timeout-php:
	$(eval TMPDIR := $(shell mktemp -d))
	printf '<?php\n function foo() { while(1) {} } }\n' > $(TMPDIR)/hello-loop.php
	kubeless function deploy timeout-php --trigger-http --runtime php7.2 --handler helloget.foo  --from-file $(TMPDIR)/hello-loop.php --timeout 4
	rm -rf $(TMPDIR)

timeout-php-verify:
	$(eval MSG := $(shell kubeless function call timeout-php 2>&1 || true))
	echo $(MSG) | egrep Request.timeout.exceeded

(you need to add this test to the integration-tests.bats file as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added!


return $response;
} else {
sleep($this->timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

does this let the parent process hanging? is it possible to serve other requests while this is "sleeping"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To achieve that i had to use nginx because the php integrated webserver is just a plain single threaded process.

} else {
sleep($this->timeout);
posix_kill($pid, SIGKILL);
throw new TimeoutFunctionException();
Copy link
Contributor

Choose a reason for hiding this comment

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

does this throw the exception even if the process were already finished at the time of posix_kill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the request dies when parent or child return a response

@paolomainardi
Copy link
Contributor Author

@andresmgot the php image has been updated using both php-fpm+nginx, we can't rely just on PHP webserver as it is just one single thread process.

@murali-reddy
Copy link
Contributor

@paolomainardi thanks for your patience in addressing all comments/suggestions. Overall looks good.

I think integration tests are almost complete. Would you mind adding a test similar to get-python-update to test the function update functionality? Should not take much of your time.

Here are some gaps I would like an issue opened so that someone can work on them or atleast we have issue to track pending items.

  • Support for custom port is missing.
  • Support for Prometheus metrics
  • Support for scheduled and Kafka support

@paolomainardi
Copy link
Contributor Author

@murali-reddy update tests added and CI is green, thanks.
I would like to work on the issues you listed, do you think that can be merged in the meanwhile ?

Copy link
Contributor

@murali-reddy murali-reddy left a comment

Choose a reason for hiding this comment

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

LGTM, pending issues can be addressed in separate PR's.

@murali-reddy murali-reddy merged commit 5844c40 into vmware-archive:master Feb 15, 2018
@murali-reddy
Copy link
Contributor

I would like to work on the issues you listed, do you think that can be merged in the meanwhile ?

Great if you can raise PR for pending issue.

@deckchairlabs
Copy link

I would have thought using something like this would be more suited for PHP https://github.com/php-pm/php-pm instead of having an nginx process running.

@andresmgot
Copy link
Contributor

@deckchairlabs I am not a PHP expert but it seems that php-pm uses Nginx as well under the hoods. It seems that it provides some performance benefits when using it with PHP frameworks or PHP-FPM (which is not our case AFAIK). Is there any specific advantage you were thinking of? We can raise a new issue and tackle that.

@paolomainardi
Copy link
Contributor Author

Hi @deckchairlabs, as @andresmgot which specific advantage you were thinking off ? I choosed nginx+php-fpm which is a well known and production grade solution for deploying php apps, i knew php-pm (as well reactphp) but it solves specific use cases.

@deckchairlabs
Copy link

PHP-PM can be run standalone, and does not need the addition of a webserver. I'll see what I can come up with using this PR as a starting point, and will open a new PR with my results. I guess the advantage I can think of is that it would more like NodeJS, whether or not that is a good thing..... we shall see.

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.

6 participants