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

improve performance of invoke when using docker #7178

Open
wants to merge 9 commits into
base: master
from

Conversation

@richarddd
Copy link
Contributor

richarddd commented Jan 6, 2020

What did you implement

This PR improves the performance of invocation though docker with a few tweaks:

  • The packaging is now only done if no package already exists which dramatically improves performance
  • Docker build files are now cached per runtime

The invoke local command also accepts -p or --package to supply a custom package directory

Closes #5410 (kind of)

How can we verify it

sls create --template aws-nodejs --path myService

serverless.yml

functions:
  hello:
    handler: handler.hello
    events:
      - http: GET hello
sls invoke local --function hello --data \"{\"resource\":\"/\",\"path\":\"/hello\",\"httpMethod\":\"GET\"}\" --docker --package ./.serverless

Todos

Useful Scripts
  • npm run test:ci --> Run all validation checks on proposed changes
  • npm run lint:updated --> Lint all the updated files
  • npm run lint:fix --> Automatically fix lint problems (if possible)
  • npm run prettier-check:updated --> Check if updated files adhere to Prettier config
  • npm run prettify:updated --> Prettify all the updated files
  • Write and run all tests
  • Write documentation
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 6, 2020

Codecov Report

Merging #7178 into master will increase coverage by 0.13%.
The diff coverage is 55.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7178      +/-   ##
==========================================
+ Coverage   87.94%   88.08%   +0.13%     
==========================================
  Files         236      236              
  Lines        8611     8659      +48     
