MGMT-15339: Run network config before NetworkManager

[jhernand]

Currently the systemd service that we use to apply static network configuration from the non-minimal discovery environment wants to run after dracut-cmdline.service and before dracut-initqueue.service. But that isn't possible because this service is created by an ignition configuration, and services created that way only run after the root has pivoted, and at that point the cmdline and initqueue steps of Dracut have already been completed.

The net result is that this ordering dependencies are just ignored, and the service will run as part of multi-user.target without any special ordering restriction. It may even run after NetworkManager. If that happens then the configuration files that it generates will be ignored. That is unlikely, but it has been observed when there are many network interfaces with static configuration. To avoid that the service should be explicitly configured to run before NetworkManager.

Note that there is another diferent systemd service that is used to generate the network config when using the minimal ISO. That is added directly to the initrd, not via ignition. It runs before the root pivots because the network is needed to fetch the root file system. This patch doesn't change that service.

Related: https://issues.redhat.com/browse/MGMT-15339
Related: https://issues.redhat.com//browse/OCPBUGS-16219
Related: openshift/installer#7355

List all the issues related to this PR

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see CONTRIBUTING guide)
• This change does not require a documentation update (docstring, docs, README, etc)
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

[openshift-ci-robot]

@jhernand: This pull request explicitly references no jira issue.

In response to this:

Currently the systemd service that we use to apply static network configuration from the non-minimal discovery environment wants to run after dracut-cmdline.service and before dracut-initqueue.service. But that isn't possible because this service is created by an ignition configuration, and services created that way only run after the root has pivoted, and at that point the cmdline and initqueue steps of Dracut have already been completed.

The net result is that this ordering dependencies are just ignored, and the service will run as part of multi-user.target without any special ordering restriction. It may even run before NetworkManager. If that happens then the configuration files that it generates will be ignored. That is unlikely, but it have been observed when there are many network interfaces with static configuration. To avoid that the service should be explicitly configured to run before NetworkManager.

Note that there is another diferent systemd service that is used to generate the network config when using the minimal ISO. That is added directly to the initrd, not via ignition. It runs before the root pivots because the network is needed to fetch the root file system. This patch doesn't change that service.

Related: https://issues.redhat.com//browse/OCPBUGS-16219
Related: openshift/installer#7355

List all the issues related to this PR

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see CONTRIBUTING guide)
• This change does not require a documentation update (docstring, docs, README, etc)
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [jhernand]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[zaneb]

typo:

It may even run before NetworkManager. If that happens then the configuration files that it generates will be ignored.

s/before/after/

Also, I think this is worth opening a bug for, because this has been wrong for a while and might need to be backported.

/lgtm

[codecov]

Codecov Report

Merging #5375 (ea545d1) into master (3756e7f) will increase coverage by 0.35%.
Report is 13 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5375      +/-   ##
==========================================
+ Coverage   67.59%   67.95%   +0.35%     
==========================================
  Files         226      226              
  Lines       33304    34020     +716     
==========================================
+ Hits        22513    23119     +606     
- Misses       8758     8852      +94     
- Partials     2033     2049      +16     

see 13 files with indirect coverage changes

[jhernand]

/hold

[openshift-ci-robot]

@jhernand: This pull request references MGMT-15339 which is a valid jira issue.

In response to this:

Currently the systemd service that we use to apply static network configuration from the non-minimal discovery environment wants to run after dracut-cmdline.service and before dracut-initqueue.service. But that isn't possible because this service is created by an ignition configuration, and services created that way only run after the root has pivoted, and at that point the cmdline and initqueue steps of Dracut have already been completed.

The net result is that this ordering dependencies are just ignored, and the service will run as part of multi-user.target without any special ordering restriction. It may even run before NetworkManager. If that happens then the configuration files that it generates will be ignored. That is unlikely, but it have been observed when there are many network interfaces with static configuration. To avoid that the service should be explicitly configured to run before NetworkManager.

Note that there is another diferent systemd service that is used to generate the network config when using the minimal ISO. That is added directly to the initrd, not via ignition. It runs before the root pivots because the network is needed to fetch the root file system. This patch doesn't change that service.

Related: https://issues.redhat.com//browse/OCPBUGS-16219
Related: openshift/installer#7355

