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

Jump to bottom

Ensure consistent load order of domains and networks. #346

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ensure consistent load order of domains and networks. #346

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
249161c
Ensure consistent load order of domains and networks.
Oct 23, 2016
e20460f
Merge branch 'master' into load_in_consistent_order
Dec 27, 2016
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
18 changes: 17 additions & 1 deletion lago/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,10 @@ def deepcopy(original_obj):
"""
if isinstance(original_obj, list):
return list(deepcopy(item) for item in original_obj)
elif isinstance(original_obj, collections.OrderedDict):
return collections.OrderedDict(
(key, deepcopy(val)) for key, val in original_obj.items()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an issue with this patch - but stdlib's copy.deepcopy will not work here?(replacing the entire function)

Copy link
Author

Choose a reason for hiding this comment

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

@david-caro can give an explanation (which I remember was quite long) on why not. IIRC, it's not really doing only deepcopy?

Copy link
Member

Choose a reason for hiding this comment

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

Let me investigate over the weekend, it's been a quite busy week :)

Copy link
Member

Choose a reason for hiding this comment

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

I remember that there was some subtlety on why to use this function instead of copy.deepcopy, I vote for trying it out and replace this one if no issues arise, hopefully the code that depended on that subtlety is fixed/removed.

Copy link
Author

Choose a reason for hiding this comment

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

@david-caro - this is the explanation you once gave me:
The copy.deepcopy function is not enough, as it's not doing what I intended
with the deepcopy one.

The issue is that, when writing yaml, you can do something like:

  • something: &common_stuff
    key1: val1
    key2: val2
  • host1:
    <<*: common_stuff
    key1: customval1
  • host2:
    <<*: common_stuff
    key2: customval2

That will allow you to reuse some common definitions over and over on the same
yaml file, the problem is that it's actually a reference to the same objects,
so if you define a list as the common_stuff, then including it twice, will
create a pointer to the same list on both dictionaries, creating issues when
updating them specifically for each host. So the deepcopy function is meant to
remove those common pointers and create totally independent objects that don't
interlink so we can safely update each of them independently.

That does not happen with json, as, afaik, there's no supported way to share
common definitions like that.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that was a good explanation on why any deepcopy is needed, not on why use our own deepcopy :/, my bad.

An example of that issue:

In [36]: import yaml
In [37]: myyaml='''
something: &common_stuff
  key: value
  key2: value2
  key3:
    - one
    - two

one:
  <<: *common_stuff
two:
  <<: *common_stuff

'''
In [38]: res = yaml.load(myyaml)
In [39]: res
Out[39]: 
{'one': {'key': 'value', 'key2': 'value2', 'key3': ['one', 'two']},
 'something': {'key': 'value', 'key2': 'value2', 'key3': ['one', 'two']},
 'two': {'key': 'value', 'key2': 'value2', 'key3': ['one', 'two']}}
In [40]: res['one']
Out[40]: {'key': 'value', 'key2': 'value2', 'key3': ['one', 'two']}
In [41]: res['one'] is res['two']
Out[41]: False
In [42]: res['one']['key3'] is res['two']['key3']
Out[42]: True

I don't see any issue overall, I'll try replacing it with the stdlib one and do some tests, see if it breaks anything.

Copy link
Member

Choose a reason for hiding this comment

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

Just replacing it with the stdlib copy.deepcopy does not seem to work, tests are failing for me after the change, will investigate:

not ok 22 basic.full_run: start and stop many vms one by one
# (from function `helpers.equals' in file tests/functional/helpers.bash, line 76,
#  from function `helpers.run_ok' in file tests/functional/helpers.bash, line 62,
#  in test file tests/functional/basic.bats, line 301)
#   `helpers.run_ok "$LAGOCLI" \' failed
# RUNNING:lago --prefix-name multi_vm init --template-repo-path /home/dcaro/Work/lago-project/lago/tests/functional/fixtures/basic/template_repo.json --template-repo-name local_tests_repo --template-store /home/dcaro/Work/lago-project/lago/tests/functional/fixtures/basic/repo_store --skip-bootstrap /home/dcaro/Work/lago-project/lago/tests/functional/fixtures/basic/suite_multi_vm.yaml
# --output--
# current session does not belong to lago group.
# @ Initialize and populate prefix: 
#   # Initialize prefix: 
#     * Create prefix dirs: 
#     * Create prefix dirs: Success (in 0:00:00)
#     * Generate prefix uuid: 
#     * Generate prefix uuid: Success (in 0:00:00)
#     * Create ssh keys: 
#     * Create ssh keys: Success (in 0:00:00)
#     * Tag prefix as initialized: 
#     * Tag prefix as initialized: Success (in 0:00:00)
#   # Initialize prefix: Success (in 0:00:00)
#   # Create disks for VM lago_functional_tests_vm02: 
#     * Create disk root: 
#       - Template local_tests_repo:lago_functional_tests:v1 not in cache, downloading
#     * Create disk root: Success (in 0:00:00)
#   # Create disks for VM lago_functional_tests_vm02: Success (in 0:00:00)
#   # Create disks for VM lago_functional_tests_vm01: 
#   # Create disks for VM lago_functional_tests_vm01: ERROR (in 0:00:00)
# @ Initialize and populate prefix: ERROR (in 0:00:00)
# Error occured, aborting
# Traceback (most recent call last):
#   File "/usr/lib/python2.7/site-packages/lago/cmd.py", line 810, in main
#     cli_plugins[args.verb].do_run(args)
#   File "/usr/lib/python2.7/site-packages/lago/plugins/cli.py", line 180, in do_run
#     self._do_run(**vars(args))
#   File "/usr/lib/python2.7/site-packages/lago/log_utils.py", line 615, in wrapper
#     return func(*args, **kwargs)
#   File "/usr/lib/python2.7/site-packages/lago/cmd.py", line 182, in do_init
#     do_bootstrap=not skip_bootstrap,
#   File "/usr/lib/python2.7/site-packages/lago/prefix.py", line 816, in virt_conf_from_stream
#     do_bootstrap=do_bootstrap,
#   File "/usr/lib/python2.7/site-packages/lago/prefix.py", line 930, in virt_conf
#     template_store=template_store,
#   File "/usr/lib/python2.7/site-packages/lago/prefix.py", line 836, in _prepare_domains_images
#     template_store=template_store,
#   File "/usr/lib/python2.7/site-packages/lago/prefix.py", line 858, in _prepare_domain_image
#     template_store=template_store,
#   File "/usr/lib/python2.7/site-packages/lago/prefix.py", line 886, in _create_disks
#     template_store=template_store,
#   File "/usr/lib/python2.7/site-packages/lago/prefix.py", line 463, in _create_disk
#     with LogTask("Create disk %s" % spec['name']):
# KeyError: 'name'
# ---
# "1" == "0"

elif isinstance(original_obj, dict):
return dict((key, deepcopy(val)) for key, val in original_obj.items())
else:
Expand All @@ -438,9 +442,21 @@ def load_virt_stream(virt_fd):
dict: Loaded virt config
"""
try:
virt_conf = json.load(virt_fd)
virt_conf = json.load(
virt_fd, object_pairs_hook=collections.OrderedDict
)
except ValueError:
virt_fd.seek(0)

# Ensure ordered dict loading from YAML. Based on
# http://stackoverflow.com/questions/5121931/in-python-how-can-you-load-yaml-mappings-as-ordereddicts
_mapping_tag = yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG

def dict_constructor(loader, node):
return collections.OrderedDict(loader.construct_pairs(node))

yaml.add_constructor(_mapping_tag, dict_constructor)

virt_conf = yaml.load(virt_fd)

return deepcopy(virt_conf)
Expand Down
19 changes: 12 additions & 7 deletions lago/virt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#
# Refer to the README and COPYING files for full details of the license
#
import collections
import functools
import hashlib
import json
Expand Down Expand Up @@ -86,11 +87,11 @@ def __init__(self, prefix, vm_specs, net_specs):
with open(self.prefix.paths.uuid(), 'r') as uuid_fd:
self.uuid = uuid_fd.read().strip()

self._nets = {}
self._nets = collections.OrderedDict()
for name, spec in net_specs.items():
self._nets[name] = self._create_net(spec)

self._vms = {}
self._vms = collections.OrderedDict()
for name, spec in vm_specs.items():
self._vms[name] = self._create_vm(spec)

Expand Down Expand Up @@ -244,17 +245,21 @@ def from_prefix(cls, prefix):
virt_path = functools.partial(prefix.paths.prefixed, 'virt')

with open(virt_path('env'), 'r') as f:
env_dom = json.load(f)
env_dom = json.load(f, object_pairs_hook=collections.OrderedDict)

net_specs = {}
net_specs = collections.OrderedDict()
for name in env_dom['nets']:
with open(virt_path('net-%s' % name), 'r') as f:
net_specs[name] = json.load(f)
net_specs[name] = json.load(
f, object_pairs_hook=collections.OrderedDict
)

vm_specs = {}
vm_specs = collections.OrderedDict()
for name in env_dom['vms']:
with open(virt_path('vm-%s' % name), 'r') as f:
vm_specs[name] = json.load(f)
vm_specs[name] = json.load(
f, object_pairs_hook=collections.OrderedDict
)

return cls(prefix, vm_specs, net_specs)

Expand Down
6 changes: 4 additions & 2 deletions tests/functional/fixtures/status/prefix_skel/expected.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ Prefix:
eth1: {ip: !!python/unicode '192.168.202.2', network: !!python/unicode 'n1'}
VNC port: null
distro: !!python/unicode 'cirros'
metadata: {}
metadata: !!python/object/apply:collections.OrderedDict
- []
Copy link
Author

Choose a reason for hiding this comment

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

I sincerely hope I actually fixed the test and not bypassed it here.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok, the array content (the second line) is the contents of the ordered dict, probably we might want to add also a test where there's actually content there to make sure that's actually ordered, though the name of the class there might be enough.

root password: !!python/unicode '123456'
snapshots: ''
status: down
Expand All @@ -23,7 +24,8 @@ Prefix:
eth1: {ip: !!python/unicode '192.168.202.3', network: !!python/unicode 'n1'}
VNC port: null
distro: !!python/unicode 'cirros'
metadata: {}
metadata: !!python/object/apply:collections.OrderedDict
- []
root password: !!python/unicode 'cubswin:)'
snapshots: ''
status: down