New inventory requires "hostname" even when not needed

[nickrusso42518]

Version 2.0.0 has introduced a regression requiring the hostname key to be defined for individual inventory hosts. This is a poor assumption since an environment with name resolution (DNS, local hosts file, or otherwise) should not require such information. For example, this inventory worked in Version 1.x because the hostname csr1000v is resolvable.

$ cat hosts.yaml 
---
csr1000v:
  groups: ["csr"]

$ ping csr1000v -c 1
PING CSR1000V (172.31.55.203) 56(84) bytes of data.
64 bytes from CSR1000V (172.31.55.203): icmp_seq=1 ttl=255 time=1.06 ms

--- CSR1000V ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 1.066/1.066/1.066/0.000 ms

However when running the existing Nornir runbook, an exception is raised because the hostname key is undefined. Even worse, the error message says ip or host must be defined which aren't even the right keys.

$ python3 get_facts_ios.py
napalm_get**********************************************************************
* csr1000v ** changed : False **************************************************
vvvv napalm_get ** changed : False vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv ERROR
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/nornir/core/task.py", line 62, in start
    r = self.task(self, **self.params)
  File "/usr/local/lib/python3.6/site-packages/nornir/plugins/tasks/networking/napalm_get.py", line 49, in napalm_get
    device = task.host.get_connection("napalm", task.nornir.config)
  File "/usr/local/lib/python3.6/site-packages/nornir/core/inventory.py", line 297, in get_connection
    **self.get_connection_parameters(connection).dict(),
  File "/usr/local/lib/python3.6/site-packages/nornir/core/inventory.py", line 347, in open_connection
    configuration=configuration,
  File "/usr/local/lib/python3.6/site-packages/nornir/plugins/connections/napalm.py", line 43, in open
    connection.open()
  File "/usr/local/lib/python3.6/site-packages/napalm/ios/ios.py", line 138, in open
    netmiko_optional_args=self.netmiko_optional_args,
  File "/usr/local/lib/python3.6/site-packages/napalm/base/base.py", line 82, in _netmiko_open
    **netmiko_optional_args)
  File "/usr/local/lib/python3.6/site-packages/netmiko/ssh_dispatcher.py", line 218, in ConnectHandler
    return ConnectionClass(*args, **kwargs)
  File "/usr/local/lib/python3.6/site-packages/netmiko/base_connection.py", line 170, in __init__
    raise ValueError("Either ip or host must be set")
ValueError: Either ip or host must be set

^^^^ END napalm_get ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I recommend two things:

1. Change the error message to say "The 'hostname' key must be defined for each host when name resolution is unavailable"
2. Change the logic to ensure that if name resolution succeeds that the exception is not raised.

[nickrusso42518]

I should note that the following inventory does resolve the issue but is not an appropriate long-term solution.

$ cat hosts.yaml 
---
csr1000v:
  hostname: "172.31.55.203"
  groups: ["csr"]

[dbarrosop]

A few notes:

1. It's not a regression. New inventory format was never meant to be backwards compatible.
2. That error is coming from netmiko as you can see in the traceback so there is nothing to change as it's the correct error. Nornir doesn't abstract napalm or netmiko or any other plugin so knowledge about how to use those is required.
3. The logic can't be changed because it will break the resolution with the groups, for instance:

---
# hosts.yaml
my_vm1:
    port: 65001
    groups: ["vms"]
my_vm2:
    port: 65002
    groups: ["vms"]
...
---
# groups.yaml
vms:
   hostname: whatever.fqdn.virt.cluster
...

Nornir doesn't assume defaults so if you don't specify hostname, port or something else it will assume it's not needed and won't be passed around, it won't assume that because you are trying to use ssh (netmiko or paramiko) it's going to be port 22, the underlying lib might but not nornir.

As a workaround we could pass an option to the Inventory to autopopulate the hostname if not set but that will be an explicit option. For instance:

nr = InitNornir(
    inventory={
       "options": "autopopulate_hostname": True  # defaults to False, current behavior
     }
)

[nickrusso42518]

I understand your first two points, but not the third. Why would multiple hosts share a common hostname? I see this as being equivalent to defining ansible_host at the group level, something I cannot see being a useful decision.

Overall this isn't a big deal so long as consumers clearly understand how to use nornir in that it doesn't try to "out think" its underlying libraries.

[dbarrosop]

Why would multiple hosts share a common hostname?

