Add default state property to interface object #562
Conversation
|
@shermdog Still a WIP but wanted to get your eyes on it early if you have any comments. |
mikewiebe
changed the title
mikewiebe
changed the title
mikewiebe
changed the title
| get_command: 'show running interface all' | ||
| context: ["interface %s"] | ||
|
|
||
| all_interfaces: |
There was a problem hiding this comment.
Do we need this? We have the same property in interface.yaml (all_interfaces). can it be reused here?
There was a problem hiding this comment.
update: seems like you need it as you changed the object code to call the new yaml properties. what is the reason here? The yaml seems pretty much the same, may be something is missing. could you add a comment?
There was a problem hiding this comment.
There is a pretty big difference now. The self.all_interfaces method in interface.rb uses the show running-config interface | section '^interface' command along with a filter get_value: '/(.*)/' that gets all of the interfaces and associated interface data. We need all of the data so that we can keep track of all interfaces and weather or not the interface is in a default state.
For the other objects interface_portchannel.rb and interface_ospf.rb we only need a simple list of interface names and not the additional data under each interface.
| get_context: ['/^interface %s$/i'] | ||
| set_context: ["interface %s"] | ||
|
|
||
| all_interfaces: |
There was a problem hiding this comment.
see my comment above. applies here as well
lib/cisco_node_utils/interface.rb
Outdated
| def create | ||
| feature_vlan_set(true) if @name[/(vlan|bdi)/i] | ||
| config_set('interface', 'create', name: @name) | ||
| if @name[/ethernet/] && !@name[/ethernet.*\.\d+/] |
There was a problem hiding this comment.
This seems odd. Are we changing a property just to make it non-default even though the manifest may not specify it?
There was a problem hiding this comment.
Yes, I added this so that creating an interface for physical interfaces and setting properties to default values is idempotent. This will NOTE get called if the manifest does not specify it. I needed at least one property to be set to a non-default value under the physical interface for it to be in a non-default state when created. Without this if the ethernet interface is in the absent state (default state) and we try and create an interface via ensure => present and/or set properties under the interface to default values it will not be idempotent.
There was a problem hiding this comment.
This seemed like a fairly harmless way to allow us to manage physical interfaces and maintain idempotence, but I plan to demo this to the customer and get there 👍 or 👎 before keeping it.
|
@mikewiebe Couple of minor questions in the review as it was not clear what the need for all_interfaces and changing the description. if you can add some comments explaining them, then it is fine. |
|
👍 |
| get_command: "show running-config interface | section '^interface'" | ||
| get_value: '/(.*)/' | ||
| get_context: ~ | ||
| get_value: '/^interface (.*)/' |
There was a problem hiding this comment.
@mikewiebe section causes some issues for some providers; e.g. interface_service_vni. See my other comment below.
There was a problem hiding this comment.
@mikewiebe per IRC I'm going to add a separate all_interfaces getter specific to vni. That seems to be the only provider having issues.
| intf_list.collect! { |x| x.strip || x } | ||
| intf_list.delete('') | ||
| intf_list = intf_list.join(' ').split('interface') | ||
| intf_list.delete('') |
There was a problem hiding this comment.
@mikewiebe This massage doesn't work right with the section option.
intf_list = node.config_get('interface', 'all_interfaces')
# => ["interface Ethernet5/48", "", " service instance 22 vni", "", ""]
intf_list.collect! { |x| x.strip || x }
intf_list.delete('')
intf_list = intf_list.join(' ').split('interface')
intf_list.delete('')
puts intf_list
# => [" Ethernet5/48 service instance 22 vni"]
There are also other NU providers that use all_interfaces (e.g. interface_service_vni) so this list massage needs to occur for all of those providers.
There was a problem hiding this comment.
I was a little hasty. This code looks okay here but interface_service_vni needs similar massage code.
This updates adds the following:
@state_defaultused to indicate weather or not interface is in a default state. This information is only tracked for ethernet interfaces and excludes other types of interfaces that can be created and destroyed (vlan, loopback, sub-interfaces, port-channels etc...)self.interfacesmethod to issue theshow run interfacescommand once and gather default and non-default state info for each interface from the single show command.NOTE:
These changes are part set of diffs in the cisco puppet module that allow the
cisco_interfacetype to treat physical and logical interfaces as trulyensurableresources. Physical interfaces can now be created and destroyed logically so that they can be managed in the same way as a logical interfaces.Puppet Changes: cisco/cisco-network-puppet-module#470
Tested on:
N9k(I2, I6 and I7), N3k(I6), N7k(D1 and 8.2)