Fixes #37367 - Switch to 'network' directive instead of ifcfg #9961

ShimShtein · 2023-12-19T12:45:06Z

Use kickstart's network directive to create interfaces according to foreman's DB instead of using ifcfg files.

ekohl

I didn't think of introducing a macro for this. Previously used a snippet.

app/services/foreman/renderer/scope/macros/kickstart.rb

ekohl · 2023-12-19T13:43:06Z

Also, isn't there a Redmine issue for this?

ShimShtein · 2023-12-20T09:39:50Z

Had to fix snapshot tests and added a meets_requirement method to deal with requirements a bit more cleaner.

@ekohl, ready for another round.

app/models/operatingsystems/redhat.rb

app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb

ekohl · 2023-12-24T03:47:01Z

app/services/foreman/renderer/scope/macros/kickstart.rb

Is it really wise to put so much kickstart logic into macros? They make it harder to maintain for users if they need changes. Why can't this be a snippet?

We are putting in the macro file logic about how to translate foreman objects into kickstart language. I think the translation is not use-case dependent. For example iface.ip will always be translated to network --ip=#{iface.ip}, so this logic can be hard-coded into our macros. I do think that we could add modification flags later on, if we see that there are parts of the Interface object that should not always be translated. This will leave enough flexibility to the user, but will maintain integrity with the kickstart language.

ekohl · 2023-12-24T03:48:31Z

app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb

I think we should get rid of this template. RHV is deprecated and going away and oVirt doesn't look too healthy these days.

I think it's an issue that should be properly discussed in the community (started a thread: https://community.theforeman.org/t/proposal-to-drop-support-for-ovirt/36324) and anyway it's a matter for a separate PR.

I meant to only drop the kicjstart tenplate. The whole of oVirt is a separate discussion. I don't know if you even still can provision with this template or if we provide functionality for something that no longer exists.

Having said that, I also noticed I can no longer compile the oVirt SDK on Fedora so that alone can be a reason to drop it.

I'll weigh in on the RFC later, when I'm home

ekohl · 2023-12-24T03:48:54Z

app/models/operatingsystems/redhat.rb

Overall I'm not too keen on mapping Anaconda capabilities into the operating system. Anaconda has complex cherry picking and if anything, it deserves its own class.

I think that what I implemented here is in tune with what you are saying: I didn't create a hard-coded capability matrix with Anaconda (which is problematic, I agree).
I have implemented only a version check, which is not directly related to Anaconda. Theoretically we could implement such a check in the base class, but I have avoided it, since it can be a challenge on its own, and currently I don't see too much value for it.
The usage of the version check is implemented inside the kickstart template, and there I am using the versions for both Fedora and Rhel-compatible systems, according to the documentation. This way the logic for each compatibility check is kept with the usage.

ShimShtein · 2023-12-25T10:33:32Z

@ekohl ready for another round.
Thanks a lot for keeping me honest and well-documented :)

ShimShtein · 2023-12-25T14:09:05Z

[test unit]

ekohl

Having thought about it more, I'm against a macro and instead want to see a snippet. This allows small fixes by users and experimentation with new features by copying. A macro doesn't allow that.

app/models/operatingsystems/redhat.rb

app/services/foreman/renderer/scope/macros/kickstart.rb

ekohl · 2024-02-21T12:29:01Z

test/unit/foreman/renderer/scope/macros/kickstart_test.rb

+    end
+
+    test 'should skip non-managed interfaces' do
+      iface = FactoryBot.build(:nic_base, :primary => true)


Shouldn't this be actually non-managed?

Suggested change

iface = FactoryBot.build(:nic_base, :primary => true)

iface = FactoryBot.build(:nic_base, :primary => true, :managed => false)

ShimShtein · 2024-02-22T12:12:09Z

@ekohl Ready for the next round. Refactored the helper to a snippet

app/views/unattended/provisioning_templates/provision/kickstart_default.erb

ekohl · 2024-02-22T12:14:43Z

test/unit/foreman/templates/snippets/kickstart_network_interface_test.rb

+  end
+
+  def render_template(iface, host:, use_slaac:, static:, static6:)
+    @snippet ||= File.read(File.expand_path('../../../../../app/views/unattended/provisioning_templates/snippet/kickstart_network_interface.erb', __dir__))


Wouldn't Rails.root.join() be easier?

ekohl · 2024-02-27T10:14:32Z

app/services/foreman/template_snapshot_service.rb

@@ -105,6 +105,7 @@ def host4dhcp
        name: 'snapshot-ipv4-dhcp-el7',
        subnet: FactoryBot.build(:subnet_ipv4_dhcp_for_snapshots),
        interfaces: [ipv4_interface])
+      host.stubs(:managed_interfaces).returns(host.interfaces)


