feat(pkg): separate components, use requisites and add clean states

[myii]

• tpldot is the equivalent of tpldir but using . instead of /:
  • https://github.com/saltstack/salt/blob/0c78d7dc894058988d171a28a11bd4a9dbf60266/salt/utils/jinja.py#L120
  • https://github.com/saltstack/salt/blob/0c78d7dc894058988d171a28a11bd4a9dbf60266/salt/utils/templates.py#L145
• Implementation based on upstream issue:
  • Require a relative sls file saltstack/salt#10838 (comment)

---

@noelmcloughlin Requesting your review because this is the implementation of the ideas you suggested in our initial discussions (see 22:34). Thanks for those ideas.

[myii]

It's a shame that relative includes (i.e. .install) can't be used when settings a requisite based on an sls -- this is an upstream bug that still hasn't been resolved.

[aboe76]

I didn't even know they existed, where did you find these...

[myii]

@aboe76 I've linked to the code in the first comment.

[myii]

@aboe76 Thanks for the review. What would be the right way to do testing for the clean states? Can it be managed in the same run as the existing tests?

[noelmcloughlin]

LGTM - can be merged.

I also want to share the formula structure I have leaned towards in recent work, illustrated below.

• each function has its own directory containing init.sls and clean.sls
• yaml files are moved to yaml/ subdirectory - Experimental, I'm undecided about this.
  The consistency is self documenting, scales, and helps make API predictable.

Today template-formula interprets pkg as Linux Package Manager format - that's good and necessary and expected. Going forward I'd like to see this formula handle repo (source) and release (architecture specific or independent) also.

• I split these under repo and release
• I did try merging both functions under a common software directory structure, but found the implementation is more simple when splitting.

Anyway, this is for future but I wanted to share one vision of what this could evolve towards.

   template-formula/
        template/
             init.sls
             clean.sls
             config/init.sls
             config/clean.sls
             pkg/init.sls
             pkg/clean.sls
             repo/init.sls
             repo/clean.sls
             release/init.sls
             release/clean.sls
             service/init.sls
             service/clean.sls
             map.jinja
             files/
                 macros.j2
                 daemon.j2
                 profile.j2
             yaml/
                  defaults.yaml
                  osmap.yaml
                  osfamilymap.yaml

[javierbertoli]

@myii suggesting some changes I think should be done

[javierbertoli]

template/pkg/init.sls shouldn't include .config nor .service, as no configuration or service management should happen under the pkg subdir, right?

[myii]

My method of refactoring on community projects is to make the smallest set of code changes possible while ensuring that there is no change in function. So this was a direct translation of what template/init.sls was doing before this PR.

It is also based on how I've found existing formulas doing things, where installation-config-service are kept close together as a logical block.

However, your point is completely valid and represents the ideal. A couple of questions before I add to this PR (and to leave time for others to comment):

1. We are still agreeing that running this formula should still perform all three stages, as it did before this PR, right?
2. In order to keep to maintain portability, how do you prefer that the other sls files are used in the include statements and the require requistes?
   1. My preference is navigating from the root directory where topdir is resolved as it is here, for example:
      1. {{ topdir }}.pkg.install
      2. {{ topdir }}.pkg.config
      3. {{ topdir }}.pkg.service
   2. Another method is relative navigation, which starts by resolving the next directory up and going from there:
      1. {{ parentdir }}.pkg.install
      2. {{ parentdir }}.pkg.config
      3. {{ parentdir }}.pkg.service

That's not very clear. Let me give examples for both.

Formula root directory-based navigation using topdir:

...
{%- set topdir = tpldir.split('/')[0] %}
{%- from topdir ~ "/map.jinja" import template with context %}
{%- from topdir ~ "/macros.jinja" import files_switch with context %}

include:
  - {{ topdir }}.pkg.install

template-config:
  ...
    - require:
      - sls: {{ topdir }}.pkg.install

Parent directory-based navigation using parentdir and parentdot:

...
{%- set parentdir = salt['file.dirname'](tpldir) %}
{%- set parentdot = parentdir|replace('/', '.') %}
{%- from parentdir ~ "/map.jinja" import template with context %}
{%- from parentdir ~ "/macros.jinja" import files_switch with context %}

include:
  - {{ parentdot }}.pkg.install

template-config:
  ...
    - require:
      - sls: {{ parentdot }}.pkg.install

[javierbertoli]

