Skip to content
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

Move packaging process into own plugin #1486

Closed
5 tasks done
pmuens opened this issue Jul 5, 2016 · 46 comments
Closed
5 tasks done

Move packaging process into own plugin #1486

pmuens opened this issue Jul 5, 2016 · 46 comments
Milestone

Comments

@pmuens
Copy link
Contributor

pmuens commented Jul 5, 2016

The packaging process should be moved into an own plugin.

Status:
Done --> See #1494

This includes:

  • Zipping up a whole service
  • One service equals one zip file which includes all the necessary code (all functions)
    • Magic handler support will be dropped (as it's not necessary anymore)
  • There will be no checksum to check if something has changed
    • The zipped up service will be redeployed every time (this ensure that the build "will always work" as no brittle steps as e.g. checksum support will be implemented)
  • Possibility to set a path to an own artefact (.zip file) via a definition in the serverless.yaml file (e.g. artefact_path). This way the deployment plugin will use the user specified artefact and won't zip up everything on it's own

Questions:

  • Do we need an own lifecycle in the deploy plugin? Or should we use before:deploy:deploy?

Current implementation:
The current implementation can be found in the awsDeploy plugin.

Open issues:

@pmuens pmuens added this to the v1.0.0-alpha.2 milestone Jul 5, 2016
@pmuens pmuens self-assigned this Jul 5, 2016
flomotlik referenced this issue Jul 5, 2016
Add zipping of handler directories. Add a walkDirSync method which collects all the
file paths in a directory (needed for the .zip file creation for "magic handlers") in a synchronous way.
@flomotlik
Copy link
Contributor

@pmuens imho we should have a package step. This makes sure other plugins can hook in there as well.

And the way to let following plugins know which artefact path they should use could simply be a configuration value in the serverless.yml, e.g.

artefact_path:

By default people don't set it, the packaging plugin zips and puts this somewhere to be picked up, but if users want to build their own artefact they can and if the artefact_path variable is present the packaging plugin will simply do nothing.

@pmuens
Copy link
Contributor Author

pmuens commented Jul 5, 2016

@flomotlik Sound reasonable.

Should the package lifecycle be included in the deploy plugin (deploy:package) and an own package plugin which hooks into this lifecycle and implements the packaging process?

Or should we define a package plugin which has a package:package lifecycle and the packaging implementation?

@flomotlik
Copy link
Contributor

Imho has to be in deploy, otherwise it won't run as people would have to run package:package directly. but we can implement a separate package command in the future imho.

@pmuens
Copy link
Contributor Author

pmuens commented Jul 5, 2016

Yep. Missed that package:package yields to the fact that we need an own command.

Here's how this might look like:

lifecycleEvents: [
  'initializeResources',
  'createProviderStacks',
  'compileFunctions',
  'compileEvents',
  'createDeploymentPackage',
  'deploy',
],

Not sure if we should name it createDeploymentPackage or just package (I like names that speak for themselves 😆 )

@flomotlik
Copy link
Contributor

createDeploymentPackage sounds good to me

@license2e
Copy link
Contributor

@pmuens @flomotlik the only downside I see to this is the size of zip file... For example, one of the current functions we have written in Java with all the jars is 38MB, and the limit is 50MB...

Would we have to separate out into different services and then keep track which lambda function has which handler?

There doesn't seem a logical way to separate out the functions and services...
We have different department (companies), languages and aws services.

Also, it would seem like function type would have to be per services not per function, is that correct?

@flomotlik
Copy link
Contributor

@license2e Is that only one function or are those 38MB combining code for multiple functions? We do want to provide a way in the future for you to build function specific packages, would be great to understand a little more how you're reusing code in services.

@license2e
Copy link
Contributor

Unfortunately one function, although, its currently pulling in ALL AWS SDK jars, so I will have to tweak the gradle zip settings to only pull in the ones that are actually used...

@eahefnawy
Copy link
Member

While I like the simplicity this introduces (for us and for our users), this will make deployment exponentially slow, and will eat up lots of space both on S3 and Lambda, which is the main reason why we went with magic handlers in the first place.

If a service has 10 functions, we'll end up deploying the entire service 10 times. Magic handlers allows users to organize their functions and their dependencies in such a way to minimize including code that is not used by the lambda/function. If all functions will have the same code footprint (most of which is not used by that function), then a service is eventually a big fat function.

@license2e
Copy link
Contributor

@eahefnawy I agree, with the setup we have and the number of functions we are writing ~1000, we will need those magic handlers. Otherwise, we will have to arbitrarily group certain functions together into a service, and manually keep track of which service has which function.

@ac360
Copy link
Member

ac360 commented Jul 7, 2016

At first, I was nervous about this. This potentially introduces problems which a lot of people complained about in V.0. However, I think they can be resolved, and this could ultimately be a step in a better direction.

Concerns:
  • Slow deployments due to large zip sizes (add in the already slower CF deployments)
  • Slower cold-starts due to large zip sizes.
  • Users hitting their Lambda size limits. This was a huge issue that many complained about.
  • Packaging up dependencies may be unnecessary since some FaaS providers install them upon deployment
Solutions:
  • It's possible all of these can be resolved with a great excludes/ignore feature, and it sounds like you guys are already on that path.

On a side note, I personally favor package instead of createDeploymentPackage, or possibly build, which might be a more common and accurate term. createDeploymentPackage feels unnecessarily long, package is short and conveys simplicity.

@license2e
Copy link
Contributor

@ac360 I am still nervous about this, for exact concerns you listed above, and for the folder structure that we will have to do.

@pmuens
Copy link
Contributor Author

pmuens commented Jul 7, 2016

Hey guys,
thanks for jumping in on this discussion and thank you for the great feedback so far.

The main reason we're doing this is to provide a more general way of packaging which can be used for every provider and every language.
Our current implementation (in v1) will loop through all the functions, zip up each function and uploads it to S3. After that the CloudFormation resource for the lambda file will point to the corresponding package.

With this change we will zip up the service (which is basically a composition of functions) once, upload it to S3 and let all functions point to the one, service based .zip file.

The .zip file might be a little bit bigger but at the end of the day it's just one .zip file which still contains the functions compared to multiple .zip files which will contain one function per file (so size should be "nearly" the same).

Another important thing this introduces is the fact that we'll introduce a new lifecycle plugin authors can hook into. So you can modify the package before it gets deployed.

We're also heavily investing in good exclude / include patterns (as @ac360 mentioned). Those will be provided out of the box for each language but you'll still be able to modify them as you'd like.

We'll test the new package plugin in a real world scenario to see how it performs.

I like the package / build naming as it's definitely more common. Will update the plugin ASAP.

@ac360
Copy link
Member

ac360 commented Jul 7, 2016

Thanks for the clarity @pmuens.

Now that I understand this better, and that all functions in a service point to a single zip, this change results in a massive problem. It removes the ability of a function to be the unit of deployment.

By having multiple functions point to the same zip file, you will be forced to deploy functions in bulk if they are in a service, and not deploy/update them individually.

The function as the unit of deployment is one of the much touted benefits of the serverless architecture. It leaves devs in a the most agile position. This change would make that impossible for services containing multiple functions. To fix one function, you would have to deploy other functions as well, risking those breaking. For example, usersCreate needs to be fixed, but in order to fix it, I have to redeploy usersShow, usersUpdate, usersDelete too? That's crazy. What if these other functions are in a broken state? This unnecessarily creates a monolithic deployment pattern for a microservice architecture.

Further, this wouldn't allow excludes to be specified at the function level, so function file sizes would always be much bigger than they need to be. Also, this would make the entire “optimizer” plugin completely unable to work, since it concatenates files based on each function handler, so that plugin would be made worthless.

Overall, the costs of this approach appear to greatly outweigh the benefits, and it clearly diverges from serverless architecture principles. The current implementation, though slower, is much better, as it retains the "function as the unit of deployment" philosophy, especially when we add an option to target a specific function within a service to deploy. Also, a new package event could still be introduced into the current implementation (see V.0).

Recommended Steps Forward:

  • Keep current implementation
  • Add in a package step for each function
  • Add in a --function option for the deploy command, to target a specific function

@pmuens
Copy link
Contributor Author

pmuens commented Jul 7, 2016

@ac360 thanks for the detailed explanation / the upsides and downsides of this approach.

I understand the upsides and downsides of this approach.

Could @flomotlik and @eahefnawy chime in on this?

@eahefnawy
Copy link
Member

@ac360 adds good points here. It seems that we're returning to the traditional monolithic servers approaches. For example, a bug in function A would cause function B to break since they all share the same buggy code/zip file. That's not what lambda/serverless was made for imo. Magic handlers and dependency management gives users the choice however they want to architect their apps. Maybe they could use GraphQL if they wanna be monolithic, but at least the function would still be isolated from any potential issues in other functions.

Also, after reading @pmuens clarification above, I think I initially misunderstood the proposition. If I understand this correctly now, we'll zip and deploy the service only once and just simply set a different handler for each function, but ultimately all functions will point to the same zip. Is that correct? In that case you can discard what I mentioned about slowness and having deploy x times. I thought the same zip would be deployed for each function.

Ultimately, I love the simplicity of this approach, but I'm still a bit worried that users won't have flexibility they had in v0 with deploying their functions and choosing their dependencies.

@ac360
Copy link
Member

ac360 commented Jul 7, 2016

Further drawbacks of the new approach:

  • To update the version of one function in a service, all of function versions would also have to be updated. This complicates/confuses versioning by creating lots of versions where the function hasn't actually changed, making versioning less relevant.
  • How would individual function rollback work? Would that be impossible now? If it is possible, it would be more complicated given that many versions would contain unchanged functions.

This new approach still seems like a huge divergence, that disables core functionality and goes against best practices. Strongly in favor of bringing back support for single function deployment.

Also, @eahefnawy I'm not as concerned about magic handlers as before, since zipping things up at the service level is smaller still than the project level, especially if you can specify files to exclude at the function level. I would be OK w/ delaying magic handlers and waiting to see if that is needed within the service-level. Maintaining functions as the unit of deployment is the immediate priority, imo.

@flomotlik
Copy link
Contributor

Imo functions are not the unit of deployment. We're helping teams build services. Those services are the main entity we're deploying for users. When functions get deployed to production from a CI/CD environment deploying functions independently (and even not deploying functions sometimes) can be very dangerous:

  • Shared Code might not get updated
  • New dependency versions might be installed that need to be pushed
  • Updated external configuration might not be up to date, e.g. If we're implementing something like Secrets that are stored in DynamoDB encrypted and get loaded when the function is started, not deploying a new version would mean old ones stay hot and the system could get into a failed state.

While Hashing can potentially resolve some of those issues it introduces a lot of complexity in our deployment, can be the source of huge problems when there are bugs and also isn't necessarily the most efficient way for us to deploy. When we only build one zip file, push it to S3 and then deploy it through Cloudformation we only have to do one upload and Cloudformation can use that artefact for all deployed lambda functions.

Deploying functions independently is important for the development workflow, as users will want to deploy independently during development. But not from a CI/CD perspective and this is what we need to implement first, then we can improve.

But most importantly why we're not deploying independently is that while it does work for Node, it simply won't work for other languages. For Java for example building separate zip files for each function, with only the code necessary for that function is practically impossible.

Individual Rollback would still be possible with versions available in lambda, but imho individual rollback is something we should absolutely discourage, as this can very very easily lead to issues with different expectations implemented in different functions and the whole system then failing (e.g. different code expects different fields in the database to be available).

If users need to create specific deployment files for specific functions (e.g. 4 of 5 functions can live with the same zip, but function 5 needs to include a few additional things) we can (and I assume will) absolutely implement this.

Those reasons are why we decided we're going to move the packaging out of AWS, generalise it and implement it in a way that makes more sense in CI/CD and makes our code much simpler.

@license2e
Copy link
Contributor

@flomotlik can you elaborate on this:

If users need to create specific deployment files for specific functions (e.g. 4 of 5 functions can live with the same zip, but function 5 needs to include a few additional things) we can (and I assume will) absolutely implement this.

Are you saying that it can still live within the service, but would have a different deploy command or setting (eg. in the functions:function5:standalone = true)? Or are you saying that it would just have to be a separate service altogether?

@flomotlik
Copy link
Contributor

@ryansb yeah I think that is the best way, though I would also call it package.

The way I see this going forward (and we've discussed it before the implementation) is that by default you have one package for each function, but you can use the same include/exclude parameters on each function as well. Every function that doesn't have include/exclude/artifact parameters will use the default package, every other function will get its own artefact.

One thing I would like to get more feedback on is if all of you consider this as a common use case, or rare optimisation. Personally I think single function artefacts are an optimisation on the general package that gets created for the whole service. So for the few functions or services where you need tighter control you can get it, but my assumption is that for most functions you don't need it.

So from an implementation perspective here is how I see this going forward:

  • when deploy is called packaging plugin checks each function for include/exclude and artefact config
  • if there are functions without include/exclude/artefact config it also checks the service wide config
  • Packages up the artefacts according the include/exclude patterns defined for each function or service wide
  • If an artefact config is set up include/exclude patterns are ignored (should we throw an error then? Imho at least we should print a warning, but not an error)
  • The packaging plugin sets the artefact config in the JS Object
  • The deployment plugin picks up all the artefacts for each function (or the general one where no artefact is set), deploys them and updates the Cloudformation stack
  • Deployment happens by pushing artefacts to S3 and then Cloudformation

One of the changes we also need to do in the packaging plugin is how many artefacts we keep around. Currently we only keep the last 5 versions inside of the S3 bucket. We should keep that, but we need to scope it per function, so we keep the last 5 around for each artefact (service wide and function specific) so we don't just remove random old artefacts.

/cc @erikerikson @eahefnawy @pmuens

Also going forward we're going to change how we deal with issues that are still being discussed. In the past we closed them when we implemented something but kept the discussion going. From now on we're going to label them discussion and keep them open. Didn't want to make it seem like we're closing the discussion with closing the issue, so I'm reopening this now.

@flomotlik flomotlik reopened this Jul 13, 2016
@ryansb
Copy link
Contributor

ryansb commented Jul 13, 2016

I'd consider it a roughly 80/20 issue: 80% of serious users will never need this split function, but for 20% of serious users it would be a total dealbreaker to not have it, and they'd need to hack something together or skip using the framework. The per-function include/exclude in addition to service-level ones would cover the use case I was discussing, and it seems to fit well in the overall design.

IMO, for the case where an artifact is set up, but so are include/exclude, I think it should be a hard error because that's definitely not going to do what they intend.

@erikerikson
Copy link
Member

@flomotlik it almost sounds like you're trying to optimize for disk space and single payment upload time. The more constrained resource seems to be cold-start loading time.

This seems like it risks forcing users to think about and get into the depths of how packages and deployments and container persistence work.

This reminds me a bit of the everything is lambda centered choice earlier in the SLS lifecycle. It is an external concept/artifact brought in because something that seemed like a bright idea. It had the effect of blocking use cases as well as generating confusion and issues. One package per function seems to reflect the thing that the SLS is managing more directly.

Can you help me grok what goal(s)/value you're trying to serve or problem(s) you're trying to avoid?

@flomotlik
Copy link
Contributor

@erikerikson the main goals are:

  • Faster deployment (because we only have to upload one asset for a service, or at least less than by creating separate packages for each function)
  • Easier sharing of code as all code in a service will simply be shared with other functions
  • be more service than function focused. We think of functions as part of a service and those functions will have dependencies between each other so sharing code/artefacts between them should most of the time be ok
  • Flexibility in creating artefacts so you can create separate artefacts per function, but you don't have to
  • Simplicity so people don't have to think what goes into which artefact, they contain everything by default and you simply remove what isn't necessary

/cc @HyperBrain as well

@erikerikson
Copy link
Member

erikerikson commented Jul 13, 2016

Thanks Flo.

Bullets 1-5:

  1. 👍 cool stuff and laudable goal
  2. I guess I don't see the true cost or savings to this. The exact same physical bits versus the exact same logical bits is a noop via the analog to digital conversion that make our information as physical machines paradigm possible. As a user there is no greater ease here if I am ignorant (and this is possible) of how my code is shared. As a disk manager there is a bit more ease but I don't think optimizing for disk space is valuable these days.
  3. This is an example of the extra bit I mentioned. Services generally share domain specificity but that may or may not imply code sharing and it seems you're adding a constraint that risks introducing user complexity.
  4. I do appreciate this point because it will enable my use case but I then think outside of myself and ask about the general case. En mass, equivalent results will be accomplished but more bits will be repeatedly pushed around the provider infrastructures and each provider will have an increase of cold start times due to increased bits being shuffled and needing to be correct, et cetera. Unless you have all providers declaring and agreeing that they have optimizations that can take advantage of a shared packaging scheme, I think this has undesirable consequences at scale.
  5. This epitomizes what seems wrongheaded to me (sorry): people are thinking about functions and their relationships to data stores and other event sources, not the artefacts and packaging that are the concrete enablers of the systems they are building out of those logical components. Maybe I am wrong headed (and, please, feel free to say so - I'd rather become more right than have been right) but I think serverless and the SLS toolset have their big win in leveling up the perspective that users take by abstracting and simplifying any details that aren't increasing their focus on what the software they are writing is meant to accomplish. Things like packaging and servers and whatnot are infrastructure concerns, not system enablers.

@flomotlik flomotlik modified the milestones: v1.0, v1.0.0-alpha.2 Jul 14, 2016
@ac360
Copy link
Member

ac360 commented Jul 15, 2016

Last weekend, I tried out the new packaging process by deploying services with 1-20 functions. Overall, I really enjoyed it.

Out of the box, CF deployments are slower, but this change makes CF deployments faster. It took 30 seconds to update 20 functions when they are in a single service, and each had their own API G endpoint. That's much faster than V.0 can do. With fewer functions in a service, provisioning via CF takes the same amount of time, and is a bit slower than V.0, however uploading them (a large time cost depending on the size of your functions) will be faster. On average, V.1 deployments will likely be a bit slower than V.0. But I think that's a good trade-off because V.1 deployments are all CF and are much safer. We used to run into weird collision issues with teams deploying to the same Lambda function simultaneously and trying to publish unique aliases.

I'm still concerned about updating multiple functions at once, but it seems like we can offer single function deployments, when necessary. So that's resolved. Versioning is another concern. We'll be creating a lot of unnecessary Lambda versions if all functions of a service are always updated, even though their code may be unmodified.

@flomotlik Has anyone put thought into versioning the actual CloudFormation stacks instead? We can still update the Lambda versions, but perhaps relying on CF Stack versions might offer a better experience. Given that a serverless service is code + resources, having versioning and rollback ability at the CF Stack level seems simple and powerful. We could easily download the previous versions from S3 and redeploy them (since their packaged w/ their serverless.yml files!) to offer a safe rollback experience.

@erikerikson Great comments, as always. I actually believe there is a lot of alignment between @flomotlik's stated goals and what you are describing. Overall, we're offering a better default experience, while still maintaining a high degree of flexibility here. So, I think there is room for solutions to the points you brought up.

@ryansb Great suggestions/solutions. Thank you!

@flomotlik
Copy link
Contributor

Has anyone put thought into versioning the actual CloudFormation stacks instead?

Not yet, but I like it. We could simply create a folder per deployment and then push artefacts and CF files into it.

@ac360
Copy link
Member

ac360 commented Jul 15, 2016

What we used to do with project buckets in JAWS is append the stage and version to the S3 key, instead of using subfolders. We also put all Lambda zips in a subfolder within the bucket root, just in case people wanted to put other stuff in the bucket.

bucketRoot
  - functions
       - usersCreate-dev-12.zip
  - other

@flomotlik
Copy link
Contributor

@ac360 we now only create one bucket per service/stage/region combination, but in there we could simply create folders for the different version and put all the files in

@ac360
Copy link
Member

ac360 commented Jul 15, 2016

@flomotlik Right! But, if we add the ability to use a single bucket across multiple services (this feature might have already been added, not sure), keeping the stage in the name will protect us in that case.

@flomotlik
Copy link
Contributor

@ac360 the most likely option for a single bucket across services is that people manage that bucket themselves imho. I see basically 2 options going forward for the buckets:

  1. We create the bucket for you and name it together with your CF stack. We guarantee naming and that everything works
  2. Users create their own bucket and point us to it (e.g. deployment_bucket setting in their serverless config) so we're going to push to that one

This might change if we get different feedback from users

@doapp-ryanp
Copy link
Contributor

Funny how things come full circle :) I'm with @flomotlik 's last comment. Thats exactly how I had it architected in JAWS (could pass S3 bucket to use as CLI opt when creating project).

Only thing I'd add is when your naming the bucket, please include the region name in it. Ex: uswest2-sls-projName-UUID. As you know when interacting with S3 u need to know what region it is in. Having region name in bucket makes things so much easier to manage when a human needs to get involved.

@erikerikson
Copy link
Member

Yar!

2 is a must have for us. We appreciated that we would give bucket ${BUCKET} and the artifacts would be placed in ${BUCKET}/Serverless/..., separated as appropriate by region/environment*/et cetera.

Having region name in bucket makes things so much easier

By convention, not mandate, yes/please?

  • prefer "environment" (standard industry language) over "stage" (ApiGateway naming artifact)

@HyperBrain
Copy link
Member

One question: Is the use of aliases and named versions for publishing lambda functions to environments (stages) still an option, instead of naming the lambda functions? For us the number of functions will explode if every environment gets its own function.

Supporting aliases and versions should be possible with CloudFormation using the AWS:Lambda:Version and AWS:Lambda:Alias CF types. With version even versions with custom names (maybe implied through the user's build system) may be used and the environment ( @erikerikson +1 for the naming ) can be easily attached as alias to that.

@eahefnawy
Copy link
Member

@HyperBrain its dropped for now. But I see your concern. Does your usecase require handling stages with aliases? or we could just add support for aliases/versions in general and that could work for you?

The issue with handling stages with aliases/versions that we've found over the months is that not all event sources and other AWS services are aware of this Qualifier or versions/aliases. Didn't sound super reliable.

Plus, when you deploy to a stage, you gotta navigate to that alias/version, and it confused some people who didnt see their deployment right away.

@flomotlik any thoughts on this?

@HyperBrain
Copy link
Member

@eahefnawy Since the latest improvements in the AWS lambda web console, it is far more centric on aliases than on versions. e.g. you now have the prominently placed "Qualifier" selection button in there.

For us, we heavily depend on the stages/aliases as we use the Qualifier property for our lambda calls.
Regarding the event sources. In my experience this is the exception. For these few that do not support aliases, the previously used "customName" override was a perfect solution where you could change the semantics quite easily.

@eahefnawy
Copy link
Member

@HyperBrain Perfectly said 😊 ... let's see what @flomotlik thinks of this.

@flomotlik
Copy link
Contributor

Yup Alias support is definitely an important one, we do have an issue open, I just can't seem to find it right now :D

@pmuens
Copy link
Contributor Author

pmuens commented Jul 28, 2016

@flomotlik we have an issue for Lambda versioning here: #1457

@flomotlik
Copy link
Contributor

Closing this issue as #1777 is targeting the per function packaging and @johncmckim is going to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants