-
-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal] Runtime changes #1078
Conversation
runtime/Makefile
Outdated
php layers/tests.php | ||
function: | ||
# Build the base image individually so that the extensions can be built using it | ||
docker-compose -f docker-compose.compile.yml build php-base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to use and try docker compose (docker compose 2.0 next gen coded in go vs python for the first one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a good hello world that I can check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And maybe the new version wouldn't suffer the docker compose issue you had
🎉 thank you @deleugpn for starting this huge work. Simplifying the build process will let us be more confident, and spend less time maintaining all that. One thing that might be worth adding to the TODO list (and early in the list preferably) is testing performances. And by that, I mean (mostly?) checking out the size of the generated layer: is it bigger/smaller than the same layer on master? Related to that: is there any positive/negative impact (or an opportunity) on Bref's composer package size? (since it's included in all applications, it is part of the cold start time as well) I'm asking this because this PR looks into copying files into the layer directly: is there any file we can exclude from publishing to Composer to lighten the package? Maybe not, I'm just curious ^^ |
I just realized that doing that too early is a bit misleading because we don't have all extensions in the image yet. |
I believe Amazon backport any critical security patches to packages from their repos, will check our Yum logs shortly to confirm if that appears to be the case as can't find anything to confirm online. Update: This doesn't appear to be the case after all and this would be prohibitive for my personal usage of Graviton/ARM sadly |
b34f27a
to
57ddd0b
Compare
runtime/configuration/bref-ext.ini
Outdated
extension=/opt/php-modules/dom.so | ||
extension=/opt/php-modules/exif.so | ||
extension=/opt/php-modules/fileinfo.so | ||
extension=/opt/php-modules/ftp.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 I doubt that everybody wants to load ftp extension. Would be nice to give the choice between a "batteries included" with all extensions loaded, and a "minimal" profile
with for example only opcache for example, so everybody's in charge then to load their required extensions then.
@mnapoli we already had this discussion in the past when I wanted to make bref as lightweight as possible and I'm still agree with you that the entry level user of Bref should have all batteries included, yet it should be possible to push forward, as loading fewer extentions worth considering it also reduce the attack surface of the λ.
Let's take only dom extension. For the record it's disabled by default on Ubuntu 20, especially because it relies on libxml... which is unfortunately known to have a very impressive CVEs record track 💥 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, removing extensions right now would be a breaking change, so I don't think it's worth addressing in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it's ok to have every of these core extensions available in the layer FS, but not loaded by default with the "minimal" profile. Because as the layers limit is set to 5, we cannot put every ext into a dedicated layer otherwise every power user will reach such limit quickly.
I just did a rebase and pushed my latest modifications to the project infrastructure. The project now has 1 Makefile for each PHP Version + 1 Makefile at the root of the Runtime for parallel build and publishing. There's more code duplication now, but also easier to replicate and evolve together with new PHP Versions. A new version can easily be obtained by copy/pasting the php folder and doing a search/replace with the PHP Version. Older versions are easier to delete as it only requires removing their entire folder + their declaration on the root Also, I dialed back in dividing things into multiple Dockerfile as they ended up being more confusing than easier to navigate. The current file structure is almost a top-down build process. It only "fails" to be top-down once it reaches the FPM Layer which then requires jumping a bit back so that the FPM Layer won't have unnecessary files that were loaded into the Function layer. The "How does it work" on the first post has been updated and the replication of layers in your own AWS account is sketched. Things still left to do:
|
It seems like |
thanks @t-richard I had forgotten about those. They're ready now. |
Sadly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For schema, would be nice to use drawio for example. Thus saving under a .drawio.png extension and you can then edit back into drawio later on as it saves all metadata used to produce the png inside png headers
@shouze that's a good tip and I didn't know that! However, I'm THE WORST when it comes to visual representations and I really like whimsical because they make it easy for me to make "pretty-ish" charts. Maybe that's something that the community might feel like contributing if we manage to land these changes |
@deleugpn I've just seen this post, maybe interesting you 😉 |
The first set of ARM Layers for prototyping are finally here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉
I did a first pass of review, awesome job @deleugpn! Most of my comments are about getting the "why" of everything written down (to ease maintenance down the road).
General feedback: could we get a comment at the top of most scripts/config files that explain what they are for?
Another general comment: the wording "php-module" confuses me. I'm more used to see these called "extensions", is the name "module" the name used by remi-collect?
Next steps for me will be reviewing the "embedding" of Bref's source code in the images, I haven't managed to wrap my head around this but will get there!
@@ -32,20 +44,27 @@ class ServerlessPlugin { | |||
// and guarantees to return a fully resolved form (even if property is configured with variables) | |||
const region = options.region || await resolveConfigurationProperty(['provider', 'region']); | |||
|
|||
// Allows compatible Bref Deployment with custom layers built by the user. | |||
const account = process.env.AWS_ACCOUNT ?? '209497400698' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use a unique variable like BREF_LAYERS_ACCOUNT
?
|
||
// TODO: | ||
// const arm64 = this.path.resolve(__dirname, 'layers.arm64.json'); | ||
// const layers_arm64 = JSON.parse(this.fs.readFileSync(filename)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the file exists and contains layers, should we uncomment this?
How stable is this?
@@ -0,0 +1,48 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly this file is autogenerated?
If so it might be worth adding a big-ass comment at the top that mentions this and encourages contributors to not modify it? (and point instead to the docs/script they should look at instead)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to be, but through several iteractions it ended up much simpler and not really worth generating it in case of region changes.
error_reporting = E_ALL & ~E_DEPRECATED & ~E_STRICT | ||
|
||
memory_limit=3008M | ||
zend_extension=/opt/php-modules/opcache.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line used to be zend_extension=opcache.so
which prompts me to wonder: any reason to put extensions in /opt/php-modules
and not the previous extension directory that is automatically picked up by PHP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHP has an order of folder scan to load extensions. I believe we can still make it work with just the extension name here, but during iteractions of it I found it much easier to debug if I know exactly which folder was expected to have the extension and why it crashed / was not loaded.
By placing just the extension name we end up relying on PHP looking on multiple folders in the PATH before giving up.
Let me know if I should remove the full path and leave just the relative path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I just read your comment again and I noticed I didn't fully address it.
So, when we were compiling PHP, we could choose some paths that would be available for scanning PHP extensions. This is no longer true for installing from a distribution. Remi Collet and Amazon Linux have different configuration betweem themselves (by default), so that would mean we would need different config files for ARM and x86. Furthermore, Amazon Linux places extension files under /etc/...
and we can't overwrite that from AWS Lambda.
I did replace this with /opt/php-extensions/
.
"cloudwatch": { | ||
"region": "EU_WEST_1" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to keep that file around.
@@ -0,0 +1,116 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For simplicity and avoiding unexpected breaks (e.g. https://runtimes.bref.sh) could we keep this file as layers.json
? (defaults will be x86 anyway)
### The php{xx} folder | ||
|
||
This is the heart of Bref Runtime Layers. We configure php using the `config` folder to store `php.ini` files that | ||
will be copied into `/opt/php-ini`. Note that we configure `/opt/bootstrap` to execute PHP with `PHP_INI_SCAN_DIR=/opt/php-ini:/var/task/php/conf.d/`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory seems to have changed compared to master right?
I think we need to keep loading ini
from the same directories (IIRC it was /opt/bref/etc/php/conf.d/
), I don't think we gain anything by changing and that avoids BC break (e.g. for https://github.com/brefphp/extra-php-extensions or any custom layer already existing)? (was there a reason to change?)
runtime/plugins/console/bootstrap.sh
Outdated
# Fail on error | ||
set -e | ||
|
||
export PHP_INI_SCAN_DIR="/opt/php-ini/:/var/task/php/conf.d/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On master we didn't need a bootstrap.sh
file (the bootstrap
file was a PHP file directly) -> do we need it now to set the PHP_INI_SCAN_DIR
variable?
I'm guessing we are now forced to set PHP_INI_SCAN_DIR
manually because we don't compile PHP anymore, is that the reason? Is there any other way we can set PHP_INI_SCAN_DIR
once? (I don't think there is, but just making sure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replied to this comment here: https://github.com/brefphp/bref/tree/runtime/runtime#bootstrapsh-file and I also added some comments to the file itself, let me know what you think
runtime/plugins/console/Dockerfile
Outdated
@@ -0,0 +1,30 @@ | |||
FROM alpine:3.14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused by the plugins
word -> the console
layer is a normal layer (unless it changed here?), I don't think we should have a concept of plugins, that's one more concept to keep track off? We might as well have this layer in runtime/
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with the console
layer is that it's not 1 layer per PHP version and it's also not one layer per CPU architecture. But in any case this was an old idea that ended up adding more complexity than necessary. I deleted the entire folder.
Co-authored-by: Marco Deleu <deleugyn@gmail.com>
Co-authored-by: Marco Deleu <deleugyn@gmail.com>
Co-authored-by: Marco Deleu <deleugyn@gmail.com>
Co-authored-by: Marco Deleu <deleugyn@gmail.com>
👋 hi, I played a lot with
I saw this PR after preparing this as an issue, but instead I post my message directly here I'm open to help / do an example with the master base if you want 🙂 |
@mykiwi what does buildx bake bring? I'm not familiar with it, trying to figure out what's next for this refactoring. |
This small documentation explain what is aim https://github.com/docker/buildx/blob/master/docs/guides/bake/index.md |
Hey everyone, I have picked up this PR and I am working in a different repository (with all these changes). More details here: #1299 |
Closing, all these commits have not been lost, they are in https://github.com/brefphp/aws-lambda-layers 🎉 We're getting closer to Bref 2.0, thank you @deleugpn for the fantastic work! More details in #1299 |
This pull request is an ongoing work with an attempt to drastically change the Runtime system.
Why?
It's an attempt to make things easier to change. Although it was triggered by AWS releasing Graviton support on Lambda, this pull request isn't entirely about supporting Graviton, but in fact making it easier and more accessible to build, test and reproduce the layers provided by Bref.
What are the changes?
StealTake inspiration from the folks at Vercel (Source)What does it mean for new PHP releases?
Although compiling PHP means we're in control of compiling point releases as soon as they come, remi-collect has a great track record of releasing new version on the same day (or even the day before) a new version is announced on the PHP internal list. The downside is that Remi doesn't provide aarch64 support, which means we would need to rely on
amazon-linux-extras
for Graviton support. Currently Amazon only provides PHP 8.0.8 (about 4 months behind). My take is that it's better to provide delayed aarch64 than not provide at all or provide it using emulators or in a hard-to-maintain process. Given that Apple is pushing arm64 with the new M1 laptops and AWS is pushing it via Graviton support, it's a matter of time until there's better release cycle around arm64.How does it work?
The project structure is divided into
common
,phpxx
,tests
and the root folder.The
common
folder contains files for the fpm layer, function layer and the scripts for publishing the layers. The FPM and Function layers are built by usingcomposer install
and generating avendor
folder that will be embedded and available inside the Lambda Layer.The
php74
,php80
andphp81
folders are suppose to be incredibly identical and easy to test and replicate. Each PHP Folder has it's own Makefile so that contributors canmake function
and publish their own layer in their own account. This allows for easy testing for layers modifications.The
tests
folder contains assertions that will be executed inside the container of each layer so that we can "prove" that the system works.Finally, the root folder contains a Makefile and a docker-compose file that allows for parallelization of building all layers / php versions and publishing them on all regions.
Special thanks to Aran Reeks for teaching me about
ldd
to show linked libraries for the php binary and its extensions.What is the
/opt/bref-internal-src
folder?This is an attempt to make PHP on Lambda easier to onboard and test. It allows to replicate the experience of other Runtime. You go to the AWS Console, create a new Function, attach Bref's layer to it, write a
hello.php
file and run it. No need forcomposer
orserverless
. Just plain old PHP. It greatly improves the User Experience when testing things out.How can I test this myself?
The biggest motivation for this pull request is to make it easier for anyone/everyone build their own layer so that you can test things out yourself. Making it more reproducible / accessible will help contributors changing the layer to test things. Here are the steps:
These steps will publish a new layer into your own AWS Account so that you can test them.
Comparison
Checklist
CI / CD
make everything
publishing all layers in parallelExtensions
Important Notes
Currently the process is blocked by docker/compose#8804 for graviton support.