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

Defining verbs used in modules #75

Open
itdependsnetworks opened this issue Sep 21, 2017 · 16 comments
Open

Defining verbs used in modules #75

itdependsnetworks opened this issue Sep 21, 2017 · 16 comments

Comments

@itdependsnetworks
Copy link

Proposal: Defining verbs used in modules

Author: Ken Celenza <@itdependsnetworks> IRC: itdependsnetwork

Date: 2017/09/21

  • Status: New
  • Proposal type: process
  • Targeted Release: 2.5
  • Associated PR: N/A
  • Estimated time to implement: N/A

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

  • How to create declarative configurations
  • How to manage complex objects
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.

  • 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
  • Absent - Ensure it does not exist, as well understood
  • Present - Ensure it does exist, as well understood
  • Declarative - Ensure this and only this exist within the feature at hand
  • Update-Absent - Remove specific object from larger group, 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)

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
Copy link

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
Copy link
Author

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: ansible/ansible@751eab1#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
Copy link
Member

bcoca commented Sep 21, 2017

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

  • get: we already have list, query and _facts modules, other modules just return the data if no state is supplied i.e not absent nor present. I would welcome some normalization here, but not sure which option is best.
  • declarative: i think this is a 'specific case of present', we already have modules that implement this via 2nd option append=no|yes.
  • Update-*: also implemented in some modules by having a recursive or dependencies extra option.

@itdependsnetworks
Copy link
Author

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

@bcoca
Copy link
Member

bcoca commented Sep 21, 2017

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

@stacywsmith
Copy link

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

  • Have separate modules for "configuration" and "facts".
  • "configuration modules" should return information on the current/resulting configuration of the feature/object, but should not return "operational state".
  • "fact" modules should return "operational state".
  • A general one to one mapping between a "configuration module" and "fact module" for a given feature/object. (i.e.) not one big _fact module even if it supports a gather_subset concept.
  • Restrict state to absent|present
  • Add append: yes|no to handle the "declaritive", and "update-" modifications.
  • If "configuration modules" only return "configuration state", I don't know that you need a modifier to turn on/off the return data, but that would be fine if people think it's needed.

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
Copy link

privateip commented Sep 21, 2017

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
Copy link

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
Copy link

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
Copy link

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
Copy link

ktbyers commented Sep 21, 2017

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
Copy link
Author

@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
Copy link
Contributor

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

  • facts modules
  • setup and customizing what it returns by default (plugins e.g. for custom enterprise stuff)
  • updating facts from related modules (if you change the hostname using a module, the hostname fact could be updated by the module)
  • namespacing (and reorganizing some of the legacy stuff)
    • Too much date_time information that would best be left to Jinja2 filters
    • Interface information is in there multiple times (clobbering the root namespace)

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Member

bcoca commented Sep 22, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants