Skip to content

Commit

Permalink
Merge pull request docker#863 from dnephin/revert_volume_recreate_cha…
Browse files Browse the repository at this point in the history 
…nges

Revert docker#711 from dnephin/fix_volumes_on_recreate
  • Loading branch information
@aanand
aanand committed Jan 20, 2015
2 parents 55095ef + 2dd1cc8 commit f57db07
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 142 deletions.
20 changes: 12 additions & 8 deletions fig/project.py
View file
Expand Up @@ -176,14 +176,18 @@ def up(self,
do_build=True):
running_containers = []
for service in self.get_services(service_names, include_links=start_links):
create_func = (service.recreate_containers if recreate
else service.start_or_create_containers)

for container in create_func(
insecure_registry=insecure_registry,
detach=detach,
do_build=do_build):
running_containers.append(container)
if recreate:
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,
do_build=do_build):
running_containers.append(container)

return running_containers

Expand Down
92 changes: 26 additions & 66 deletions fig/service.py
View file
Expand Up @@ -242,9 +242,8 @@ def create_container(self,

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.
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:
Expand All @@ -254,22 +253,21 @@ def recreate_containers(self, insecure_registry=False, do_build=True, **override
do_build=do_build,
**override_options)
self.start_container(container)
return [container]
return [(None, container)]
else:
return [
self.recreate_container(
container,
insecure_registry=insecure_registry,
**override_options)
for container in containers
]
tuples = []

for c in containers:
log.info("Recreating %s..." % c.name)
tuples.append(self.recreate_container(c, insecure_registry=insecure_registry, **override_options))

return tuples

def recreate_container(self, container, **override_options):
"""Recreate a container. An intermediate container is created so that
the new container has the same name, while still supporting
`volumes-from` the original container.
"""
log.info("Recreating %s..." % container.name)
try:
container.stop()
except APIError as e:
Expand All @@ -280,30 +278,24 @@ def recreate_container(self, container, **override_options):
else:
raise

intermediate_options = dict(self.options, **override_options)
intermediate_container = Container.create(
self.client,
image=container.image,
entrypoint=['/bin/echo'],
command=[],
detach=True,
)
intermediate_container.start(
binds=get_container_data_volumes(
container, intermediate_options.get('volumes')))
intermediate_container.start(volumes_from=container.id)
intermediate_container.wait()
container.remove()

# TODO: volumes are being passed to both start and create, this is
# probably unnecessary
options = dict(override_options)
new_container = self.create_container(do_build=False, **options)
self.start_container(
new_container,
intermediate_container=intermediate_container)
self.start_container(new_container, intermediate_container=intermediate_container)

intermediate_container.remove()
return new_container

return (intermediate_container, new_container)

def start_container_if_stopped(self, container, **options):
if container.is_running:
Expand All @@ -315,6 +307,12 @@ def start_container_if_stopped(self, container, **options):
def start_container(self, container, intermediate_container=None, **override_options):
options = dict(self.options, **override_options)
port_bindings = build_port_bindings(options.get('ports') or [])

volume_bindings = dict(
build_volume_binding(parse_volume_spec(volume))
for volume in options.get('volumes') or []
if ':' in volume)

privileged = options.get('privileged', False)
net = options.get('net', 'bridge')
dns = options.get('dns', None)
Expand All @@ -323,14 +321,12 @@ def start_container(self, container, intermediate_container=None, **override_opt
cap_drop = options.get('cap_drop', None)

restart = parse_restart_spec(options.get('restart', None))
binds = get_volume_bindings(
options.get('volumes'), intermediate_container)

container.start(
links=self._get_links(link_to_self=options.get('one_off', False)),
port_bindings=port_bindings,
binds=binds,
volumes_from=self._get_volumes_from(),
binds=volume_bindings,
volumes_from=self._get_volumes_from(intermediate_container),
privileged=privileged,
network_mode=net,
dns=dns,
Expand Down Expand Up @@ -392,7 +388,7 @@ def _get_links(self, link_to_self):
links.append((external_link, link_name))
return links

def _get_volumes_from(self):
def _get_volumes_from(self, intermediate_container=None):
volumes_from = []
for volume_source in self.volumes_from:
if isinstance(volume_source, Service):
Expand All @@ -406,6 +402,9 @@ def _get_volumes_from(self):
elif isinstance(volume_source, Container):
volumes_from.append(volume_source.id)

if intermediate_container:
volumes_from.append(intermediate_container.id)

return volumes_from

def _get_container_create_options(self, override_options, one_off=False):
Expand Down Expand Up @@ -520,45 +519,6 @@ def pull(self, insecure_registry=False):
)


def get_container_data_volumes(container, volumes_option):
"""Find the container data volumes that are in `volumes_option`, and return
a mapping of volume bindings for those volumes.
"""
volumes = []
for volume in volumes_option or []:
volume = parse_volume_spec(volume)
# No need to preserve host volumes
if volume.external:
continue

volume_path = (container.get('Volumes') or {}).get(volume.internal)
# New volume, doesn't exist in the old container
if not volume_path:
continue

# Copy existing volume from old container
volume = volume._replace(external=volume_path)
volumes.append(build_volume_binding(volume))

return dict(volumes)


def get_volume_bindings(volumes_option, intermediate_container):
"""Return a list of volume bindings for a container. Container data volume
bindings are replaced by those in the intermediate container.
"""
volume_bindings = dict(
build_volume_binding(parse_volume_spec(volume))
for volume in volumes_option or []
if ':' in volume)

if intermediate_container:
volume_bindings.update(
get_container_data_volumes(intermediate_container, volumes_option))

return volume_bindings


NAME_RE = re.compile(r'^([^_]+)_([^_]+)_(run_)?(\d+)$')


Expand Down
35 changes: 16 additions & 19 deletions tests/integration/service_test.py
View file
Expand Up @@ -98,7 +98,7 @@ def test_create_container_with_unspecified_volume(self):
service = self.create_service('db', volumes=['/var/db'])
container = service.create_container()
service.start_container(container)
self.assertIn('/var/db', container.get('Volumes'))
self.assertIn('/var/db', container.inspect()['Volumes'])

def test_create_container_with_cpu_shares(self):
service = self.create_service('db', cpu_shares=73)
Expand Down Expand Up @@ -148,23 +148,30 @@ def test_recreate_containers(self):
self.assertIn('FOO=1', old_container.dictionary['Config']['Env'])
self.assertEqual(old_container.name, 'figtest_db_1')
service.start_container(old_container)
volume_path = old_container.get('Volumes')['/etc']
volume_path = old_container.inspect()['Volumes']['/etc']

num_containers_before = len(self.client.containers(all=True))

service.options['environment']['FOO'] = '2'
containers = service.recreate_containers()
self.assertEqual(len(containers), 1)
tuples = service.recreate_containers()
self.assertEqual(len(tuples), 1)

new_container = containers[0]
self.assertEqual(new_container.get('Config.Entrypoint'), ['sleep'])
self.assertEqual(new_container.get('Config.Cmd'), ['300'])
self.assertIn('FOO=2', new_container.get('Config.Env'))
intermediate_container = tuples[0][0]
new_container = tuples[0][1]
self.assertEqual(intermediate_container.dictionary['Config']['Entrypoint'], ['/bin/echo'])

self.assertEqual(new_container.dictionary['Config']['Entrypoint'], ['sleep'])
self.assertEqual(new_container.dictionary['Config']['Cmd'], ['300'])
self.assertIn('FOO=2', new_container.dictionary['Config']['Env'])
self.assertEqual(new_container.name, 'figtest_db_1')
self.assertEqual(new_container.get('Volumes')['/etc'], volume_path)
self.assertEqual(new_container.inspect()['Volumes']['/etc'], volume_path)
self.assertIn(intermediate_container.id, new_container.dictionary['HostConfig']['VolumesFrom'])

self.assertEqual(len(self.client.containers(all=True)), num_containers_before)
self.assertNotEqual(old_container.id, new_container.id)
self.assertRaises(APIError,
self.client.inspect_container,
intermediate_container.id)

def test_recreate_containers_when_containers_are_stopped(self):
service = self.create_service(
Expand All @@ -179,16 +186,6 @@ def test_recreate_containers_when_containers_are_stopped(self):
service.recreate_containers()
self.assertEqual(len(service.containers(stopped=True)), 1)

def test_recreate_containers_with_volume_changes(self):
service = self.create_service('withvolumes', volumes=['/etc'])
old_container = create_and_start_container(service)
self.assertEqual(old_container.get('Volumes').keys(), ['/etc'])

service = self.create_service('withvolumes')
container, = service.recreate_containers()
service.start_container(container)
self.assertEqual(container.get('Volumes'), {})

def test_start_container_passes_through_options(self):
db = self.create_service('db')
create_and_start_container(db, environment={'FOO': 'BAR'})
Expand Down
59 changes: 10 additions & 49 deletions tests/unit/service_test.py
View file
Expand Up @@ -11,15 +11,13 @@
from fig import Service
from fig.container import Container
from fig.service import (
APIError,
ConfigError,
split_port,
build_port_bindings,
parse_volume_spec,
build_volume_binding,
get_container_data_volumes,
get_volume_bindings,
APIError,
parse_repository_tag,
parse_volume_spec,
split_port,
)


Expand Down Expand Up @@ -59,6 +57,13 @@ def test_get_volumes_from_container(self):

self.assertEqual(service._get_volumes_from(), [container_id])

def test_get_volumes_from_intermediate_container(self):
container_id = 'aabbccddee'
service = Service('test')
container = mock.Mock(id=container_id, spec=Container)

self.assertEqual(service._get_volumes_from(container), [container_id])

def test_get_volumes_from_service_container_exists(self):
container_ids = ['aabbccddee', '12345']
from_service = mock.create_autospec(Service)
Expand Down Expand Up @@ -283,50 +288,6 @@ def test_building_volume_binding_with_home(self):
binding,
('/home/user', dict(bind='/home/user', ro=False)))

def test_get_container_data_volumes(self):
options = [
'/host/volume:/host/volume:ro',
'/new/volume',
'/existing/volume',
]

container = Container(None, {
'Volumes': {
'/host/volume': '/host/volume',
'/existing/volume': '/var/lib/docker/aaaaaaaa',
'/removed/volume': '/var/lib/docker/bbbbbbbb',
},
}, has_been_inspected=True)

expected = {
'/var/lib/docker/aaaaaaaa': {'bind': '/existing/volume', 'ro': False},
}

binds = get_container_data_volumes(container, options)
self.assertEqual(binds, expected)

def test_get_volume_bindings(self):
options = [
'/host/volume:/host/volume:ro',
'/host/rw/volume:/host/rw/volume',
'/new/volume',
'/existing/volume',
]

intermediate_container = Container(None, {
'Volumes': {'/existing/volume': '/var/lib/docker/aaaaaaaa'},
}, has_been_inspected=True)

expected = {
'/host/volume': {'bind': '/host/volume', 'ro': True},
'/host/rw/volume': {'bind': '/host/rw/volume', 'ro': False},
'/var/lib/docker/aaaaaaaa': {'bind': '/existing/volume', 'ro': False},
}

binds = get_volume_bindings(options, intermediate_container)
self.assertEqual(binds, expected)


class ServiceEnvironmentTest(unittest.TestCase):

def setUp(self):
Expand Down

0 comments on commit f57db07

Please sign in to comment.