I'd prefer to build a host with the right interfaces. I think it means ipv4_interface needs to be marked as managed.

managed_interfaces is based on DB scopes, it does not work correctly for non-saved hosts:

host = FactoryBot.build(:managed_host, name: 'snapshot-ipv6-dhcp-el7', interfaces: [FactoryBot.build(:managed_interface, managed: true)]) host.managed_interfaces # => [] host.save! host.managed_interfaces # => [<Nic::Managed ... >]

ekohl · 2024-02-27T10:15:28Z

app/views/unattended/provisioning_templates/provision/kickstart_default.erb

-  else
-    network_options.push("--nodns")
-  end
+# start with provisioning interface, then other non-bond interfaces and the bonds at the end


Can you explain why this is needed? Is it a requirement by Anaconda?

By default only the first interface is activated, and I want it to be the provisioning interface.

I can't find it now, but I have read somewhere in the docs, that the commands are executed in the order they are written, so I would prefer to have all the "daughter" interfaces defined before I define them in the bonds.

ekohl · 2024-02-27T10:18:11Z

app/views/unattended/provisioning_templates/snippet/kickstart_network_interface.erb

+if @iface.respond_to?(:mtu)
+  network_options.push("--mtu=#{@iface.mtu}") if @iface.mtu


Suggested change

if @iface.respond_to?(:mtu)

network_options.push("--mtu=#{@iface.mtu}") if @iface.mtu

if @iface.respond_to?(:mtu) && @iface.mtu

network_options.push("--mtu=#{@iface.mtu}")

ekohl · 2024-02-27T10:19:48Z

app/views/unattended/provisioning_templates/snippet/kickstart_network_interface.erb

+end
+
+# dual stack MTU check
+raise("IPv4 and IPv6 subnets have different MTU") if subnet4 && subnet6 && subnet4.mtu.present? && subnet6.mtu.present? && subnet4.mtu != subnet6.mtu


I think we should implement this check in @iface.mtu instead. Perhaps not in this PR because I don't know how big the implications are.

Created an issue: https://projects.theforeman.org/issues/37216

ekohl · 2024-02-27T10:21:56Z

test/unit/foreman/templates/snippets/kickstart_network_interface_test.rb

+      assert_match(/--nameserver/, actual)
+      assert_match(/192.168.42.2/, actual)
+      assert_match(/192.168.42.3/, actual)


Ideally a test only has a single assertion. This is a much stricter check. It's showing up more often in this file, so only commenting once.

Suggested change

assert_match(/--nameserver/, actual)

assert_match(/192.168.42.2/, actual)

assert_match(/192.168.42.3/, actual)

assert_includes(actual, '--nameserver=192.168.42.2,192.168.42.3')

ekohl · 2024-02-27T10:26:52Z

app/models/operatingsystems/redhat.rb

@@ -72,4 +76,21 @@ def pxe_kernel_options(params)
  def minor_version_help
    '0, 6.1810'
  end
+
+  def fedora?


My biggest concern with these is that they're no longer a pure implementation of the OperatingSystem interface. So you can end up with a template that calls it on a Debian OS or something else. Perhaps make fedora? and rhel_compatible? private methods and implement meets_requirement? on OperatingSystem? For Debian/Ubuntu I can see similar uses.

I have moved the meets_requirement to the base OperatingSytem class, although each family would need to override it with the correct keyword arguments to make it more readable. In the current state, the code will not throw an error, but would return false for families that are not overriding the default implementation.
I do prefer this method of implementation, since it will make throw meaningful exceptions when incorrect arguments are used, unless you would like to see something like:

host.operatingsystem = ubuntu_20_04 do_something if host.operatingsystem.meets_requirement(rhel: 8)

As for fedora? and rhel_compatible?: I do think that these methods should be public, but on the derived class. So I would like to allow the following code:

do_something if host.operatingsystem.is_a(RedHat) && host.operatingsystem.fedora?

app/models/operatingsystems/redhat.rb

ShimShtein · 2024-02-29T10:20:55Z

Now the tests should pass

ekohl

I'm still not a fan of the meets_requirement method. Getting the API right is just too hard IMHO. What if you also need to specify ranges, like upper bounds? I'd prefer to get rid of it and make the templates slightly more verbose. It's only used twice in your patch, so writing that out is probably easier for long term support.

ekohl · 2024-03-19T16:31:16Z

app/models/operatingsystems/redhat.rb

+    name == 'Fedora'
+  end
+
+  def rhel_compatible?


Where do we place Amazon Linux here? It's within the Red Hat ecosystem, but it's based on Fedora with its own independent release cycle.

Since you're including this in core, we're on the hook unlike if we kept the logic in the templates.

