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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 31, 2014
Merged

Speed up fig up #586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b777ee
Cleanup service unit tests and restructure some service create logic.
dnephin Oct 26, 2014
3056ae4
Add a no-build option to fig up, to save time when services were alre…
dnephin Oct 26, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion fig/cli/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ def up(self, project, options):
--no-color Produce monochrome output.
--no-deps Don't start linked services.
--no-recreate If containers already exist, don't recreate them.
--no-build Don't build an image, even if it's missing
"""
insecure_registry = options['--allow-insecure-ssl']
detached = options['-d']
Expand All @@ -440,7 +441,8 @@ def up(self, project, options):
start_links=start_links,
recreate=recreate,
insecure_registry=insecure_registry,
detach=options['-d']
detach=options['-d'],
do_build=not options['--no-build'],
)

to_attach = [c for s in project.get_services(service_names) for c in s.containers()]
Expand Down
18 changes: 15 additions & 3 deletions fig/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,26 @@ def build(self, service_names=None, no_cache=False):
else:
log.info('%s uses an image, skipping' % service.name)

def up(self, service_names=None, start_links=True, recreate=True, insecure_registry=False, detach=False):
def up(self,
service_names=None,
start_links=True,
recreate=True,
insecure_registry=False,
detach=False,
do_build=True):
running_containers = []
for service in self.get_services(service_names, include_links=start_links):
if recreate:
for (_, container) in service.recreate_containers(insecure_registry=insecure_registry, detach=detach):
for (_, container) in service.recreate_containers(
insecure_registry=insecure_registry,
detach=detach,
do_build=do_build):
running_containers.append(container)
else:
for container in service.start_or_create_containers(insecure_registry=insecure_registry, detach=detach):
for container in service.start_or_create_containers(
insecure_registry=insecure_registry,
detach=detach,
do_build=do_build):
running_containers.append(container)

return running_containers
Expand Down
67 changes: 48 additions & 19 deletions fig/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,17 @@
'workdir' : 'working_dir',
}

DOCKER_START_KEYS = [
'cap_add',
'cap_drop',
'dns',
'dns_search',
'env_file',
'net',
'privileged',
'restart',
]

VALID_NAME_CHARS = '[a-zA-Z0-9]'


Expand Down Expand Up @@ -145,7 +156,8 @@ def restart(self, **options):

def scale(self, desired_num):
"""
Adjusts the number of containers to the specified number and ensures they are running.
Adjusts the number of containers to the specified number and ensures
they are running.

- creates containers until there are at least `desired_num`
- stops containers until there are at most `desired_num` running
Expand Down Expand Up @@ -192,12 +204,24 @@ def remove_stopped(self, **options):
log.info("Removing %s..." % c.name)
c.remove(**options)

def create_container(self, one_off=False, insecure_registry=False, **override_options):
def create_container(self,
one_off=False,
insecure_registry=False,
do_build=True,
**override_options):
"""
Create a container for this service. If the image doesn't exist, attempt to pull
it.
"""
container_options = self._get_container_create_options(override_options, one_off=one_off)
container_options = self._get_container_create_options(
override_options,
one_off=one_off)

if (do_build and
self.can_be_built() and
not self.client.images(name=self.full_name)):
self.build()
Copy link
Author

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


try:
return Container.create(self.client, **container_options)
except APIError as e:
Expand All @@ -212,15 +236,18 @@ def create_container(self, one_off=False, insecure_registry=False, **override_op
return Container.create(self.client, **container_options)
raise

def recreate_containers(self, insecure_registry=False, **override_options):
def recreate_containers(self, insecure_registry=False, do_build=True, **override_options):
"""
If a container for this service doesn't exist, create and start one. If there are
any, stop them, create+start new ones, and remove the old containers.
"""
containers = self.containers(stopped=True)
if not containers:
log.info("Creating %s..." % self._next_container_name(containers))
container = self.create_container(insecure_registry=insecure_registry, **override_options)
container = self.create_container(
insecure_registry=insecure_registry,
do_build=do_build,
**override_options)
self.start_container(container)
return [(None, container)]
else:
Expand Down Expand Up @@ -259,7 +286,7 @@ def recreate_container(self, container, **override_options):
container.remove()

options = dict(override_options)
new_container = self.create_container(**options)
new_container = self.create_container(do_build=False, **options)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set this to always False. I believe since this is in recreate_container() we know that an image must already exist for it, since we already have a container. If you think this assertion is incorrect, I can change this to use the param as well.

self.start_container(new_container, intermediate_container=intermediate_container)

intermediate_container.remove()
Expand All @@ -273,8 +300,7 @@ def start_container_if_stopped(self, container, **options):
log.info("Starting %s..." % container.name)
return self.start_container(container, **options)

def start_container(self, container=None, intermediate_container=None, **override_options):
container = container or self.create_container(**override_options)
def start_container(self, container, intermediate_container=None, **override_options):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the call to create_container() and made container a required arg. Every call (outside of tests) was actually already passing in a container, and having this extra create_container() call makes the order of calls harder to follow. I moved this case (of having start_container() actually call create), to a helper function in the test module.

options = dict(self.options, **override_options)
port_bindings = build_port_bindings(options.get('ports') or [])

Expand Down Expand Up @@ -307,14 +333,19 @@ def start_container(self, container=None, intermediate_container=None, **overrid
)
return container

def start_or_create_containers(self, insecure_registry=False, detach=False):
def start_or_create_containers(
self,
insecure_registry=False,
detach=False,
do_build=True):
containers = self.containers(stopped=True)

if not containers:
log.info("Creating %s..." % self._next_container_name(containers))
new_container = self.create_container(
insecure_registry=insecure_registry,
detach=detach
detach=detach,
do_build=do_build,
)
return [self.start_container(new_container)]
else:
Expand Down Expand Up @@ -407,16 +438,13 @@ def _get_container_create_options(self, override_options, one_off=False):
container_options['environment'] = merge_environment(container_options)

if self.can_be_built():
if len(self.client.images(name=self._build_tag_name())) == 0:
self.build()
container_options['image'] = self._build_tag_name()
container_options['image'] = self.full_name
else:
container_options['image'] = self._get_image_name(container_options['image'])

# Delete options which are only used when starting
for key in ['privileged', 'net', 'dns', 'dns_search', 'restart', 'cap_add', 'cap_drop', 'env_file']:
if key in container_options:
del container_options[key]
for key in DOCKER_START_KEYS:
container_options.pop(key, None)

return container_options

Expand All @@ -431,7 +459,7 @@ def build(self, no_cache=False):

build_output = self.client.build(
self.options['build'],
tag=self._build_tag_name(),
tag=self.full_name,
stream=True,
rm=True,
nocache=no_cache,
Expand All @@ -451,14 +479,15 @@ def build(self, no_cache=False):
image_id = match.group(1)

if image_id is None:
raise BuildError(self)
raise BuildError(self, event if all_events else 'Unknown')
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug fix, picked from #457

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the event variable makes me a bit nervous - I mentally scope it to the for-loop. Could we be a bit more explicit about it and use a last_event variable which is updated within the loop?

Happy to merge this change as a separate PR btw.


return image_id

def can_be_built(self):
return 'build' in self.options

def _build_tag_name(self):
@property
def full_name(self):
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

picked from #457

"""
The tag to give to images built for this service.
"""
Expand Down
Loading