-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Speed up fig up #586
Speed up fig up #586
Conversation
if (do_build and | ||
self.can_be_built() and | ||
not self.client.images(name=self.full_name)): | ||
self.build() |
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 block was moved from _get_container_create_options()
, which is mostly focused on building up an options dict. It feels a lot more appropriate here, since this function is doing other docker operations related to creating the container.
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 guess I forgot to mention this is backwards compatible, the flag defaults to the old behaviour. |
Would be nice to get this in. Needs a rebase now, though. |
1a1a650
to
367c3fd
Compare
367c3fd
to
2f80545
Compare
Rebased (and fixed new tests that used the old |
2f80545
to
b51b175
Compare
Another rebase |
b51b175
to
0048ab9
Compare
This branch seems to be pretty prone to conflicts. Got any feedback on this branch @aanand, see anything concerning that might delay a merge? |
LGTM @aanand any opinions on the new option? |
LGTM The new option sounds sensible to me. In future (as you outline in #693) we might make Once you've rebased, feel free to merge it straight away. |
Signed-off-by: Daniel Nephin <dnephin@gmail.com>
…ady freshly built. Signed-off-by: Daniel Nephin <dnephin@gmail.com>
0048ab9
to
3056ae4
Compare
We've noticed an interesting issue with wercker. If you set up wercker on your own fork, it won't run for pull requests on the upstream repo. I just set that up the other day, and now it's running this PR here (https://app.wercker.com/#buildstep/54a445852ca6bc215572b0b0), but I don't think it ever gets linked up to this PR. |
The build passed. I also sent some feedback to wercker about the issue. I wasn't able to find a bug tracker, so I just used the feedback form on their site. |
There is a shiny new jenkins instance for the docker project now, so we might be able to use that. https://jenkins.dockerproject.com/ |
Speed up fig up Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
I did some profiling of one of our larger fig setups. I noticed a lot of time wasted on an unnecessary
build()
. This branch adds a flag to disable thebuild()
that was called duringfig up
. The slowdown wasn't on thebuild()
itself, but the conditionalif not client.images()
, because it requires hitting the docker remote again, and filtering through the images.In almost every use of fig I perform a
fig build
before afig up
already, to ensure thatfig up
is actually running the latest code/containers, so this extra check is completely wasted (and is actually slower than the build itself).This branch also includes a few cleanup and bug fixes. Some from an earlier
fig tag
branch that probably wont get merged, and some new ones I found while working on the tests. I'll comment on the specifics inline.Background
On this host:
I suspect this issue is less noticeable when there are fewer images, but 1335 doesn't seem like a lot. We actually cleanup images that are greater than 30 days old, so there shouldn't be anything terribly old here.
Total time: 30s
Number of fig services: 8 (2 image, 6 build)
This setup basically does 3 things:
fig 1.0 profile
This is heavily cleaned up, but shows the relevant lines.
Tracing the
project.up()
call through the stack:We see that 12 seconds out of 30 seconds are spent on an operation that we already know to be a no-op
this branch profile
Time is down to 18 seconds, and
fig up
is now 4 seconds instead of 16.