Currently the templates ignore Amazon Linux distinction.
If we want to do such a distinction, we should add a new method amazon? and have it in the meets_requirement method too.

I don't think it should be done in this PR though. If you think it's important, I'll open a new PR that will be based on this one.

ShimShtein · 2024-03-20T11:42:54Z

I'm still not a fan of the meets_requirement method. Getting the API right is just too hard IMHO. What if you also need to specify ranges, like upper bounds? I'd prefer to get rid of it and make the templates slightly more verbose. It's only used twice in your patch, so writing that out is probably easier for long term support.

We have quite a lot of version specifications in the code, just search for os_major in the code. Once we have this infra in place, I can refactor the other templates too.
As for upper bounds, we can add modifiers if that is needed:

os.meets_requirement(:at_most, rhel: 10)

I think it's readable, and will make a lot of sense.

stejskalleos · 2024-03-20T13:19:42Z

Failing unit tests might be related:

NicTest#test_0033_nic with both subnet and subnet6 should be valid if MTU is consistent between subnets [test/models/nic_test.rb:526]

[2024-03-20T11:59:47.998Z] Minitest::Assertion: #<Nic::Managed id: nil, mac: "00:33:45:ab:01:96", ip: "241.84.94.35", type: "Nic::Managed", name: "nick", host_id: 114, subnet_id: 1018350845, domain_id: nil, attrs: {}, created_at: nil, updated_at: nil, provider: nil, username: nil, password: nil, virtual: false, link: true, identifier: "eth946", tag: "", attached_to: "", managed: true, mode: "balance-rr", attached_devices: "", bond_options: "", primary: false, provision: false, compute_attributes: {}, ip6: nil, subnet6_id: 1018350846> errors: Ip does not match selected subnet

ShimShtein · 2024-03-21T08:30:31Z

[test unit]

ShimShtein · 2024-03-21T12:02:54Z

rerunning the tests solved it

ares

just one question from my side

app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb

ShimShtein · 2024-03-27T10:41:06Z

Ready for hopefully final round of review

ekohl · 2024-04-08T10:42:37Z

I think it's readable, and will make a lot of sense.

I don't think it does and really think it deserves its own PR with its own discussion. Introducing it here makes the whole code inconsistent and doesn't give a good idea of whether the API really is a good one. Once again, please modify the PR to not include it.

ShimShtein · 2024-04-10T12:07:49Z

@ekohl switched to explicit version limits.

ShimShtein · 2024-04-11T09:40:46Z

[test unit]

ares · 2024-04-12T14:57:30Z

@ShimShtein does this fix the issue for RHEL 8 and 9? in the recent version I didn't see OS condition. I believe Anaconda behaves differently when it comes to configuration of interfaces in 7,8 and 9. In 8 it still generates ifcfg files from network commands, in 9 it generates nm connection files.

So in 8 we had issue with sometimes the files created by Anaconda conflicting with the ones we generate through our snippet in %post.
In 9 we ended up with nm connection (from Anaconda) and ifcfg files (from our %post).
In 7 Anaconda didn't create the files at all so our %post snippet was the only thing creating this.

Needless to say, we need to have a solution that works for all 7,8,9

ShimShtein · 2024-04-14T08:30:49Z

does this fix the issue for RHEL 8 and 9?

@ares Yes, this is the whole idea of this PR. Instead of always generating ifcfg files in the %post step, we switch to requesting anaconda to define the interfaces for us. Anaconda is the one to choose the proper way to do it - for RHEL 7 it will use the ifcfg files, and for RHEL 8+ it can use the nm records for defining the interfaces. In either way, instead of having the installation procedure (anaconda) creating a default record for us and then overriding the behavior manually, we request a correct configuration from anaconda.
Effectively I am removing the old ifcfg template altogether from the %post step here and instead I am adding the network anaconda command for each interface that is defined in Foreman.

stejskalleos · 2024-04-17T05:32:03Z

QEs confirmed that the changes are OK and work as expected.
@ShimShtein Please rebase, squash commits, and add Fixes #... to the commit message.
@ares @ekohl, I will merge the PR today if there are no other objections.

ShimShtein · 2024-04-18T16:01:40Z

Fixed the new rocky snapshots too. Should be OK

The requested changes have been done or dropped meanwhile

stejskalleos · 2024-04-19T07:00:29Z

Thanks @ShimShtein, @ekohl, and rest for the help!

ares · 2024-04-23T17:11:08Z

Is there a reason why this does not have redmine issue?

ekohl · 2024-04-23T17:35:08Z

The commit has one, just the PR doesn't. The bot doesn't check for that

pr-processor bot added Not yet reviewed Waiting on contributor labels Dec 19, 2023

ShimShtein force-pushed the rhel9_networking branch from 2e60cd8 to f342ccf Compare December 19, 2023 12:59

