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

cli service: Allow user to opt-in on not deleting applied file #2546

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

cathay4t
Copy link
Member

@cathay4t cathay4t commented Feb 7, 2024

Previously, we introduced new method on finding un-applied state YAML
files. It was a changed behaviour which break API.

This patch change nmstatectl service to original behaviour which just
rename applied YAML file with .applied postfix.

If you don't want the applied state been deleted, please place
/etc/nmstate/nmstate.conf (folder can be defined by --config
argument) with content of:

[service]
delete_applied = false

Manpage updated.

Integration test case included.

@cathay4t cathay4t marked this pull request as draft February 7, 2024 16:34
@cathay4t cathay4t changed the title cli service: Allow user to opt-in the checksum based apply WIP: cli service: Allow user to opt-in the checksum based apply Feb 7, 2024
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/nmstate-nmstate-2546
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

doc/nmstate.service.8.in Outdated Show resolved Hide resolved
@drawks
Copy link

drawks commented Feb 7, 2024

I don't know enough rust to evaluate the code in this diff, but I do want to clarify, historic behavior was to apply all network state files ending with .yml and not to make any attempt at skipping application of files which may have been previously applied. Each of these files declares a state and nothing prevents a later file from mutating a state declared in an earlier file; ergo ALL files must be applied in order to maintain the same behavior as before.

I'm being very specific here because configuring network interfaces is a very foundational bit of configuration and changing this behavior may leave some hosts with broken network configs which can be a SEV 1 type outage.

Consider a case where an operator has configuration management dropping 2 files in /etc/nmstate, 000-reset.yml and 100-configure-defaults.yml where the first file deletes various interface and route configurations to establish a known initial state and then in the second file applies the intended end state. It would break networking if the contents of 000-reset.yml were changed and were resultantly re-applied while the contents of 100-configure-defaults.yml were unchanged and then not re-applied. You will have effectively stopped nmstate service from declarative applying the intended configuration AND will have left the host in a configuration which has no working networking.

The above example is only an example, but it is meant to illustrate the importance of EXACTLY preserving the default behavior of this tool until/unless a behavioral change is released as part of a major version change and/or is well documented and disclosed in a series of "coming breaking change/deprecation" notices.

@cathay4t cathay4t changed the title WIP: cli service: Allow user to opt-in the checksum based apply cli: Allow user to opt-in on not deleting applied file Feb 8, 2024
@cathay4t cathay4t changed the title cli: Allow user to opt-in on not deleting applied file cli service: Allow user to opt-in on not deleting applied file Feb 8, 2024
@cathay4t cathay4t force-pushed the fix_service branch 2 times, most recently from d93d386 to ec7e2bd Compare February 8, 2024 03:30
@cathay4t
Copy link
Member Author

cathay4t commented Feb 8, 2024

@drawks Thanks for the detailed use case.

I have included test case test_nmstate_service_remove_applied_file_by_default to make sure nmstate.service remove the applied file.

By default, nmstate.service apply all .yml files in /etc/nmstate folder.

Any user want to keep applied by as it was, they can opt-in by create /etc/nmstate/nmstate.conf with content:

[service]
delete_applied = false

In general, this PR is restoring the old behavior and we have test to ensure that.

@cathay4t cathay4t marked this pull request as ready for review February 8, 2024 03:36
Copy link
Member

@qinqon qinqon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit on the config field name.

doc/nmstate.service.8.in Outdated Show resolved Hide resolved
Previously, we introduced new method on finding un-applied state YAML
files. It was a changed behaviour which break API.

This patch change `nmstatectl service` to original behaviour which just
rename applied YAML file with `.applied` postfix.

If you don't want the applied state been deleted, please place
`/etc/nmstate/nmstate.conf` (folder can be defined by `--config`
argument) with content of:

```toml
[service]
keep_state_file_after_apply = true
```

Manpage updated.

Integration test case included.

Signed-off-by: Gris Ge <fge@redhat.com>
@cathay4t cathay4t enabled auto-merge (rebase) February 8, 2024 06:49
@cathay4t cathay4t merged commit 733deba into nmstate:base Feb 8, 2024
25 of 26 checks passed
@cathay4t cathay4t deleted the fix_service branch February 8, 2024 07:03
qinqon added a commit to qinqon/os that referenced this pull request Feb 8, 2024
When [1] land to RHCOS the nmstate service will go back to previous
behaviour, the /etc/nmstate/*.yml files are moved to .applied
files, so we need to configure nmstate to keep the those files, if we
don't do so MCO complains at in place upgrades since the file system has
change.

[1] nmstate/nmstate#2546

Signed-off-by: Enrique Llorente <ellorent@redhat.com>
qinqon added a commit to qinqon/machine-config-operator that referenced this pull request Feb 15, 2024
When [1] land to RHCOS the nmstate service will go back to previous
behaviour, the /etc/nmstate/*.yml files are moved to .applied
files, so we need to configure nmstate to keep the those files, if we
don't do so MCO complains at in place upgrades since the file system has
change.

[1] nmstate/nmstate#2546

Signed-off-by: Enrique Llorente <ellorent@redhat.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/machine-config-operator that referenced this pull request Feb 16, 2024
When [1] land to RHCOS the nmstate service will go back to previous
behaviour, the /etc/nmstate/*.yml files are moved to .applied
files, so we need to configure nmstate to keep the those files, if we
don't do so MCO complains at in place upgrades since the file system has
change.

[1] nmstate/nmstate#2546

Signed-off-by: Enrique Llorente <ellorent@redhat.com>
dkhater-redhat pushed a commit to dkhater-redhat/machine-config-operator that referenced this pull request Mar 8, 2024
When [1] land to RHCOS the nmstate service will go back to previous
behaviour, the /etc/nmstate/*.yml files are moved to .applied
files, so we need to configure nmstate to keep the those files, if we
don't do so MCO complains at in place upgrades since the file system has
change.

[1] nmstate/nmstate#2546

Signed-off-by: Enrique Llorente <ellorent@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants