Ensure consistent load order of domains and networks. #346

mykaul · 2016-10-23T17:03:46Z

By processing JSON or YAML files via Ordered dicts.,
we ensure that domains are processed in the order they are specified
in the relevant configuration files.

mykaul · 2016-10-23T18:54:26Z

lago/prefix.py

@@ -742,7 +742,7 @@ def _ova_to_spec(self, filename):
                          "template_type": "qcow2",
                          "format": "qcow2",
                          "dev": "vda",
-                          "name": "root",
+                          # "name": "root",


This is in order to pass 'make check' - at some point we'll have to decide which is the correct line - this one or the next one. The next one makes more sense to me, so commented out this one.

I'll have to reread some of the code, but I recall that there was needed to have at least one image called root in order to know which one to boot from and such (ssh keys, configs that virt tools use).

Though I agree that the second makes more sense, not sure if we are using multi-image ovas anywhere though.

mykaul · 2016-10-23T18:54:49Z

tests/functional/fixtures/status/prefix_skel/expected.yaml

@@ -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
+      - []


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

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.

mykaul

Please review.

david-caro · 2016-10-23T20:04:14Z

Aside from the questions above, a first check looks ok for me, though I have not tested it yet though. If you made sure that it really solves your issues with the ordering, 👍 from me :)

pilou- · 2016-10-25T13:56:09Z

lago/virt.py

@@ -23,6 +23,7 @@
 import logging
 import os
 import uuid
+import collections


Use alphabetical order

nvgoldin · 2016-10-26T07:24:47Z

lago/utils.py

+    elif isinstance(original_obj, collections.OrderedDict):
+        return collections.OrderedDict(
+            (key, deepcopy(val)) for key, val in original_obj.items()
+        )


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

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

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

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.

@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.

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.

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"

mykaul · 2016-10-26T10:44:35Z

ci merge please

nvgoldin · 2016-10-26T12:54:28Z

looks like true failure http://jenkins.ovirt.org/job/lago_master_check-merged-fc23-x86_64/238/console:

11:01:33 #   File "/usr/lib64/python2.7/site-packages/yaml/constructor.py", line 88, in construct_object
11:01:33 #     data = constructor(self, node)
11:01:33 #   File "/usr/lib/python2.7/site-packages/lago/utils.py", line 457, in dict_constructor
11:01:33 #     return collections.OrderedDict(loader.construct_pairs(node))
11:01:33 #   File "/usr/lib64/python2.7/site-packages/yaml/constructor.py", line 144, in construct_pairs
11:01:33 #     key = self.construct_object(key_node, deep=deep)
11:01:33 #   File "/usr/lib64/python2.7/site-packages/yaml/constructor.py", line 88, in construct_object
11:01:33 #     data = constructor(self, node)
11:01:33 #   File "/usr/lib64/python2.7/site-packages/yaml/constructor.py", line 414, in construct_undefined
11:01:33 #     node.start_mark)
11:01:33 # ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:merge'
11:01:33 #   in "/home/jenkins/workspace/lago_master_check-merged-fc23-x86_64/lago/tests/functional/fixtures/ovirt.collect/suite.yaml", line 14, column 7
11:01:33 # ---

mykaul · 2016-10-26T13:39:09Z

Is the YAML there correct? I expect a prefix for the artifacts. Something like:

 - file: /var/log/something.log
 - file: /var/log/something_else.log

nvgoldin · 2016-11-01T05:45:58Z

@mykaul - I had a look at the /ovirt.collect/suite.yaml file, can you elaborate what you mean(what you think is wrong there)?
The error is in parsing the yaml as far as I understand(and not on different expected output) .

mykaul · 2016-11-01T06:27:32Z

@nvgoldin - I don't know why my change broke the parsing. I wasn't sure the YAML is entirely correct - for multiple entries, I expect a prefix (as I've shown in my previous comment)

By processing JSON or YAML files via Ordered dicts., we ensure that domains are processed in the order they are specified in the relevant configuration files.

mykaul · 2016-12-27T08:38:09Z

ci merge please

mykaul added enhancement needs review labels Oct 23, 2016

mykaul force-pushed the load_in_consistent_order branch 2 times, most recently from a856345 to 934d49c Compare October 23, 2016 18:25

mykaul commented Oct 23, 2016

View reviewed changes

pilou- reviewed Oct 25, 2016

View reviewed changes

nvgoldin reviewed Oct 26, 2016

View reviewed changes

mykaul force-pushed the load_in_consistent_order branch from 934d49c to 7c88ae8 Compare October 26, 2016 08:45

Ensure consistent load order of domains and networks.

249161c

By processing JSON or YAML files via Ordered dicts., we ensure that domains are processed in the order they are specified in the relevant configuration files.

mykaul force-pushed the load_in_consistent_order branch from 7c88ae8 to 249161c Compare November 23, 2016 10:59

nvgoldin modified the milestone: 0.30 Dec 14, 2016

nvgoldin modified the milestones: 0.30, 0.31 Dec 26, 2016

Merge branch 'master' into load_in_consistent_order

e20460f

nvgoldin modified the milestones: 0.31, 0.32 Jan 9, 2017

This was referenced Mar 12, 2017

Ensure LagoInitFile is loaded by a consistent order #472

Open

Generate an init file that can be used with exported images #477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure consistent load order of domains and networks. #346

Ensure consistent load order of domains and networks. #346

mykaul commented Oct 23, 2016

mykaul Oct 23, 2016

david-caro Oct 23, 2016

david-caro Oct 23, 2016

mykaul Oct 23, 2016

david-caro Oct 23, 2016

mykaul left a comment

david-caro commented Oct 23, 2016

pilou- Oct 25, 2016

mykaul Oct 26, 2016

nvgoldin Oct 26, 2016

mykaul Oct 26, 2016

david-caro Oct 28, 2016

david-caro Oct 31, 2016

mykaul Nov 1, 2016

david-caro Nov 1, 2016

david-caro Nov 1, 2016

mykaul commented Oct 26, 2016

nvgoldin commented Oct 26, 2016

mykaul commented Oct 26, 2016 •

edited

Loading

nvgoldin commented Nov 1, 2016 •

edited

Loading

mykaul commented Nov 1, 2016

mykaul commented Dec 27, 2016

Ensure consistent load order of domains and networks. #346

Are you sure you want to change the base?

Ensure consistent load order of domains and networks. #346

Conversation

mykaul commented Oct 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykaul left a comment

Choose a reason for hiding this comment

david-caro commented Oct 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykaul commented Oct 26, 2016

nvgoldin commented Oct 26, 2016

mykaul commented Oct 26, 2016 • edited Loading

nvgoldin commented Nov 1, 2016 • edited Loading

mykaul commented Nov 1, 2016

mykaul commented Dec 27, 2016

mykaul commented Oct 26, 2016 •

edited

Loading

nvgoldin commented Nov 1, 2016 •

edited

Loading