Detection of changes in host network configuration by foreman

Support
21

Excellent debugging work @dannys, thanks! Generally, I’d say our support for bonds is weaker than we’d like - I think @Marek_Hulan would be the best person to comment on this, but he’s away at the moment. I’ve bookmarked the thread and will check back next week to make sure it’s not forgotten :slight_smile:

1 Like
22

That code is complicated and does not work well with fact importing because it’s built on assumption that XXX_Y or XXX.Y is always a bond or bridge. That’s not the real-world case, people or drivers may decide to name the interfaces with underscores or dots and then Foreman import code errors out.

23

Yes, I agree that the assumption based on NIC names is tricky. That probably requires some significant refactoring to fix that.

That may also be part of my problem, as I have a vlan interface named ‘XXX.Y’ (which I think is common for vlan interfaces), but that one is actually not recognized as a virtual interface. There doesn’t seem to be anything that could detect that it is a virtual interface, neither based on the XXX.Y naming scheme or other attributes.

24

I think you are hitting this problem:

25

Thanks! That seems part of the problem.

I’m saying ‘part of’ because it seems that the real magic for that change occurs in the FactParser::set_additional_attributes, which is called by FactParser::interfaces. Unfortunately the latter seems to be overruled by foreman salt, which doesn’t call FactParser::set_additional_attributes anymore. I’m trying to add that to foreman_salt as well and to see if that helps.

26

Adding FactParser::set_additional_attributes to FactParser::interfaces in foreman_salt actually helps (together with the fix from github). That solves at least my bonding + vlan issue with a disappearing physical interface. So that may be a possible solution.

Unfortunately, the more I dig into the code, the more issue I run into. Mostly in the foreman_salt plugin I think. I think it would be better to start creating individual issues for that.

27

Be aware that @Shimon_Shtein and I are working on the Salt importer right now - this just got merged (which fixes some issues for me, testing from you would be appreciated!)

1 Like
28

That is a nice reduction of code. What exactly should that fix?

Does it depend on a change in foreman core? Or is this all there is to it?

29

I was hitting an issue with 1.17.1 / salt 2017.6 where I couldn’t import hosts at all, the importer was bombing out - this was due to changes in core that hadn’t been reflected in the plugin.

30

Right, I didn’t upgrade to 1.17.1 yet, that is probably the reason why I didn’t have the same issue. I probably need to upgrade in order to test your fix properly.

Most of the issues I noticed seem to originate from the foreman_salt FactParser. Looking at the code in the master/development branch, these issues are probably still present.

I’ll give a short overview of the issues that I noticed:

  1. foreman_salt FactParser doesn’t add additional attributes based on VIRTUAL, BRIDGES, BONDS regex
    It seems that adding a call to ‘set_additional_attributes’ in the FactParser can fix that

  2. foreman_salt does not update / clear IP addresses of existing NICs if the NIC has no IP address anymore
    This is what we discussed earlier in this topic. That seems to originate from foreman_proxy_salt / foreman-node command.

  3. foreman_salt FactParser creates interfaces like bond0.1 for the IPv6 ip address definitions.
    That is strange and would problably create a separate interface with an IPv6 instead of adding it to the existing interface. However, foreman doesn’t get that far as it seems it tries to parse the IPv6 address as an IPv4 address, which fails with the messages:

        [app] [W]  Ip is too long (maximum is 15 characters)
    [app] [W]  Ip is invalid

    The cause seems to be this line: https://github.com/theforeman/foreman_salt/blob/master/app/services/foreman_salt/fact_parser.rb#L64

1 Like
31

I created the following bugreport: Bug #24139: foreman_salt does not recognize vlan interfaces (and bonds/bridges) as virtual interfaces - Salt - Foreman
and a pull request to propose a fix: https://github.com/theforeman/foreman_salt/pull/80

I also found this existing bugreport: Bug #12399: Error saving eth0.1 NIC for host failed - Salt - Foreman
which seems to be one of the issues that I discoved as well. I created a pull request for that also: https://github.com/theforeman/foreman_salt/pull/80

I didn’t add nor change tests though, as wasn’t able to get the tests up and running for the foreman salt plugin…

Any thoughts are welcome.

32

Small correction, it should be this pull request: https://github.com/theforeman/foreman_salt/pull/81

1 Like
33

Thanks @dannys, great work. I’ve found a small issue with one of the PRs for your attention, and then we can get this packaged up and released as 10.1.0 (I think)