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.

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

Rethinking the way configuration management for network devices is handled #48945

Merged
merged 9 commits into from Aug 13, 2018

Conversation

Projects
None yet
4 participants
@mirceaulinic
Copy link
Member

commented Aug 6, 2018

What does this PR do?

There are two root causes for these changes:

  1. The community asked several times to be able to render ore than one template in one go. This is due to the fact that many would prefer having the templates grouped logically and eventually reuse them in other contexts. In other words, break a big Jinja template into smaller and easier to digest parts, that can be later reused in other states.

I agree with this, and with these changes, this is going to be possible from now on. For example, a state such as:

hostname_and_ntp:
  netconfig.managed:
    - template_name:
        - https://bit.ly/2OhSgqP
        - https://bit.ly/2M6C4Lx
        - https://bit.ly/2OIWVTs
    - debug: true
    - context:
        hostname: {{ opts.id }}
        servers:
          - 172.17.17.1
          - 172.17.17.2
        peers:
          - 192.168.0.1
          - 192.168.0.2

Would totally be valid (previously only template_name: https://bit.ly/2OhSgqP was possible).

  1. When I firstly implemented the net.load_template function more than 2 years ago, I still wasn't fully comfortable with Salt and I didn't have enough experience to evaluate what's the best way to render templates on the fly. This is why I ended up using file.get_managed: I was reading the template contents, dumping the rendered result into a temporary file, then reading the contents from there and load them into the device. Not only this is suboptimal, but Salt already has file.apply_template_on_contents for this exact purpose, without creating temporary files and all that mess. Using file.get_managed made me add a bunch of arguments such as template_user, template_mode etc., which never made sense, nobody actually used those as they refer to the temporary file which is anyway removed after this function is executed.

With this said, I'm swapping the usage of the file.get_managed with the more appropriate file.apply_template_on_contents. This also means that the template_user, template_mode, template_group et. al. arguments would no longer be used.

I have removed these arguments in b5a642e, but in case this should better be done in a later release I am happy to revert this commit and have the deprecated tags for the time being: 2ea0cbe

CC @gtmanfred

Notable byproducts:

  • Faster template rendering.
  • Fewer lines of code to maintain.

After discussion with @gtmanfred, @rallytime and @terminalmage, (2) is not enough good reason to drop the template hash arguments, therefore we'll continue using file.get_managed. However, deprecating template_user, template_group, etc.

@mirceaulinic mirceaulinic force-pushed the mirceaulinic:rethink-net-config branch 2 times, most recently from 6a2ae96 to 6a70bb1 Aug 6, 2018

template_source=None,
template_hash=None,

This comment has been minimized.

Copy link
@rallytime

rallytime Aug 6, 2018

Contributor

We can't remove any of these older kwargs without putting them on a deprecation path first. We'll need to support these with a warning for 2 feature releases. (So, Sodium would be the version to wan until and then remove the kwargs.) You can use the warn_until function in salt.utils.versions.

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Aug 6, 2018

Author Member

I understand that I should revert 6a70bb1.
In that case, they will no longer be used (although defined in the function header). How would you suggest to proceed? Leave them defined and unused?
Sorry I know this is a bit controversial, it's also the first time I'm deprecating something so I'm not sure what's the best way. Also, please remember that the arguments in subject, as of today, basically aren't useful / don't make sense to be there in the first place. Thanks!

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Aug 6, 2018

Contributor

so, you have **template_vars, so you don't want anyone that is still passing stuff to this, to inject extra arguments to the template_vars.

I would put a for loop at the top of the function, that iterates over all of the arguments here, and just tells the user that they do not need to specify them, and in 2 releases, they will be passed to template_vars if they do not stop using them.

That should be good enough, then in 2 releases we will just delete all that, and delete these variables from the function definition.

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Aug 6, 2018

Author Member

Thanks @gtmanfred - I will update this branch tomorrow.

template_source=None,
template_hash=None,

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Aug 6, 2018

Contributor

I do not like removing the template_hash. We should probably still keep this, even though cp.get_template does not support giving it a hash. We should be verifying the hash manually as part of the load_template process.

template_source=None,
template_hash=None,

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Aug 6, 2018

Contributor

so, you have **template_vars, so you don't want anyone that is still passing stuff to this, to inject extra arguments to the template_vars.

I would put a for loop at the top of the function, that iterates over all of the arguments here, and just tells the user that they do not need to specify them, and in 2 releases, they will be passed to template_vars if they do not stop using them.

That should be good enough, then in 2 releases we will just delete all that, and delete these variables from the function definition.

template_engine='jinja',
skip_verify=False,

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Aug 6, 2018

Contributor

These should be handled the same way as they are in the load_template module.

@gtmanfred gtmanfred added the Fluorine label Aug 6, 2018

@rallytime rallytime changed the base branch from develop to fluorine Aug 6, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

@mirceaulinic I have changed the base branch of this PR from develop to fluorine for inclusion in the Fluorine feature release.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

I spoke with @mirceaulinic a few minutes ago and helped clear up how the file state/module work internally. I think the confusion comes from the fact that file.get_managed does not have default values for most of its arguments. This is because it's meant to be used internally by the file states, which do have default values for these arguments. But if you're invoking the file.managed state via the __states__ dunder, you don't actually need to specify the user, group, mode, or attrs arguments. When they are not specified in a file.managed state, they assume the defaults (which in most cases means we don't enforce the ownership/mode/whatever).