List all the issues related to this PR

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see CONTRIBUTING guide)
• This change does not require a documentation update (docstring, docs, README, etc)
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@jhernand: This pull request references MGMT-15339 which is a valid jira issue.

In response to this:

Currently the systemd service that we use to apply static network configuration from the non-minimal discovery environment wants to run after dracut-cmdline.service and before dracut-initqueue.service. But that isn't possible because this service is created by an ignition configuration, and services created that way only run after the root has pivoted, and at that point the cmdline and initqueue steps of Dracut have already been completed.

The net result is that this ordering dependencies are just ignored, and the service will run as part of multi-user.target without any special ordering restriction. It may even run after NetworkManager. If that happens then the configuration files that it generates will be ignored. That is unlikely, but it has been observed when there are many network interfaces with static configuration. To avoid that the service should be explicitly configured to run before NetworkManager.

Note that there is another diferent systemd service that is used to generate the network config when using the minimal ISO. That is added directly to the initrd, not via ignition. It runs before the root pivots because the network is needed to fetch the root file system. This patch doesn't change that service.

Related: https://issues.redhat.com//browse/OCPBUGS-16219
Related: openshift/installer#7355

List all the issues related to this PR

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see CONTRIBUTING guide)
• This change does not require a documentation update (docstring, docs, README, etc)
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jhernand]

typo:

It may even run before NetworkManager. If that happens then the configuration files that it generates will be ignored.

s/before/after/

Done.

Also, I think this is worth opening a bug for, because this has been wrong for a while and might need to be backported.

Here is the bug: https://issues.redhat.com/browse/MGMT-15339

[openshift-ci-robot]

@jhernand: This pull request references MGMT-15339 which is a valid jira issue.

In response to this:

Currently the systemd service that we use to apply static network configuration from the non-minimal discovery environment wants to run after dracut-cmdline.service and before dracut-initqueue.service. But that isn't possible because this service is created by an ignition configuration, and services created that way only run after the root has pivoted, and at that point the cmdline and initqueue steps of Dracut have already been completed.

The net result is that this ordering dependencies are just ignored, and the service will run as part of multi-user.target without any special ordering restriction. It may even run after NetworkManager. If that happens then the configuration files that it generates will be ignored. That is unlikely, but it has been observed when there are many network interfaces with static configuration. To avoid that the service should be explicitly configured to run before NetworkManager.

Note that there is another diferent systemd service that is used to generate the network config when using the minimal ISO. That is added directly to the initrd, not via ignition. It runs before the root pivots because the network is needed to fetch the root file system. This patch doesn't change that service.

Related: https://issues.redhat.com/browse/MGMT-15339
Related: https://issues.redhat.com//browse/OCPBUGS-16219
Related: openshift/installer#7355

List all the issues related to this PR

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see CONTRIBUTING guide)
• This change does not require a documentation update (docstring, docs, README, etc)
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[bfournie]

/lgtm

[romfreiman]

@tsorya @eranco74 ptal

[eranco74]

/lgtm

[eranco74]

/lgtm

[zaneb]

/retest-required

[bfournie]

One minor clarification - the problem that this fixes isn't really that this service runs after NetworkManager, its that that the script doesn't complete writing out the files in /etc/NetworkManager/system-connections before NetworkManager sets up interfaces using these files. That's what makes the bug susceptible to the time that pre-network-manager-config.sh takes to run, which is longer when there are more interfaces. This fixes that timing issue.

[jhernand]

To verify this I introduced an artificial delay in the script that generates the NetworkManager configuration, something like this:

diff --git a/internal/constants/scripts.go b/internal/constants/scripts.go
index c118f254c..89be73284 100644
--- a/internal/constants/scripts.go
+++ b/internal/constants/scripts.go
@@ -30,6 +30,8 @@ package constants
 //    '/etc/coreos-firstboot-network' when the script is part of the minimal ISO.
 const PreNetworkConfigScript = `#!/bin/bash
 
+sleep 60
+
 PATH_PREFIX=${PATH_PREFIX:=''}
 
 # The directory that contains nmconnection files of all nodes

Then I tried to install a cluster without this patch and using static network configuration. I observed that the static network configuration was ignored and instead the network interfaces got the IP addresses from the DHCP server.

I repeated the same test with this patch and then the network interfaces got the IP addresses specified in the configuration.

[jhernand]

/unhold

[openshift-ci]

@jhernand: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.