ekohl requested changes Dec 19, 2023

View reviewed changes

pr-processor bot removed the Not yet reviewed label Dec 19, 2023

ShimShtein force-pushed the rhel9_networking branch from f342ccf to e812fec Compare December 20, 2023 09:36

pr-processor bot added the Needs re-review label Dec 20, 2023

ShimShtein force-pushed the rhel9_networking branch from e812fec to c164f1a Compare December 20, 2023 09:40

ekohl reviewed Dec 24, 2023

View reviewed changes

ShimShtein force-pushed the rhel9_networking branch from c164f1a to e2482ea Compare December 25, 2023 10:08

ekohl requested changes Jan 22, 2024

View reviewed changes

app/models/operatingsystems/redhat.rb Outdated Show resolved Hide resolved

pr-processor bot removed the Needs re-review label Jan 22, 2024

stejskalleos self-assigned this Jan 29, 2024

stejskalleos reviewed Feb 6, 2024

View reviewed changes

app/services/foreman/renderer/scope/macros/kickstart.rb Outdated Show resolved Hide resolved

ShimShtein force-pushed the rhel9_networking branch from e2482ea to 13b7ee3 Compare February 21, 2024 10:13

pr-processor bot added the Needs re-review label Feb 21, 2024

github-actions bot added the Templates label Feb 21, 2024

ekohl reviewed Feb 21, 2024

View reviewed changes

stejskalleos reviewed Feb 27, 2024

View reviewed changes

app/views/unattended/provisioning_templates/provision/kickstart_default.erb Show resolved Hide resolved

ekohl reviewed Feb 27, 2024

View reviewed changes

ShimShtein force-pushed the rhel9_networking branch 3 times, most recently from 159687d to f7cf7ba Compare February 29, 2024 10:19

ekohl mentioned this pull request Mar 5, 2024

Fixes #37098 - setting the hostname from the primary interface #10013

Open

ekohl previously requested changes Mar 19, 2024

View reviewed changes

ShimShtein force-pushed the rhel9_networking branch from 421d052 to b434b71 Compare March 20, 2024 11:27

pr-processor bot added the Needs re-review label Mar 20, 2024

ares reviewed Mar 25, 2024

View reviewed changes

app/views/unattended/provisioning_templates/provision/kickstart_ovirt.erb Outdated Show resolved Hide resolved

ShimShtein force-pushed the rhel9_networking branch from b434b71 to ced9373 Compare March 26, 2024 15:39

ShimShtein force-pushed the rhel9_networking branch from ced9373 to 76af8f6 Compare April 10, 2024 09:46

ShimShtein force-pushed the rhel9_networking branch from 76af8f6 to 9c38bd1 Compare April 18, 2024 12:15

pr-processor bot removed the Waiting on contributor label Apr 18, 2024

Fixes #37367 - Switch to 'network' directive instead of ifcfg

6b2d02c

ShimShtein force-pushed the rhel9_networking branch from 9c38bd1 to 6b2d02c Compare April 18, 2024 16:00

stejskalleos approved these changes Apr 19, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Apr 19, 2024

stejskalleos merged commit ca34010 into theforeman:develop Apr 19, 2024
54 checks passed

ares changed the title ~~Switch to 'network' directive instead of ifcfg~~ Fixes #37367 - Switch to 'network' directive instead of ifcfg Apr 24, 2024

	iface = FactoryBot.build(:nic_base, :primary => true)
	iface = FactoryBot.build(:nic_base, :primary => true, :managed => false)

		if @iface.respond_to?(:mtu)
		network_options.push("--mtu=#{@iface.mtu}") if @iface.mtu

Fixes #37367 - Switch to 'network' directive instead of ifcfg #9961

Fixes #37367 - Switch to 'network' directive instead of ifcfg #9961

Conversation

ShimShtein commented Dec 19, 2023

ekohl left a comment

Choose a reason for hiding this comment

ekohl commented Dec 19, 2023

ShimShtein commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Dec 25, 2023

ShimShtein commented Dec 25, 2023

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Feb 29, 2024

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Mar 20, 2024

stejskalleos commented Mar 20, 2024

ShimShtein commented Mar 21, 2024

ShimShtein commented Mar 21, 2024

ares left a comment

Choose a reason for hiding this comment

ShimShtein commented Mar 27, 2024

ekohl commented Apr 8, 2024

ShimShtein commented Apr 10, 2024

ShimShtein commented Apr 11, 2024

ares commented Apr 12, 2024

ShimShtein commented Apr 14, 2024

stejskalleos commented Apr 17, 2024

ShimShtein commented Apr 18, 2024

stejskalleos commented Apr 19, 2024

ares commented Apr 23, 2024

ekohl commented Apr 23, 2024