However, it would still be good to have a function in the file module which accepts mostly the same arguments as file.get_managed but instead of returning the path to a file, we can instead just return the file's contents. But that would be something to consider for a future feature release.

In the meantime, I think it would be good to do the following:

  1. Continue to use file.managed.

  2. Set the defaults for template_user, template_group, template_mode, and template_attrs to None. If any of these have a non-None value, we should then add a warning to the state return:

    if template_user is not None:
        ret.setdefault('warnings', []).append(
            'The \'template_user\' argument is deprecated and '
            'has been ignored'
        )

I am also not a fan of using arguments like template_name, template_source_name, etc... If we are using the underlying file state/module support, then we should use the same argument names to provide a more uniform interface (similar to how we do things in the cron and archive state modules, which invoke file states to handle things like templating a source file).

@mirceaulinic mirceaulinic force-pushed the mirceaulinic:rethink-net-config branch 2 times, most recently from 8be9fa2 to 68019e3 Aug 8, 2018

@mirceaulinic

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2018

Hello everyone - thanks again for your time and good ideas. I have pushed some changes into this branch, please re-review.

@@ -2018,6 +2033,13 @@ def load_template(template_name,
}
)
else:
salt.utils.versions.warn_until(

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Aug 8, 2018

Author Member

I should have removed this in Fluorine (https://docs.saltstack.com/en/develop/topics/releases/fluorine.html#module-deprecations), but it turns out that it has more implications than I initially thought, postponing this till Sodium as well. Is that alright?

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Aug 8, 2018

Contributor

👍

@gtmanfred
Copy link
Contributor

left a comment

One small change, otherwise this looks good.

'Sodium',
('The \'{arg}\' argument to \'net.load_template\' is deprecated '
'and has been ignored').format(arg=deprecated_arg)
)

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Aug 8, 2018

Contributor

Can we remove these args from template args?

Should be a simple del template_vars[deprecated_arg]

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Aug 8, 2018

Author Member

Shall do.

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Aug 10, 2018

Author Member

Okay, just added: ed8cec1. Thanks!

template_hash_name = [template_hash_name]
elif not template_hash_name:
template_hash_name = [None] * len(template_name)
if template_hash and isinstance(template_hash, six.string_types) and not\

This comment has been minimized.

Copy link
@mirceaulinic

mirceaulinic Aug 8, 2018

Author Member

I was wondering what's @terminalmage's take on this? Does this make sense to you, or would you suggest doing this in a better way? Thanks.

This comment has been minimized.

Copy link
@gtmanfred

gtmanfred Aug 10, 2018

Contributor

Once this is added, this should be good to merge IMO

@rallytime rallytime requested a review from gtmanfred Aug 10, 2018

@mirceaulinic mirceaulinic force-pushed the mirceaulinic:rethink-net-config branch from 0a2220f to ed8cec1 Aug 10, 2018

@rallytime rallytime merged commit 24a6614 into saltstack:fluorine Aug 13, 2018

3 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.