host: add interfaces_attributes parameter

[achevalet]

I've just tested this and it looks to address a part of #757 (via the host API). I'm not sure but I think we could not make it idempotent, as for the compute_attributes.

[mdellweg]

Thank you for addressing that.
Why is it not idempotent? Don't we have enough information to see whether something needs change? I agree that it might not "just work" with the tools we have in place, but not that it is generally impossible.
Also i'd like to see the foreman_spec to specify the subitems in the interface list.

[achevalet]

I think that @evgeni 's comment in #788 also applies to interfaces_attributes:

• foreman does not return these attributes on a normal "show" of a host and you need an additional API request to get them
• not all compute resources allow updating these for existing machines
• different CRs support different options here, so validating them is a tad hard (we'd have just to accept "everything")

[evgeni]

So interfaces are even worse than compute_attributes…

There is an API for managing interfaces directly (without the Host) as you can see in #792, but doing so we would have the same problem we had with parameters "if we do it after the host creation, it will not propagate to the CR for initial deployment" (see #941).

And then the API supports both, managing the interface as the entity in Foreman but also managing the NIC in a virtual system.

So I think what we could do here is to take the spec @Fobhep started in #792 and use the "ansible" part of it (as returned by _foreman_spec_helper) to at least validate the input from the user (c.f. https://theforeman.org/api/2.1/apidoc/v2/hosts/create.html that lists all the possible entries in that list) and then feed that to foreman.

And idempotency is then left as an exercise to the reader

[mdellweg]

What if we added the interfaced in the same call for host creation, but in the update case we do it properly, separate and idempotent.
(host update does not have the problem that a host might be started without network.)

[evgeni]

discussion during triage:

• if we extend this PR to describe the various possible settings that go into an interface (I can provide pointers how to do that later), we are good to merge this and then continue to make it idempotent as a second step
• only thing to make sure: if we provide a list of interfaces at creation and then update the same host with the same interfaces, it should not duplicate them (I am kinda sure it won't but having tried that out would be good)

[evgeni]

example how to do a list that contains dicts with more strict content checks:

foreman-ansible-modules/plugins/modules/repository_set.py

Lines 252 to 255 in 04a98bc

	repositories=dict(type='list', elements='dict', options=dict(
	basearch=dict(),
	releasever=dict(),
	)),

also needs similar updates to documentation:

foreman-ansible-modules/plugins/modules/repository_set.py

Lines 46 to 63 in 04a98bc

	repositories:
	description:
	- Release version and base architecture of the repositories to enable.
	- Some reposotory sets require only I(basearch) or only I(releasever) to be set.
	- See the examples how you can obtain this information using M(theforeman.foreman.resource_info).
	- Required when I(all_repositories) is unset or C(false).
	required: false
	type: list
	elements: dict
	suboptions:
	basearch:
	description:
	- Basearch of the repository to enable.
	type: str
	releasever:
	description:
	- Releasever of the repository to enable.
	type: str

[achevalet]

@evgeni thanks for the pointers, I will amend the PR according to the apidoc.

[achevalet]

@evgeni I've updated the foreman_spec but those 3 points remain unclear to me:

• all parameters defined in the foreman_spec are passed even if they are not specified (with None value), so I have filtered them here:
  https://github.com/theforeman/foreman-ansible-modules/pull/979/files#diff-c1ec80744bf25b0075ff44bfa0b77cd0R448-R451
  Is it correct to do it that way?

• I've tried to use entity type for subnet, subnet6 and domain here:
  https://github.com/theforeman/foreman-ansible-modules/pull/979/files#diff-c1ec80744bf25b0075ff44bfa0b77cd0R390-R392
  But I get this error: implementation error: unknown type entity requested for ...

• The update of an existing host fails with:
  Failed to ensure entity state: 'NoneType' object is not iterable

Any help would be appreciated.

[evgeni]

Thanks @achevalet!

For the first question: yes, this looks correct. We do a similar kind of filtering for the top-level parameters automatically here:

foreman-ansible-modules/plugins/module_utils/foreman_helper.py

Line 320 in bc64c25

self.foreman_params = {k: v for (k, v) in self.params.items() if v is not None and k not in aliases}

But for sub options like in your case, the individual modules have been doing the filtering themself:

foreman-ansible-modules/plugins/modules/repository_set.py

Line 271 in bc64c25

repositories = [{k: v for (k, v) in sub.items() if v is not None} for sub in module.foreman_params.get('repositories', [])]

I think extending the automation to sub options might be a good idea (:eyes: @mdellweg), but sounds like out-of-scope for the current PR.

For the second question: I guess you mean type='entity', not type='element'? At least that matches the error you posted. Yes, this is currently not implemented for sub options either. I'll think how this can be made better.

And third: oof, I'll give it a try myself, not sure from just looking at the code.

[achevalet]

Sorry, yes I mean type='entity' (comment updated).
Thanks for your help!

[evgeni]

Thanks @achevalet! I've finally found some time to look at this.

Creating a host with interface_attributes seem to work (I only tested identifier/ip/mac, but there is no reason why it should differ for the other params), but when I try to run the same playbook a second time, I get an error: "Identifier has already been taken" -- which is because it's trying to create another interface, with the same parameters. I'll have a look what can be done to avoid this.

Another thing (but unrelated to the comment above): for subnet, subnet6 and domain you've used the "id", which is correct for the API, but confusing for the user -- they usually want to put the name in there. We should make the module do the lookup for us before merging this.

[achevalet]

Thanks the update.
On my side after rebase, I still have this error when I run the playbook a second time:
Failed to ensure entity state: TypeError: 'NoneType' object is not iterable
I'm using the minimum attributes (others are inherited from the compute profile):

        interfaces_attributes:
        - compute_attributes:
            network: "<my_network>"

Tested on ovirt and vmware, same error.

For the second point, should we update entity type to manage sub options?

[evgeni]

So I took your work here and applied the logic we discussed with @mdellweg above: if we create a host, we pass the interfaces directly, but if we update it, we use the dedicated interfaces api.

the code is here: https://github.com/evgeni/foreman-ansible-modules/tree/interfaces -- if you want I can just push it to your fork, which will update this PR and we can discuss further.

this also avoid the NoneType error you mention.

[evgeni]

also ping @Fobhep who tried to implement interfaces in #792 -- I honestly wonder if we really need a dedicated interfaces module?

[achevalet]

Awesome! Yes please push :)

[cmeissner]

There we go.

cc @cmeissner if you want to try it out? :)

