ansible / proposals

Repository for sharing and tracking progress on enhancement proposals for Ansible.
Creative Commons Zero v1.0 Universal
93 stars 19 forks source link

Defining verbs used in modules #75

Open itdependsnetworks opened 6 years ago

itdependsnetworks commented 6 years ago

Proposal: Defining verbs used in modules

Author: Ken Celenza <@itdependsnetworks> IRC: itdependsnetwork

Date: 2017/09/21

Motivation

The verb usage of absent/present is well understood, and helps developers and operators understand and deploy configuration in an idempotent fashion. However, it has it's limitations, specifically around complex objects which are often seen in the networking space. It is difficult to ensure full intent of a configuration or feature with these two verbs.

I would like to do something similar to what is done in HTTP, (GET/PUT/POST) but hopefully simpler.

Problems

bgp_neighbors:
  - neighbor: 10.1.1.1
    asn: 64801
    as_prepend: 2
  - neighbor: 10.2.2.2
    asn: 64802

In the above, how do I say that not only is 10.2.2.2 configured but does not have AS prepend on it? How can I say that this is the final intent of all bgp neighbors?

Firewall rules are especially prone to these issues but their complexity does not lead to easily understood examples.

Solution proposal

I would like to agree on the verb's used and their definition moving forward. While I expect absent/present to be the primary verb's, there simply is no current way to manage configurations of complex object types.

The intention would not be that every module would need/have/require each verb, but simply that it should be understood that there is a common language

Dependencies (optional)

N/A

Testing (optional)

N/A

Documentation (optional)

N/A

Anything else?

The names themselves are not important to me, simply that we agreed upon them, have a common language, and have a shared understanding of what they do. The verbs used should be all encompassing leaving no reason to add more verbs in the future.

I would also like to warn against worrying about the specific names until we agree which verbs are needed.

pinging @gundalow @privateip @dagwieers @stacywsmith @ktbyers

privateip commented 6 years ago

Get - Get operational data that is related to configuration data. While traditionally handled by _facts, it is not intended to return bgp adjacencies as an example

This is facts and should remain so imho. I don't see a need for this. If there is information missing from facts modules, then lets fix it there instead of blurring the lines here

Absent - Ensure it does not exist, as well understood Present - Ensure it does exist, as well understood

Makes sense

Declarative - Ensure this and only this exist within the feature at hand Update-Absent - Remove specific object from larger group,sd not worrying about anything else (do not really like the name, simply wanted to put something down) Update-Present - Add specific object from larger group, not worrying about anything else (do not really like the name, simply wanted to put something down)

How area these related to the proposal here?

itdependsnetworks commented 6 years ago

GET

The logic used to understand bgp neighbors, should already be in the bgp module, why not keep it there?

@privateip Also this idea comes based on you removing features from facts, which made me think this was not your intention: https://github.com/ansible/ansible/commit/751eab187f51cdc58a4c112abcb4265b7112ef1b#diff-83a6bf7294d0c5ac59ee458de130f795R355

How area these related to the proposal

Overlap for sure, but I took the previous proposal as defining the issues with current implementation where is this is just agreeing on nomenclature.

bcoca commented 6 years ago

I don't think all of these should be a 'state', in many cases they are just modifier of the basic states.

itdependsnetworks commented 6 years ago

@bcoca I'm taking this as you agree, we need to define the nomenclature as it is not well known

bcoca commented 6 years ago

always .. its just been hard to get people to agree what it should be, I've been trying for a few years now.

stacywsmith commented 6 years ago

@itdependsnetworks

Get - Get operational data that is related to configuration data. While traditionally handled by _facts, it is not intended to return bgp adjacencies as an example

In your particular example, what "operational data" would you expect bgp_neighbors to return, and why would that not include "bgp adjacencies"? I'm just trying to further understand your proposed definition of the "get" verb.

Maybe you can also clarify whether or not "get" should return the current "configuration data" for the module. That is, do you envision that state: get on bgp_neighbors should return information on what BGP neighbors/parameters are actually configured (configuration data) and/or "operational data" about those BGP neighbors (which in my mind would likely include bgp adjacency information for this particular example.)

@privateip

Get - Get operational data that is related to configuration data. While traditionally handled by _facts, it is not intended to return bgp adjacencies as an example

