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

WIP: Foreman interface #792

Closed
wants to merge 4 commits into from
Closed

Conversation

Fobhep
Copy link
Contributor

@Fobhep Fobhep commented Apr 15, 2020

@evgeni This is a first draft to resolve #757

It is far from being "done" but I'd appreciate your opinion on the general idea.
Also including @mdellweg since I shared some ideas with him alreday.

@mdellweg
Copy link
Member

The module looks fine so far.
Thanks a lot.

@evgeni
Copy link
Member

evgeni commented Apr 15, 2020

Thanks! This looks like a very good start!

Two questions/concerns:

  • This does not implement all the parameters an interface can have, are you aiming at including all when the PR leaves WIP status? If not +and I think that's totally fine), we should have a checklist somewhere so we can tick things off ;)
  • There are two ways to manage interfaces: using the interfaces resource as you did here (which I find is the more straight forward way) and by using the interfaces_attributes parameter to the hosts resource. I have not much experience with setups with compute resources, but I would expect that when you create a host in Foreman and that triggers a creation on the CR, having the full list of the interfaces is preferred over manipulating them afterwards. Especially as the host will usually be booted/booting after creation and changing interfaces will require a reboot. What I want to say: I fear we will have to port this to the host module as step 2 in the future (but we can reuse the foreman_spec etc, so this is the right way to start).

plugins/modules/foreman_interface.py Outdated Show resolved Hide resolved
plugins/modules/foreman_interface.py Outdated Show resolved Hide resolved
plugins/modules/foreman_interface.py Outdated Show resolved Hide resolved
plugins/modules/foreman_interface.py Outdated Show resolved Hide resolved
plugins/modules/foreman_interface.py Outdated Show resolved Hide resolved
plugins/modules/foreman_interface.py Outdated Show resolved Hide resolved
@mdellweg
Copy link
Member

@evgeni I think, that is not a very big problem, because with the ansible modules, creating and starting (in order to install) the host are two separate steps anyway. So it should be easy to let interface creation sneak in.

@bagasse
Copy link
Member

bagasse commented Apr 15, 2020

Same concern as @evgeni regarding compute resource. Two things to keep in mind:

  • Host is booted at creation time and depending on which way you provision your host, you have at least these three ways:

    • The host try to reach dhcp server to do a network boot (tftp/ipxe)
    • Foreman try to reach host to execute finish script (ssh)
    • Cloud init script may try to get external resources / configure network
  • Some compute resource in foreman don't support updates after host creation.

@Fobhep
Copy link
Contributor Author

Fobhep commented Apr 15, 2020

Thanks! This looks like a very good start!

Two questions/concerns:

* This does not implement _all_ the parameters an interface can have, are you aiming at including all when the PR leaves WIP status? If not +and I think that's totally fine), we should have a checklist somewhere so we can tick things off ;)

* There are two ways to manage interfaces:  using the interfaces resource as you did here (which I find is the more straight forward way) and by using the `interfaces_attributes` parameter to the hosts resource. I have not much experience with setups with compute resources, but I would expect that when you create a host in Foreman and that triggers a creation on the CR, having the full list of the interfaces is preferred over manipulating them afterwards. Especially as the host will usually be booted/booting after creation and changing interfaces will require a reboot. What I want to say: I fear we will have to port this to the host module as step 2 in the future (but we can reuse the foreman_spec etc, so this is the right way to start).

Thanks for the feedback!
True - I left out the BMC stuff so far, since I don't think I can test this anywhere on our infrastructure properly. The checklist is a great idea - I had this thought sometime in the past but forgot it in the mean time. We should probably attach it to the issue.

Second point - yes I share that fear 😛
I had a look at the other implementation and thought it is better to start with the easier approach (as you wrote).

@Fobhep
Copy link
Contributor Author

Fobhep commented Apr 15, 2020

Same concern as @evgeni regarding compute resource. Two things to keep in mind:

* Host is booted at creation time and depending on which way you provision your host, you have at least these three ways:
  
  * The host try to reach dhcp server to do a network boot (tftp/ipxe)
  * Foreman try to reach host to execute finish script (ssh)
  * Cloud init script may try to get external resources / configure network

* Some compute resource in foreman don't support updates after host creation.

@bagasse thanks also for the points.
I will add them to the issue, so we don't forget them.

@Fobhep
Copy link
Contributor Author

Fobhep commented Apr 15, 2020

I have some troubles with the module implementation.
Currently I am also not quite familiar with the apypie implementation and the usage of the entity resolving within the module so I'd appreciate some input :)