We are still agreeing that running this formula should still perform all three stages, as it did before this PR, right?

Yes, I think there's no discussion about this point: "the template formula should provide an example of:

• installing/removing a package (app),
• configuring/deleting the config of such app
• starting/stopping (deleting) the service of such app.

My comment in this PR regarding the structure is that you included the config and service states inside the pkg subdir.

            {{ topdir }}.pkg.install
            {{ topdir }}.pkg.config
            {{ topdir }}.pkg.service

But from @noelmcloughlin proposal (wich makes sense to me), the structure should be (from the formula's topdir)

{{ topdir }}.init.sls       # includes {pkg,config,service}.init
{{ topdir }}.clean.sls      # includes {service,config,pkg}.clean

{{ topdir }}.pkg            # `pkg/init.sls` which invokes `pkg.install`
{{ topdir }}.pkg.install
{{ topdir }}.pkg.clean

{{ topdir }}.config         # `config/init.sls` which configs the app
{{ topdir }}.config.clean

{{ topdir }}.service    # `service/init.sls which starts the service
{{ topdir }}.service.clean

[myii]

@javierbertoli Yes, that's fine. I pulled @noelmcloughlin into the conversation at the beginning because I knew he had interesting things to offer.

[noelmcloughlin]

I agree with @javierbertoli observations that configuration and service should be separate (from pkg/ ) but we could capture those review comments for future PR.

Using '{{ topdir }}' instead of relative path has one advantage - in terms of consistency - where foreign states are included in the top states.

For example the following highlights that nginx and {{ topdir }} should co-exist in the top directory (i.e. base).

include:
  - nginx.ng
   - {{ topdir }}.pkg.install

This relationship maybe less obvious when parentdir is used.

include:
  - nginx.ng
   - {{ parentdir }}.pkg.install

However the scenario I am raising is not the common pattern so its minor consideration.

[daks]

to ease separation of states and (OS) data

[aboe76]

@daks thats true, and this is one of the things that needs to be discussed openly and maybe there are more solutions to the same problem.

[myii]

@daks @aboe76 If this needs to be discussed in more detail, it will be far easier to do that in our Slack/IRC/Matrix room than via. GitHub comments...

[javierbertoli]

@noelmcloughlin raised a need ('formulas should include 'tear down' capabilities) and with that suggested a solution (the directories structure and states).

While I agree the solution is complex, I OK'ed it because it was better than what we have now 😄

Another solution for that need is to keep out simple states:

{{ topdir }}.init.sls   # includes {pkg,config,service}
{{ topdir }}.package    #  manages pkg install/remove
{{ topdir }}.config     # `manages config/remove of config files
{{ topdir }}.service    # starts/stop/remove the service

and use a conditional to manage the desired state, like say, in pseudo for a pillar

template:
  pkg:
    ensure: absent  # default: installed

the state can be

template-package:
  pkg.{{ pkg.ensure }}

I used to do that a lot in puppet, and kept things simple. On the directory for the yaml/json files, I'd go with a single directory wich can have any of them, with a name like 'local_values' or something meaningful (sad that 'config' is taken 😄)? and that's it?

My 2 GH cents ;)

[noelmcloughlin]

@aboe76 If there are concerns we should stop and discuss! If nothing else, to capture your concerns because this PR can be superseded by another PR to address your concerns. Beyond this PR there are many considerations that concern me.

Here is my take - assume a formula consists of two parts-

(1) state model
The standard use case is a single system (i.e. nginx) with three components-

• system packages/libs,
• system config
• system daemon

Three subdirectories models the system components and API calls-

template-formula/
    template/
           package/     (states:  template.package, template.package.clean)
           config/      (states: template.config, template.config.clean)
           service/     (states: template.service, template.service.clean)

(2) data model
Data artifacts are consumed by deployment automation model (Salt, but maybe Ansible, etc). We could try to figure out what directory structure works best.

template-formula/
    template/
           software/package/     (states:  template.software.package, template.core.package.clean)
           software/config/         (states: template.software.config, template.core.config.clean)
           software/service/        (states: template.software.service, template.core.service.clean)
           model/yaml            
           model/jinja             (*.jinja, *.j2, templates, macros too)
           model/json
           model/mako

Common formula patterns

This list is not exhaustive.

• Docker runs the daemon
  ======
  A docker container may provide the "service" but depend on package and config states for binding purposes (see https://docs.docker.com/storage/bind-mounts/). I have seen this pattern with nginx reverse proxy for example.

• Multiple services
  =====
  The iscsi-formula is the best example I can think of.

iscsi-formula/
           isns/package/     
           isns/config/      
           isns/service/ 
           target/package/
           target/config/
           target/service/
           initiator/package/
           initiator/config/
           initiator/service

• Toolchains
  ======
  A package state is needed, but the service state is not. Examples are Java, vim, IntelliJ, etc.

• Cross Platform packaging
  ======

• Tarball
• docker compose file
• git repo
• snap

I agree that this formula cannot handle everything but this needs discussion over longer period I guess.

[javierbertoli]

pkg subdir should not have a service management state file. See that this service file should be moved to template/service/init.sls, as per @noelmcloughlin ' s proposed structure

[myii]

As opened for discussion in the comment above.

[myii]

@javierbertoli Thanks for the excellent feedback. It's given me some food for thought about wider implementation issues including some TOFSv2 stuff. I now really believe that alongside tplfile, tpldir and tpldot, upstream should also provide topdir (or rootdir) as well by default. That gives a fixed base for all path and dot-based navigation throughout entire formulas.

@aboe76 @noelmcloughlin Please see @javierbertoli's excellent comments above and my responses. There are some important decisions to be made and your feedback will be invaluable.

[myii]

@javierbertoli Can I trouble you to also look at this comment above? I know we discussed this in our Slack room but I can't recall what the final conclusion was.

[javierbertoli]

@myii I think testing for clean states can be done on the same test run. From the top of my head, I think it should involve creating a clean suite which requires first the install states. Perhaps like:

suites:
  - name: default
  - name: clean
    provisioner:
      state_top:
        base:
          '*':
            - template       # Run all the "up" (install, config, service running) states
            - template.clean # Run all the "down" states

Then, a test/integration/clean subdir should be created, which verifies service is not running, config files do not exist and package is not installed.

I think that should do it. wdyt?

[myii]

@javierbertoli That sounds right. But how about this: how can we be 100% sure that than installation, configuration and service states even ran in the first place?! Meaning, the tests that would be written here would match the pre-run state exactly. What I really want to be able to do is:

1. Perform the installation-configuration-service states.
2. Run tests to ensure those have been completed successfully.
3. Perform the clean states.
4. Run tests to ensure those have been completed successfully.

That's the only way we can be 100% sure that the tests are passing.

[javierbertoli]

But how about this: how can we be 100% sure that than installation, configuration and service states even ran in the first place?

That's the point: the

suites:
  - name: default

already ensures the 'up' states run correctly and will do the Right Thing (TM).

So, if the default suite fails, the formula needs to be fixed.

Now, regarding the clean suite, as the 1st state in the clean suite runs the 'up' state again, you're sure that the clean suite is preparing everything to test the down states. 😄

The only cost is running the 'up' suite twice, but that's not much of a price, right?

[myii]

@javierbertoli That's brilliant, you've put my mind at ease! Of course, a little duplication isn't going to hurt. So, can we look forward to your wonderful clean testing PR...? This is the "price" of showing off your experience! No pressure though, honest.

[javierbertoli]

So, can we look forward to your wonderful clean testing PR...? This is the "price" of showing off your experience! No pressure though, honest.

LOL!! Sweetening the deal, uh? If only I were that eas...OK, I'll do it. Although I foresee a busy week, I'll try to submit a PR these days 😄

[myii]

@javierbertoli You walked into that one! But seriously, your offer is appreciated. Take your time; we all look forward to another masterpiece of inspec testing!

[myii]

CC: @aboe76 @javierbertoli @noelmcloughlin.

OK, that's everything done except for the README, which I'll push in a subsequent commit, soon. This has become quite a hefty change from what was submitted earlier so please look it over carefully.

[myii]

I've just pushed the changes to the README, so the work here is done.

[javierbertoli]

Nicely done, @myii !!! Just a minor addition to the README

[daks]

@myii I'm OK with the merge, I think we agree on most of the code changed

[noelmcloughlin]

Thanks for driving this PR through @myii - it can be merged & evolve through future PRs.

[myii]

@daks @noelmcloughlin Thank you both for your reviews (and all of the feedback along the way).

[aboe76]

if everybody is on board with this change, then LGTM. and discuss it on slack/riot/irc

[myii]

Thanks @aboe76.

@javierbertoli Would you be so kind as to hit the merge button?

[saltstack-formulas-travis]

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