Virtualization, console servers; the hostname may point to the virtualization cluster or the console server while the port points to the specific VM/device.

In any case, it's mostly done for consistency. All attributes behave the same way. I understand the convenience of auto assigning the hostname from the yaml so I don't mind adding the optional feature.

@ktbyers @dmfigol opinions?

[ktbyers]

I think it is probably fine the way it is...in Ansible INI-style format it is more assumed that the name by itself is something you could connect to.

Once you are in YAML, I think the assumption that YAML entry name is what you would connect to is much lower.

I would probably be against making it an option in InitNornir as it adds complexity in the code, and it adds complexity in the inventory (i.e. where is "hostname" coming from: is it coming from the host-yaml entry, the group-yaml entry, the default-yaml entry or from this implied "name" entry). I think it is probably not worth adding that extra choice. I also think the mapping the schema is clearer in not doing this.

[dmfigol]

I am against changing the current defaults, but I am open to the feature "auto-populating the hostname" (as long as it is not default!) because "Explicit is better than implicit".
That said, I agree that the error Nick encountered is very confusing. I think we should fix our default connection plugins (netmiko, napalm) to check if the hostname is None and raise a custom exception with a sane message.

@nickrusso42518
you can specify the hostname instead of IP.

---
csr1000v:
  hostname: csr1000v
  groups: ["csr"]

[ktbyers]

I disagree with @dmfigol here:

I think we should fix our default connection plugins (netmiko, napalm) to check
if the hostname is None and raise a custom exception with a sane message.

It presupposes that we are going to put the right behavior into the connection-plugin and implement the proper checking there as opposed to (generally) letting the arguments flow through to the underlying library.

For example in Netmiko, Netmiko can use either a host argument or an ip argument. The host argument could be coming from the Nornir hostname, but it also could be stuffed into extras. Similarly, ip could be stuffed into Nornir-extras. Finally, Netmiko can have a serial-port connection.

I think it is a mistake to try to push this checking up into the Nornir connection-plugin. We would push a lot of complexity into the connection-plugin and likely have a set of bugs because of it.

[dbarrosop]

I agree with ktbyers. I think we should log what we are passing to the connection plugins but we shouldn't add any smartness. Imagine you write a connection plugin that uses some service discovery tool instead of DNS/IP to connect to devices/services, making hostname mandatory would break it or force you to add a dummy hostname.

[nickrusso42518]

I agree adding smartness would be messy and puts undue pressure on Nornir to know "too much" about the libraries it consumes. The auto-population option would be nice and seems relatively easy to implement. I imagine this would also slightly simplify v1 to v2 migrations.

Is there any reason not to enable this by default? I noticed @dbarrosop said Nornir doesn't assume defaults, and I also assume this is by design. Any particular reason for it, or is "Explicit better than implicit" a big part of the Nornir implementation plan?

All in all this isn't a big deal, and could probably be mitigated with a bit of documentation.

https://github.com/nornir-automation/nornir/blob/develop/docs/upgrading/1_to_2.rst

After saying hostname here, could say:

hostname (top-level host key name no longer used for connectivity; use IP or FQDN here)

[dmfigol]

@ktbyers @dbarrosop
But today the connection plugins assume some knowledge of the underlying library and convert our standard nornir parameters to library-specific parameters. Here is an example:
https://github.com/nornir-automation/nornir/blob/develop/nornir/plugins/connections/netmiko.py#L38

[dbarrosop]

Explicit better than implicit

Pretty much.

convert our standard nornir parameters to library-specific parameters

Yeah, and that's as far as we should go I think; we don't manipulate parameters, we don't assume a parameter is wrong, we just map them for convenience and if a user overrides that mapping that fact is respected (lines 49-50 in that same file). As of today nornir is not in the business of abstracting connection plugins and if we decide to do it (I am not against it) we should do it explicitly by having a wrapper around existing connection plugins and tasks so the user can explicitly choose to use the abstractions (something new) or be explicit about which one to use (current plugins/tasks).

[anuzellig]

I agree that an option to autopopulate the hostname would be nice, but for now I'm using the following as a simple workaround:

nr = InitNornir(config_file="config.yaml")
# Assign hostname if it was not set in the hosts file
for key, host in nr.inventory.hosts.items():
    if host.hostname is None:
        host.hostname = key

[ktbyers]

Closing as not an issue (i.e. the current behavior is the intended behavior).