==========================================
+ Hits         7573     7627      +54     
+ Misses       1038     1032       -6
Impacted Files Coverage Δ
lib/plugins/aws/invokeLocal/index.js 75.78% <55.81%> (-1.22%) ⬇️
...ckage/compile/events/apiGateway/lib/permissions.js 87.5% <0%> (-5.36%) ⬇️
lib/plugins/package/lib/packageService.js 86.06% <0%> (-1.44%) ⬇️
lib/plugins/aws/package/compile/functions/index.js 96.73% <0%> (ø) ⬆️
lib/utils/resolveCliInput.js 100% <0%> (ø) ⬆️
...plugins/aws/package/compile/events/stream/index.js 98.82% <0%> (+0.02%) ⬆️
lib/plugins/aws/lib/updateStack.js 98.11% <0%> (+0.07%) ⬆️
lib/plugins/aws/lib/naming.js 97.81% <0%> (+0.08%) ⬆️
lib/utils/downloadTemplateFromRepo.js 96.63% <0%> (+0.41%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41d7d0b...5cd498f. Read the comment docs.

Copy link
Member

medikoo left a comment

@richarddd great thanks for that. Still I don't think we should take that in.

So far experience is that it's current version as is in code put for invocation, while (if I understand correctly this proposal) after taking this in, it'll be version that was packaged with latest deployment that'll be put to invocation.
I'm pretty sure it'll be unexpected and shortly we will receive bug reports. It's also not on pair with how local invocation works when no docker image is involved.

I'm aware that packing of lambdas performs below expectations at this point. We plan to address it in near future. Follow #6580 for notifications on that

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 8, 2020

@richarddd great thanks for that. Still I don't think we should take that in.

So far experience is that it's current version as is in code put for invocation, while (if I understand correctly this proposal) after taking this in, it'll be version that was packaged with latest deployment that'll be put to invocation.
I'm pretty sure it'll be unexpected and shortly we will receive bug reports. It's also not on pair with how local invocation works when no docker image is involved.

I'm aware that packing of lambdas performs below expectations at this point. We plan to address it in near future. Follow #6580 for notifications on that

Thanks for your input @medikoo. I've updated the PR so that packaging is ignored if you specify the -p or --package argument. That way users can opt-out of packaging.

In regards to making the packaging faster, it still won't be fast enough as large projects with bundling/minification could take up to several minutes to complete.

Copy link
Member

medikoo left a comment

I've updated the PR so that packaging is ignored if you specify the -p or --package argument.

Thanks @richarddd, good idea, still I think no packaging should be opt in and not opt out, as otherwise we're introducing a breaking change, and I think such decision should always be implicit (as there's a high chance of service code not being up to date with what's currently packaged).

Also --package CLI param is already used for pointing alternative package folder path (this name is not very good, but we can't revert from it without major bump, I've opened proposal to change it: #7190).

I suggest then to name this option --skip-package

@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 9, 2020

Thanks @medikoo! Changed now to --skip-package

Copy link
Member

medikoo left a comment

@richarddd thanks, it looks really good! Just few minor concerns and we should be good to go

lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
richarddd added 2 commits Jan 10, 2020
@richarddd

This comment has been minimized.

Copy link
Contributor Author

richarddd commented Jan 10, 2020

@medikoo i did some additional changes now:

  1. layers which point to an archive is now unpacked and included in image (this caused issues before, only "path" layers where working
  2. The contents of the newly generated Dockerfile and a cached copy are compared. If the content is the same, there is no need to build the image again (the build is pretty slow, takes a few seconds)
  3. Use "decompress" package rather than the zip magic which was before (it broke symlinks in layers etc)
Copy link
Member

medikoo left a comment

Thank you @richarddd, those were a good calls! I've added some remarks

lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
() =>
new BbPromise((resolve, reject) => {
this.serverless.cli.log('Building Docker image...');
const docker = spawn('docker', [

This comment has been minimized.

Copy link
@medikoo

medikoo Jan 13, 2020

Member

It'll be good to rely on child-process-ext/spawn, it's a promised wrapped spawn, we already use

This comment has been minimized.

Copy link
@richarddd

richarddd Jan 15, 2020

Author Contributor

This is a dev dependency only and used in tests/scripts only so will keep original node spawn :)

This comment has been minimized.

Copy link
@medikoo

medikoo Jan 15, 2020

Member

It is a normal dependency, ensure to be up to date with master :)

This comment has been minimized.

Copy link
@richarddd

richarddd Jan 16, 2020

Author Contributor

In this case, however, where we don't rely on the output and listen for exit in most cases it kinds of defeats the purpose :) Please check the updated PR :)

This comment has been minimized.

Copy link
@medikoo

medikoo Jan 16, 2020

Member

it kinds of defeats the purpose

Not really, it provides promise support and eventual error debugging out of a box. So there's need for creating promise wrapper if you use that.

We definitely should rely on it.

This comment has been minimized.

Copy link
@richarddd

richarddd Jan 17, 2020

Author Contributor

@medikoo I started implementing this but it caused a lot of issues, and a lot of stuff broke in tests etc where child process is stubbed. I gave it a try but I can't get it right. Please check out my branch https://github.com/richarddd/serverless/tree/use-child-process-ext which is based.

Maybe we could merge this and then do another pr with your suggested optimization or you could take a look at my code and figure out what I'm doing wrong? ☺️

This comment has been minimized.

Copy link
@medikoo

medikoo Jan 17, 2020

Member

Indeed as it's function that's directly exported you can't stub with sinon easily. But you can do it via proxyquire, as e.g. it's done here:

'child-process-ext/spawn': spawnStub,

This comment has been minimized.

Copy link
@richarddd

richarddd Jan 17, 2020

Author Contributor

I gave that a try as well with no success :( The spawn function returned by use-child-process-ext is a promised wrapped and we cant simply stub its in:

'child-process-ext/spawn': spawnStub,

I just have a feeling this PR is getting out of scope:

// TODO: the following tests need to be refactored since they use mockRequire in a breaking way

This comment has been minimized.

Copy link
@richarddd

richarddd Jan 20, 2020

Author Contributor

@medikoo could you give me a hit of what im doing wrong here? I'm not very experianced with sinon:

writeChildStub = sinon.stub();
endChildStub = sinon.stub();
stdinStub = sinon.stub().resolves('');
spawnStub = sinon.stub().returns({
  stderr: new EventEmitter().on('data', () => {}),
  stdout: new EventEmitter().on('data', () => {}),
  stdin: {
    write: writeChildStub,
    end: endChildStub,
  },
  on: (key, callback) => {
    if (key === 'close') process.nextTick(callback);
  },
  then: () => {},
});
AwsInvokeLocal = proxyquire('./index', {
  'get-stdin': stdinStub,
  'child-process-ext/spawn': spawnStub,
});

This comment has been minimized.

Copy link
@medikoo

medikoo Jan 20, 2020

Member

If you need to read output of the spawned process. Then promise resolves with it. See the doc: https://github.com/medikoo/child-process-ext/tree/60e5781d2c848186c9727e8c638afdee73bcb3a6#spawncommand-args-options

Stub that provides command output can be configured as:

AwsInvokeLocal = proxyquire('./index', {
  'get-stdin': stdinStub,
  'child-process-ext/spawn': sinon.stub().resolves({
    stdoutBuffer: new Buffer("Mocked output")
  });
});
lib/plugins/aws/invokeLocal/index.js Outdated Show resolved Hide resolved
richarddd added 3 commits Jan 16, 2020
…o improve-invoke-docker-performance
@softprops

This comment has been minimized.

Copy link
Contributor

softprops commented Jan 17, 2020

It might be out of scope for this or but I did some investigation and that by simply changing the directory docker was invoked from (the docker build context) I was able to get the docker build time down from 38 seconds to
1 #6895 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.