We give it a try tomorrow I think

[Fobhep]

also ping @Fobhep who tried to implement interfaces in #792 -- I honestly wonder if we really need a dedicated interfaces module?

I think the idea back then was to use a dedicated module for a dedicated resource and avoid the more tough way using the host module :)
However - thanks to you and @achevalet this is probably not really needed anymore

[evgeni]

also ping @Fobhep who tried to implement interfaces in #792 -- I honestly wonder if we really need a dedicated interfaces module?

I think the idea back then was to use a dedicated module for a dedicated resource and avoid the more tough way using the host module :)
However - thanks to you and @achevalet this is probably not really needed anymore

Cool, so you'd say merging this would close #757 ans #792?

Oh and note to myself, this needs a changelog entry

[achevalet]

Suggested change

	existing_interface = next((x for x in current_interfaces if x.get(unique_identifier) == interface[unique_identifier]), None)
	existing_interface = next((x for x in current_interfaces if unique_identifier and x.get(unique_identifier) == interface[unique_identifier]), None)

[achevalet]

note: the parameter name actually defines (or is defined by) the host's FQDN and identifier is optional.
If those are not specified, it ends up with an empty interface list.

[evgeni]

Yeah, so I needed to "find" an existing interface somehow to know if we're updating it or creating a new one. My idea was to look if there is an interface defined with the same identifier, hostname or mac (and I didn't add mac in my code, ups). This behavior is not documented yet.

[achevalet]