This is facts and should remain so imho. I don't see a need for this. If there is information missing from facts modules, then lets fix it there instead of blurring the lines here

I tend to agree with you in this case, but I feel you're being inconsistent on this, and I feel we need to further clarify this.

Isn't what @itdependsnetworks is proposing exactly what's been implemented with declarative intent? In the "interfaces" example that we've been discussing for declarative intent the module is specifically gathering and checking the "operational state" of the interface.

Wether or not a "configuration module" should also gather/return/check "operational state" is at the heart of the debate on #70. The general idea behind declarative intent seems to have assumed that the answer was "yes".

Personally, I think the answer should be "no" as you seem to be recommending in this case. If there is a "configuration module" and a separate "fact module", then Ansible syntax already provides the ability for users to customize the "intent" or "expectation" from their configuration using playbooks/roles.

The idea behind declarative intent seems to have been a hope to "simplify" this, but then we end up in the trap of re-implementing things like pause, wait_for, conditionals, loops, etc. within a module (or at least the module infrastructure.)

@bcoca I appreciate your insight into what other modules have already implemented, and I agree that "get", "declaritive", and "update-" as defined by the proposal are really modifiers of "absent"/"present" state. I think it's important to keep them separate so that you can add config with "present" or delete config with "absent" and independently choose wether or not to get the resulting "state" (whatever we decide that is) with an additional argument. I also agree with your assessment that "declaritive" is better handled with an append: yes modifier.

Here's what I would propose:

I also think this proposal and #70 have interaction with "platform agnostic network modules". When returning state ("configuration" or "operational") we now have to map from vendor->agnostic, not just agnostic->vendor. We must consider how this is handled if there's additional vendor-specific configuration and/or arguments which a particular vendor doesn't support.

privateip commented 6 years ago

I tend to agree with you in this case, but I feel you're being inconsistent on this, and I feel we need to further clarify this. Isn't what @itdependsnetworks is proposing exactly what's been implemented with declarative intent? In the "interfaces" example that we've been discussing for declarative intent the module is specifically gathering and checking the "operational state" of the interface.

Declarative intent always return the both operational and configuration data regardless of any argument. This would, if I am reading this right, strictly pin the return of operational data to the use of an argument verb (get in this case). If my understanding is wrong, happy to be corrected

stacywsmith commented 6 years ago

Declarative intent always return the both operational and configuration data regardless of any argument. This would, if I am reading this right, strictly pin the return of operational data to the use of an argument verb (get in this case). If my understanding is wrong, happy to be corrected

Yes. Your understanding of the proposal matches mine. In the proposal, operational data would be returned depending on the value of "get".

However, I don't understand why that matters. You stated:

This is facts and should remain so imho. I don't see a need for this. If there is information missing from facts modules, then lets fix it there instead of blurring the lines here

Your statement seems true regardless of whether or not the "get" value is specified. If "this is facts and should remain so" applies in this case, why doesn't it also apply to the idea of "declaritive intent"?

privateip commented 6 years ago

Take the following three examples

net_interface:
  name: eth0
  enabled: yes
  state: present
{
  interfaces: [
    { 
      name: eth0,
      {{ context_specific_data }}
     },
     {
        name: eth1,
        {{ context_specific_data }}
      }
    ]
}

The above will attach to the device, administratively enable eth0 and then return config and operational data.

Example 2

net_interface_facts:
  gather_subset: interfaces