My testplaybook is firstly creating the well-known test-host from the host module test.

Afterwards the first "real" test step should create an additional interface to the already existing one from the test-host.

    - name: Create a new interface
      include_tasks: tasks/interface.yml
      vars:
        host_name: "test-host.{{ host.domain }}"
        interface_name: "test-interface"
        subnet_name: "{{ host.subnet }}"
        domain_name: "{{ host.domain }}"
        interface_managed: false
        interface_primary: false
        interface_provision: false
        interface_virtual: false
        expected_change: true

What it actually does is renaming the existing one, that has the name
test-host.example.net.

Looking at the foreman-production log I can also see that the module searches for an interface and then updates that one, rather than creating a new one.

The second task is supposed to prove idempotence by not changing anything:

    - name: Recreate interface
      include_tasks: tasks/interface.yml
      vars:
        host_name: "test-host.{{ host.domain }}"
        interface_name: "test-interface"
        subnet_name: "{{ host.subnet }}"
        domain_name: "{{ host.domain }}"
        expected_change: false

What it does is trying to create a new interface. I can see in the log, that the module is searching via api call for all interfaces of the host, but is not using the name of the interface eg "test-interface` for the search.
Consequently there is a POST Request and not a PUT Request.

Am I missing anything in my code regarding the entity resolving?

@evgeni
Copy link
Member

evgeni commented Apr 17, 2020

I think the issue is, that the interfaces api does not support search, so you always get "all" interfaces for the host (see https://theforeman.org/api/2.0/apidoc/v2/interfaces/index.html) and then you end up replacing the one you got instead of creating a new one (as it never matched the name of the interface)

@Fobhep
Copy link
Contributor Author

Fobhep commented Apr 17, 2020

@evgeni ok - thanks. that acutally makes sense.

However I am not sure I understand it fully.
This curl gives me all three interaces of a host

 curl -k https://or.deploy1.dev.atix/api/hosts/17/interfaces/
{
  "total": 3,
  "subtotal": 3,
  "page": 1,
  "per_page": 20,
  "search": null,
  "sort": {
    "by": null,
    "order": null
  },
  "results": [{"subnet_id":2,"subnet_name":"Test subnet4","subnet6_id":null,"subnet6_name":null,"domain_id":3,"domain_name":"example.net","created_at":"2020-04-15 15:02:17 UTC","updated_at":"2020-04-15 15:02:19 UTC","managed":false,"identifier":null,"id":24,"name":"test-interface.example.net","ip":null,"ip6":null,"mac":null,"mtu":1500,"fqdn":"test-interface.example.net","primary":false,"provision":false,"type":"interface","virtual":false},{"subnet_id":null,"subnet_name":null,"subnet6_id":null,"subnet6_name":null,"domain_id":null,"domain_name":null,"created_at":"2020-04-15 15:06:04 UTC","updated_at":"2020-04-15 15:06:04 UTC","managed":true,"identifier":null,"id":25,"name":"firlefanz232","ip":null,"ip6":null,"mac":null,"mtu":null,"fqdn":"firlefanz232","primary":false,"provision":false,"type":"interface","virtual":false},{"subnet_id":2,"subnet_name":"Test subnet4","subnet6_id":null,"subnet6_name":null,"domain_id":3,"domain_name":"example.net","created_at":"2020-04-15 15:07:38 UTC","updated_at":"2020-04-15 15:07:38 UTC","managed":false,"identifier":null,"id":26,"name":"test123-interface.example.net","ip":null,"ip6":null,"mac":null,"mtu":1500,"fqdn":"test123-interface.example.net","primary":false,"provision":false,"type":"interface","virtual":false}]
}

Whereas this one returns exactly one:

curl -k https://or.deploy1.dev.atix/api/hosts/17/interfaces/26 
{"subnet_id":2,"subnet_name":"Test subnet4","subnet6_id":null,"subnet6_name":null,"domain_id":3,"domain_name":"example.net","created_at":"2020-04-15 15:07:38 UTC","updated_at":"2020-04-15 15:07:38 UTC","managed":false,"identifier":null,"id":26,"name":"test123-interface.example.net","ip":null,"ip6":null,"mac":null,"mtu":1500,"fqdn":"test123-interface.example.net","primary":false,"provision":false,"type":"interface","virtual":false}

Is the correct approach then to use one of the "manual" entitiy searches (nailgun style?)

@evgeni
Copy link
Member

evgeni commented Nov 25, 2020

#979 is merged, so I am closing this.

@evgeni evgeni closed this Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow host interfaces management
4 participants