I think it would make sense for PXE deployment (mac is probably the best option since it is required) but for image based provisioning the changes won't be propagated to the VM since most compute resources does not manage host update (afaik, same applies to compute_attributes). Shouldn't we just exclude interfaces_attributes in this case?

[evgeni]

Is there anything that we could ask the user to provide to make the entry unique? If you for example use identifier:fam01 (like I did in the tests), would the interface be properly deployed with that name inside the OS, even if we don't define ip/mac to match on?

[evgeni]

So I had a few more thoughts:

• I'll look what hammer does tomorrow
• If both the user and API both provide exactly one interface, that's the one we're updating, no need to match
• In the case we couldn't find an unique identifier, proceed to add a new interface and warn the user that this is probably not what they want

[achevalet]

Sounds good! We just need to be careful with Foreman setting ignore_puppet_facts_for_provisioning, disabled by default. Puppet facts may update some values and/or add new interfaces. If new interfaces are created by puppet facts, we need to match, and if the identifier has changed, we might create a new interface at each run, which is not what we want :)

[evgeni]

I've implemented the "only one" and "warning" proposals from above.

Now to hammer:

• Given the non-declarative way hammer is designed, you have individual commands to "add", "update" and "delete" host interfaces.
• Let's ignore adding, that is trivial.
• Deleting an interface requires two inputs: host id (or name) and interface id (which you need to look up manually via hammer host interfaces list before)
• Updating also (technically) requires host id/name and interface id, but nothing enforces the entry of the ID, and if you don't you get the good old "Interfaces some interfaces are invalid" error because it tries to add an interface with an identifier that's already present.

So, all in all, I'm no way wiser now.

[achevalet]

Now the update tries to remove the interface :)

fatal: [localhost]: FAILED! => {"changed": false, "error": {"errors": {"primary": ["can't delete primary interface of managed host"], "provision": ["can't delete provision interface of managed host"]}

[Fobhep]

also ping @Fobhep who tried to implement interfaces in #792 -- I honestly wonder if we really need a dedicated interfaces module?

I think the idea back then was to use a dedicated module for a dedicated resource and avoid the more tough way using the host module :)
However - thanks to you and @achevalet this is probably not really needed anymore

Cool, so you'd say merging this would close #757 ans #792?

Oh and note to myself, this needs a changelog entry

Given the fact that our inital plan was anyways to port all features from a standalone module to the host module I would say so, yes. I don't see a lot of people using an extra module to handle interfaces, when really I can also do it via the host-module. But if someone disagrees, I will gladly be persuaded otherwise

[cmeissner]

Works for me. Nice will be the lookup for entities e.g. subnet and domain.

[evgeni]

okay, so I've pushed some changes that make subnet/domain lookup work, but it's not as pretty as I'd like it to be

ping @mdellweg for ideas

quick recap of our problem: currently, our lookup code only looks at entity and entity_list elements of the "main" spec, but in this module (and in some other modules before that), we actually have a list of hashes, and some parts of those hashes are entities. 🤯 I've now implemented the lookup specifically for the interfaces_attributes, but that specific thing makes it even worse, as we need to (yes, really: https://projects.theforeman.org/issues/31311) to submit the interfaces differently on create and update.

[evgeni]

Maybe we can also fix the apidoc to accept the interfaces on host update too...

Edit: given that's what hammer does, it probably should work with a small apidoc patch

[evgeni]

But even if we use "same api request", the lookup is needed the same way, so let's not overcomplicate things.

Or rather: I needed to apply enough other workarounds to make update work using the "same api request" so that it's not worth it.

[mdellweg]

Added my thoughts.

[mdellweg]

Interesting side effect.

[mdellweg]

I left some comments. Nothing that should stop us from merging necessarily.
But consider the doc improvement.

[mdellweg]

This is not yet fully recursive, right?
Maybe we can incorporate the nested lookup into the _lookup_entity eventually, in a followup pr...

[evgeni]

Yeah. It's not. But I'd also defer that to another PR :)