The above will return fact information about the interfaces of the device regardless of context. So the above module (which doesn't exist yet by the way), could / would potentially return something like:

{
  interfaces: [
    { 
      name: eth0,
      {{ a_whole_bunch_of_interface_data_with_no_regards_to_context }}
     },
     {
        name: eth1,
        {{ a_whole_bunch_of_interface_data_with_no_regards_to_context }}
      }
    ]
}

Example 3

net_interface:
  name: eth0
  state: get
{
  interfaces: [
    { 
      name: eth0,
      {{ what_goes_here }}
     },
     {
        name: eth1,
        {{ what_goes_here }}
      }
    ]
}

Examples 2 and 3 are essentially the same with {{ what_goes_here }} has no context. If {{ what_goes_here }} has context then Examples 1 and 3 are the same.

Whats the different? The cost of retrieving the operational data. For something like BGP, the operational data is gathered in context of the module to limit the scope of how much data needs to be collected. If fictitious BGP module has to grab all of the operational data on every run even though the module might only use a fraction of it, the processing cost could be high on the device. Of course the same plenty is incurred when using facts modules but that becomes more manageable at the playbook level.

stacywsmith commented 6 years ago

Sorry @privateip , I feel like I'm not fully groking your examples. In particular, I don't understand:

Examples 2 and 3 are essentially the same with {{ what_goes_here }} has no context. If {{ what_goes_here }} has context then Examples 1 and 3 are the same.

I guess I also just don't understand what you feel the difference between {{ context_specific_data }}, {{ a_whole_bunch_of_interface_data_with_no_regards_to_context }}, and {{ what_goes_here }} should be in the return value of the three different examples.

Is Example 1 a "configuration module" which returns both "configuration data" and "operational" date for all interfaces?

Is Example 2 a "fact module" which returns "operational data" for all interfaces?

If so, is the "operational data" returned by Example 1 a subset of the "operational data" returned by Example 2?

Are you proposing that both a "configuration module" (Example 1) and a "fact module" (Example 2) should exist for a given feature (such as net_interface)?

ktbyers commented 6 years ago

I don't like the get operation. Most of the networking modules are clearly config operations or information gathering operations. My initial reaction is that it doesn't make sense to overload config operations with information gathering. I think it is more logical for information gathering to be a part of facts or as separate modules.

I also would be worried on how much work this (adding a get operation) would create and how confusing it could potentially be. What information is returned in which contexts and how much screen-scraping parsing is going to be embedded in the modules.

I like declarative, but not the term (i.e. I think using declarative is confusing here). Discussions on this should just be part of the aggregate proposal.

update-absent and update-present sound fairly problematic to me (definitely don't like the terms). You can have potential nesting and multiple level of hierarchies so I get worried this is going to be very challenging. I would want to see more examples and get a better understanding of the implications and the places this would break (definitely alarm bells going off...that this will be hard to pull-off without lots of bugs/side-effects).

itdependsnetworks commented 6 years ago

@bcoca Can you share examples of: append, recursive and dependencies. These may be enough to fit all needs, I personally prefer one parameter to define, state in this case, however I rather just define more than anything, everything else is just semantics/preference.

@privateip I have similar confusions that @stacywsmith is highlighting.

Declarative intent always return the both operational and configuration data regardless of any argument. This would, if I am reading this right, strictly pin the return of operational data to the use of an argument verb (get in this case).

So is there a mechanism to only get operational data without setting any configuration?

The cost of retrieving the operational data. For something like BGP, the operational data is gathered in context of the module to limit the scope of how much data needs to be collected. If fictitious BGP module has to grab all of the operational data on every run even though the module might only use a fraction of it, the processing cost could be high on the device.

I think I am lost here, the point of the get was to reduce processing cost to a single context, instead of a global one.

Again, it seems as if there was active removal of at least nxos_facts, if that is the case, does that not signal ansible's preference to go away from *_facts? If so what is the new direction? If not, why convert functionality to legacy?

dagwieers commented 6 years ago

/me thinks we still need a holistic view on facts, including:

And there was a myriad of other stuff related to facts that I can't remember. Should make a wiki page were people can add ideas/naming/interface changes.

itdependsnetworks commented 6 years ago

@dagwieers well put, I'm fine for facts being the getters just did not seem like that was the direction. Also to add should have normalized responses regardless of vendor, (which may or may not be true already?)

dagwieers commented 6 years ago

I don't know what the direction is, there's a lot of stuff the needs a redesign (and some of that was already in the pipeline), but I think we need to know first what the ideal end-result would be before making any steps in any direction. Example: Renaming ansible_os_distribution in ansible_facts.ansible_os_distribution is not helping to reach the ideal end-result (and improved user experience) we should be aiming for.

bcoca commented 6 years ago

+1 for modules that change 'known facts' returning the updated fact, hostname is a good example, this is just something that module authors can do now w/o any code changes to core

@dagwieers we specifically reverted the facts namespacing from 2.4 until we could properly do ansible_facts.os_distribution (hopefully in 2.5)

but I think that the facts redesign is not germane to this discussion, even though the functionality does depend on facts or information that could be consider such.