inventory query, reference merging and nested references enhancements

[AndrewPickford]

I'm using reclass and salt for a new configuration management setup and found I needed to do some things with reclass that it didn't yet do so I added in functionality to do the basic inventory querying mentioned in the todo list, to allow references to dicts and lists to be merged and to allow references to be nested. The new code is available at https://github.com/AndrewPickford/reclass/tree/master/reclass

The inventory querying works using a new key type - exports to hold values which other node definitions can read using a $[] query, for example with:

# nodes/node1.yml
exports:
  test_one:
    name: ${name}
    value: 6
  test_two: ${dict}

parameters:
  name: node1
  dict:
    a: 1
    b: 2
  exp_value_test: $[ exports:test_two ]
  exp_if_test1: $[ exports:test_one if exports:test_one:value == 7 ]
  exp_if_test2: $[ exports:test_one if exports:test_one:name == self:name ]

# nodes/node2.yml
exports:
  test_one:
    name: ${name}
    value: 7
  test_two: ${dict}

parameters:
  name: node2
  dict:
    a: 11
    b: 22

running reclass.py --nodeinfo node1 gives (listing only the exports and parameters):

exports:
  test_one:
    name: node1
    value: 6
  test_two:
    a: 1
    b: 2
parameters:
  dict:
    a: 1
    b: 2
  exp_if_test1:
    node2:
      name: node2
      value: 7
  exp_if_test2:
    node1:
      name: node1
      value: 6
  exp_value_test:
    node1:
      a: 1
      b: 2
    node2:
      a: 11
      b: 22
  name: node1

Exports defined for a node can be a simple value or a reference to a parameter in the node definition. The $[] inventory queries are calculated for simple value expressions, $[ exports:key ] , by returning a dictionary with an element ({ node_name: key value }) for each node which defines 'key' in the exports section. For tests, $[ exports:key if exports:test_key == test_value ], the element ({ node_name: key value }) is only added to the returned dictionary if the test_key defined in the node exports section equals the test value. The test value can either be a simple value or a node parameter.

I've also run into a use case for nested references, for example:

# nodes/node1.yml
parameters:
  alpha:
    one: ${beta:${alpha:two}}
    two: a
  beta:
    a: 99

reclass.py --nodeinfo node1 then gives:

parameters:
  alpha:
    one: 99
    two: a
  beta:
    a: 99

The ${beta:${alpha:two}} construct first resolves the ${alpha:two} reference to the value 'a', then resolves the reference ${beta:a} to the value 99

Finally referenced lists or dicts can be merged:

# nodes/test.yml
classes:
  - test1
  - test2
parameters:
  one:
    a: 1
    b: 2
  two:
    c: 3
    d: 4
  three:
    e: 5

# classes/test1.yml
parameters:
  three: ${one}

# classes/test2.yml
parameters:
  three: ${two}

running reclass.py --nodeinfo node1 then gives:

parameters:
  one:
    a: 1
    b: 2
  three:
    a: 1
    b: 2
    c: 3
    d: 4
    e: 5
  two:
    c: 3
    d: 4

This first sets the parameter three to the value of parameter one (class test1) then merges parameter two into parameter three (class test2) and finally merges the parameter three definition given in the node definition into the final value for parameter three.

The code works but isn't yet production ready. There are some tests to write for the inventory query operations, more informative errors need to be returned in the case of inventory query syntax errors, some documentation added for the new functionality and a couple of brute force kludges need to be finessed away.

My question is then: is this functionality something to put into the reclass main line once the new code is polished up?

[Rtzq0]

Hi Andrew, None of these ideas are things we'd necessarily be opposed to without question, but they definitely would need to be decomposed into much smaller modifications in their own PRs.

70 is now almost 1400 lines of code covering a number of diverse modifications, and I think it's beyond any of us to review it safely.

Basically what I'm saying is I love collaborators and collaboration, but this is a very large mouthful to even try to understand the ramifications of, and I'd like to ask you to make it into bite size pieces so that we can safely improve reclass to meet user needs.

[AndrewPickford]

Hi Jason,

I quite understand about the size of the commit - it's a lot to process in one go. Specifically for #70 could you look at the branch yaml-output in my fork. That's a single commit to provide the option not to use references in the yaml output.

For the rest the main problem is my fork changes how the parsing and merging of dictionaries in the Parameters class are handled and this is where most of the changes are. I've broken up the changes a little already, the branch recursive I have has the merge and nested references changes and then the exports branch builds on the recursive branch and adds the inventory query functionality. The recursive branch is still a big change but is a little bit more manageable. I've also added some extra tests to check that the new parsing gives the same results as the old and my fork passes all the current tests with the exception of three mocked tests due, I think, to how mocked dictionaries are handled.

What do you think about making a new branch, importing the commits from my recursive branch and then adding in some more tests (and configuration options) to really verify this new parsing scheme reproduces what the old one does.

[mbrgm]

@AndrewPickford Is there a chance that you are going to tackle breaking down the PR in the near-to-midterm future? Your changes (judging from the commit messages) look pretty awesome, but I think they should flow back into the project, so they're distributed with distro packages as well. I see there's a (still?) open question to @Rtzq0 in your last comment.

[AndrewPickford]

@mbrgm I was waiting on an answer from @Rtzq0 and with no reply merging my changes in got put on the back burner. I'd still like to see the changes merged in but I've done more work on my fork with plenty of bug fixes and the addition of using a git repo as a data source. This makes picking the changes apart a fair bit of work which would need help from the maintainers and even then the smallest change sets are still going to be fairly big. It would be easier (at least from my point of view) to setup a new branch for my fork and then show that the new parsing scheme reproduces what the old one does.

[bbinet]

The new features available in @AndrewPickford fork sound great. I'm planning to switch my reclass setup to use his fork. @AndrewPickford it would be great if the reclass documentation could also be updated in your fork.

Also @AndrewPickford, would you integrate this pull request #76 if I rebase to use your fork?

[AndrewPickford]

@bbinet I'm currently on holiday, back at the start of September so I won't be updating my fork until then. But I had quick look at #76 and I don't see any problem with including it. The docs are also creeping up my todo list. I'll be starting with a readme for my extensions including more examples similar to ones I've given at the start of this issue.

[epcim]

@bbinet commented on Aug 4, 2017, 12:06 PM GMT+2:

The new features available in @AndrewPickford fork sound great. I'm planning to switch my reclass setup to use his fork.

@AndrewPickford it would be great if the reclass documentation could also be updated in your fork.

Also @AndrewPickford, would you integrate this pull request #76 if I rebase to use your fork?

Hi, I am assuming the same (switch to Andrew fork or at least get the #76 as well #77), but as I have to take actions right now so I forked. https://github.com/salt-formulas/reclass. Master branch has #76, #77 as of now I will used it under my CI. I had thought it would be nice to have a kind of "develop" (where actually is the #73) and where will be more open policy to merge features.

I have an option to test it with extensive set of CI where I may give a try to

I created a fork to give a try both.

[AndrewPickford]

@bbinet, @epcim if it's still of interest I've updated my fork to include the ignore class not found flag and made a start on documentation for the extensions I've added.

[bbinet]

Thanks @AndrewPickford, I now have everything I need in you fork to proceed the switch