assisted-service icon indicating copy to clipboard operation
assisted-service copied to clipboard

[WIP] OCPBUGS-63472: Include --copy-network for all ABI installations

Open rwsu opened this issue 4 months ago • 6 comments

The --copy-network flag needs to be included so that network configuration changes applied through the agent-tui/nmtui persist after the node reboots.

List all the issues related to this PR

  • [ ] New Feature
  • [ ] Enhancement
  • [x] Bug fix
  • [ ] Tests
  • [ ] Documentation
  • [ ] CI/CD

What environments does this code impact?

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

How was this code tested?

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

Checklist

  • [x] Title and description added to both, commit and PR.
  • [x] Relevant issues have been associated (see CONTRIBUTING guide)
  • [x] 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?

rwsu avatar Oct 25 '25 02:10 rwsu

@rwsu: This pull request references Jira Issue OCPBUGS-63472, which is invalid:

  • expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

The --copy-network flag needs to be included so that network configuration changes applied through the agent-tui/nmtui persist after the node reboots.

List all the issues related to this PR

  • [ ] New Feature
  • [ ] Enhancement
  • [x] Bug fix
  • [ ] Tests
  • [ ] Documentation
  • [ ] CI/CD

What environments does this code impact?

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

How was this code tested?

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

Checklist

  • [x] Title and description added to both, commit and PR.
  • [x] Relevant issues have been associated (see CONTRIBUTING guide)
  • [x] 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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot avatar Oct 25 '25 02:10 openshift-ci-robot

/jira refresh

rwsu avatar Oct 25 '25 02:10 rwsu

@rwsu: This pull request references Jira Issue OCPBUGS-63472, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.21.0) matches configured target version for branch (4.21.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact: /cc @mhanss

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot avatar Oct 25 '25 02:10 openshift-ci-robot

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 43.87%. Comparing base (62373a0) to head (a5ab3bb). :warning: Report is 10 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8155      +/-   ##
==========================================
+ Coverage   43.84%   43.87%   +0.03%     
==========================================
  Files         414      414              
  Lines       71843    71893      +50     
==========================================
+ Hits        31498    31546      +48     
+ Misses      37483    37477       -6     
- Partials     2862     2870       +8     
Files with missing lines Coverage Δ
cmd/agentbasedinstaller/register.go 51.78% <100.00%> (+16.28%) :arrow_up:

... and 2 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 25 '25 03:10 codecov[bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot avatar Jan 25 '26 09:01 openshift-bot

Walkthrough

The changes introduce conditional placeholder StaticNetworkConfig generation in InfraEnv creation when NMStateConfig is absent, ensuring ABI workflow support. Additionally, comprehensive test coverage validates behavior across scenarios with present/absent network configurations, cluster references, and error conditions.

Changes

Cohort / File(s) Summary
InfraEnv Registration Logic
cmd/agentbasedinstaller/register.go
Adds staticNetworkConfig local variable and implements conditional placeholder injection. When NMStateConfig processing yields no entries, a minimal YAML placeholder with dummy interface is generated to enable --copy-network flow while preserving real config from NetworkManager keyfiles. StaticNetworkConfig is now unconditionally assigned to InfraenvCreateParams.
InfraEnv Registration Tests
cmd/agentbasedinstaller/register_test.go
Introduces comprehensive test suite for RegisterInfraEnv with BeforeEach/AfterEach scaffolding, test manifests, and temporary directory management. Validates StaticNetworkConfig population with real configs, placeholder generation when NMStateConfig missing, cluster reference propagation, and error handling. Adds mockInfraEnvTransport mock type to capture and verify API parameters.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

[!TIP]

🧪 Unit Test Generation v2 is now available!

We have significantly improved our unit test generation capabilities.

To enable: Add this to your .coderabbit.yaml configuration:

reviews:
  finishing_touches:
    unit_tests:
      enabled: true

Try it out by using the @coderabbitai generate unit tests command on your code files or under ✨ Finishing Touches on the walkthrough!

Have feedback? Share your thoughts on our Discord thread!


Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Jan 25 '26 09:01 coderabbitai[bot]

@rwsu: This pull request references Jira Issue OCPBUGS-63472, which is invalid:

  • expected the bug to target either version "4.22." or "openshift-4.22.", but it targets "4.21.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Enable --copy-network flag when NetworkManager keyfiles are detected in /etc/NetworkManager/system-connections/. This ensures static network configuration persists after installation for both NMStateConfig and agent TUI configured networks.

When keyfiles exist but no NMStateConfig is provided, set a placeholder StaticNetworkConfig with a dummy0 interface (type: dummy, state: down). This passes validation and triggers --copy-network without interfering with the real network configuration from NetworkManager keyfiles.

Also refactored functions to accept fs.FS parameter for testability and fixed path compatibility issues (fs.FS requires relative paths).

List all the issues related to this PR

  • [ ] New Feature
  • [ ] Enhancement
  • [x] Bug fix
  • [ ] Tests
  • [ ] Documentation
  • [ ] CI/CD

What environments does this code impact?

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

How was this code tested?

  • [ ] assisted-test-infra environment
  • [x] dev-scripts environment
  • [ ] Reviewer's test appreciated
  • [x] Waiting for CI to do a full test run
  • [x] Manual, created network config using agent-tui/nmtui and observed it persisted after node is installed
  • [ ] No tests needed

Checklist

  • [x] Title and description added to both, commit and PR.
  • [x] Relevant issues have been associated (see CONTRIBUTING guide)
  • [x] This change does not require a documentation update (docstring, docs, README, etc)
  • [x] 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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot avatar Jan 28 '26 04:01 openshift-ci-robot

/jira refresh

rwsu avatar Jan 28 '26 04:01 rwsu

@rwsu: This pull request references Jira Issue OCPBUGS-63472, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.22.0) matches configured target version for branch (4.22.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact: /cc @bmanzari

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot avatar Jan 28 '26 04:01 openshift-ci-robot

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rwsu

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:

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

openshift-ci[bot] avatar Jan 30 '26 07:01 openshift-ci[bot]

@rwsu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/edge-lint a5ab3bbd2259583e1ea82ebb6250dd479a915942 link true /test edge-lint
ci/prow/edge-unit-test a5ab3bbd2259583e1ea82ebb6250dd479a915942 link true /test edge-unit-test

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci[bot] avatar Jan 30 '26 10:01 openshift-ci[bot]