search

alexellis / derek

Reduce maintainer fatigue by automating GitHub
https://github.com/alexellis/derek/blob/master/USER_GUIDE.md
MIT License
808 stars 70 forks source link

Move to OpenFaaS golang-http-template - part 2 #96

Open alexellis opened 5 years ago

alexellis commented 5 years ago

Expected Behaviour

I would like to move Derek to the OpenFaaS golang-http-template so that the process can stay alive for longer without re-forking. This would allow access tokens etc to be cached in memory to reduce API requests and will increase memory under load.

It is also a step towards #62 (break out SDK) so that Derek can be run on other platforms or by other processes other than the OpenFaaS watchdog to enable wider use.

Current Behaviour

Each request runs in a new process meaning that panic and os.Exit() are fair-game to appear anywhere in the code. This prevents the above, so part of the work is refactoring any exiting and error handling.

Possible Solution

Template:

golang-http via: https://github.com/openfaas-incubator/golang-http-template

The initial work will be moving to the new template signature, this should be minimal, then from there testing that we never call panic/os.Exit.

The current Dockerfile is unnecessary at this point, since it could be replaced by the original Golang template. https://github.com/alexellis/derek/blob/master/Dockerfile

cheikhshift commented 5 years ago

Would it be possible to keep panics and catch them by deferring a call to function recover? I searched through the code, there are os.Exit invocations in vendor packages and 4 within Derek

alexellis commented 5 years ago

It would be possible, but I think we could change the code as part of this instead. Can you link to the instances (with line number) please?

cheikhshift commented 5 years ago

These are the calls to os.Exit within Derek, there are more from vendor packages :

https://github.com/alexellis/derek/blob/master/main.go#L34

https://github.com/alexellis/derek/blob/master/main.go#L40

https://github.com/alexellis/derek/blob/master/main.go#L47

https://github.com/alexellis/derek/blob/master/main.go#L55

alexellis commented 5 years ago

Thanks for the research.

Do you want to try doing this issue first? https://github.com/alexellis/derek/issues/97

For the line numbers you can click the line on GitHub's UI and then copy/paste the URL.

cheikhshift commented 5 years ago

Sure, I updated my previous comment with links to the corresponding line.

alexellis commented 5 years ago

@cheikhshift are you still working on this? 😄

cheikhshift commented 5 years ago

Of course, here is the OpenFaaS template I wrote a while back : (if ti looks good we can proceed with it. :D )

provider: name: faas

gateway: http://127.0.0.1:80 # can be a remote server

functions: derek: handler: ./fn image: alexellis/derek:0.6.4-rc2 lang: go environment: debug: true customers_url: https://raw.githubusercontent.com/alexellis/derek/master/.CUSTOMERS validate_hmac: false validate_customers: true secret_path: /var/openfaas/secrets/ # use /run/secrets/ for older OpenFaaS versions environment_file:

The contents in folder 'fn' has one file. (fn is an abbreviation of function) The content of the file is a modified version of the current main.go file. Let me know if the template looks Ok, I'll submit another pull request.

On Mon, Jun 3, 2019 at 11:51 AM Alex Ellis notifications@github.com wrote:

@cheikhshift https://github.com/cheikhshift are you still working on this? 😄

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alexellis/derek/issues/96?email_source=notifications&email_token=ABGGN3TVZP3LTK3QJPFK7LDPYUAUNA5CNFSM4F7VIYMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWZFA5Q#issuecomment-498225270, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGGN3XBM6N4C2CUJTVOEPDPYUAUNANCNFSM4F7VIYMA .

alexellis commented 5 years ago

Can you propose a PR?

Alex

cheikhshift commented 5 years ago

Sure.

Cheikh.

On Jun 3, 2019, at 2:46 PM, Alex Ellis notifications@github.com wrote:

Can you propose a PR?

Alex

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cheikhshift commented 5 years ago

I reviewed the issue, this is what I'm going to work on :

Since I've mentioned HTTP responses, OpenFaas should have a helper package, to provide a sort of translation of how OpenFaaS interfaces with HTTP requests. This may reduce friction for users coming from a RESTful API background.

alexellis commented 5 years ago

Thanks for your take on the changes. We have a REST-compatible template called golang-http and golang-middleware both are available via faas-cli template store list and would have the benefit of keeping Derek's process alive between requests.