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

alternatives: update doc to use the usual alternatives commands #673

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

HuijingHei
Copy link
Member

modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
@HuijingHei HuijingHei force-pushed the update-alternatives branch 3 times, most recently from 46da572 to 14a0c25 Compare October 24, 2024 13:02
@HuijingHei HuijingHei requested a review from travier October 25, 2024 10:50
@travier
Copy link
Member

travier commented Oct 25, 2024

This looks good. In coordination with coreos/fedora-coreos-tracker#1818, I think we should refocus this page on the reverse of what we have for instructions right now, which is to say that we should give the instructions to "update" systems to the nft backend instead, as we'll be removing the legacy one.

We can only do that for the manual part for now.

@travier
Copy link
Member

travier commented Oct 25, 2024

Let's see how that works in a systemd unit as well to run this via Ignition.

modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
@HuijingHei HuijingHei force-pushed the update-alternatives branch 2 times, most recently from 8d08485 to 416801a Compare October 25, 2024 14:40
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

your examples here set iptables to use iptables-nft but that happens to already be the default, correct? on my freshly installed 41.20241027.2.0 node I see:

core@localhost:~$ iptables --version
iptables v1.8.10 (nf_tables)
core@localhost:~$ alternatives --display iptables
iptables - status is manual.
 link currently points to /usr/sbin/iptables-nft
/usr/sbin/iptables-legacy - priority 10
 follower ip6tables: /usr/sbin/ip6tables-legacy
 follower ip6tables-restore: /usr/sbin/ip6tables-legacy-restore
 follower ip6tables-save: /usr/sbin/ip6tables-legacy-save
 follower iptables-restore: /usr/sbin/iptables-legacy-restore
 follower iptables-save: /usr/sbin/iptables-legacy-save
/usr/sbin/iptables-nft - priority 10
 follower ip6tables: /usr/sbin/ip6tables-nft
 follower ip6tables-restore: /usr/sbin/ip6tables-nft-restore
 follower ip6tables-save: /usr/sbin/ip6tables-nft-save
 follower iptables-restore: /usr/sbin/iptables-nft-restore
 follower iptables-save: /usr/sbin/iptables-nft-save
Current `best' version is /usr/sbin/iptables-legacy.

also.. what does this output mean? I see link currently points to /usr/sbin/iptables-nft, but also Current 'best' version is /usr/sbin/iptables-legacy., which is a bit confusing.

@HuijingHei
Copy link
Member Author

your examples here set iptables to use iptables-nft but that happens to already be the default, correct?

I see link currently points to /usr/sbin/iptables-nft, but also Current 'best' version is /usr/sbin/iptables-legacy., which is a bit confusing.

I think it is because we add postprocess to make it manually default to iptables-nft. If we remove the postprocess part, it will be default to legacy in auto mode (in my testing).

[core@cosa-devsh ~]$ iptables --version
iptables v1.8.10 (legacy)

[core@cosa-devsh ~]$ alternatives --display iptables
iptables - status is auto.
 link currently points to /usr/sbin/iptables-legacy
...

Both iptables-legacy and iptables-nft have the same priority 10 (see coreos/fedora-coreos-tracker#1818), guess it chooses the current best alternatives by alphabetical order.

@dustymabe
Copy link
Member

I think it is because we add postprocess to make it manually default to iptables-nft. If we remove the postprocess part, it will be default to legacy in auto mode (in my testing).

right. I'm just pointing out that our example should probably be changing it from what we already ship as default? Maybe there is another better example?

@travier
Copy link
Member

travier commented Oct 29, 2024

The idea was to give as example the migration that we would want to have users do in coreos/fedora-coreos-tracker#1818 (moving legacy systems to the nft backend).

But I agree that it does not work for the Butane config as we already do that. I don't think we should show the reverse however as we don't want users to go to the legacy backend as we ideally should remove it.

Not sure if we actually have another "alternatives" command to use for the example.

@dustymabe
Copy link
Member

agree. Thanks for the clarification

@HuijingHei
Copy link
Member Author

Not sure if we actually have another "alternatives" command to use for the example.

There is one cifs-idmap-plugin in alternatives --list, mabye we can use it? See doc for reference.

[core@cosa-devsh ~]$ sudo alternatives --display cifs-idmap-plugin
cifs-idmap-plugin - status is auto.
 link currently points to /usr/lib64/cifs-utils/cifs_idmap_sss.so
/usr/lib64/cifs-utils/cifs_idmap_sss.so - priority 20
/usr/lib64/cifs-utils/idmapwb.so - priority 10
Current `best' version is /usr/lib64/cifs-utils/cifs_idmap_sss.so.

@dustymabe
Copy link
Member

dustymabe commented Oct 30, 2024

There is one cifs-idmap-plugin in alternatives --list, mabye we can use it? See doc for reference.

right, but there is only one option installed, so still probably not a good example?

I say we just have the butane example show setting it to iptables-legacy and note that we don't recommend doing that, it's just an example. Then we can have the interactive example set iptables-nft (for those systems that are somehow still on legacy).

@HuijingHei
Copy link
Member Author

I say we just have the butane example show setting it to iptables-legacy and note that we don't recommend doing that, it's just an example. Then we can have the interactive example set iptables-nft (for those systems that are somehow still on legacy).

Agree this is just an example to run alternatives using butane or manually command, then is this OK to merge?

@dustymabe
Copy link
Member

Agree this is just an example to run alternatives using butane or manually command, then is this OK to merge?

I would update the PR to set iptables legacy in the butane example. Other than that - LGTM.

@dustymabe
Copy link
Member

and, of course @travier may have a different opinion.

@HuijingHei
Copy link
Member Author

How about adding a note to remind that if want to set iptables to legacy, just replace the target to /usr/sbin/iptables-legacy, instead of update the example?

@dustymabe
Copy link
Member

I guess. It's just confusing to have an example that sets a value to what is already the default. i.e. if I was testing this documentation I'd have no way to verify it worked

  • boot without butane from this example
    • value == iptables-nft
  • boot with butane from this example
    • value == iptables-nft

I'd think it was a problem with the docs personally.

@HuijingHei HuijingHei force-pushed the update-alternatives branch 2 times, most recently from d0006a2 to ac0627a Compare October 31, 2024 13:30
@HuijingHei
Copy link
Member Author

Update doc: the butane example to configure the default iptables to legacy, and change to nft using alternative commands, not sure my understanding is correct, thanks @dustymabe @travier for the kind review.

modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/alternatives.adoc Outdated Show resolved Hide resolved
@dustymabe
Copy link
Member

looks pretty good to me - a few final comments.

@dustymabe dustymabe merged commit 78e8bbc into coreos:main Nov 1, 2024
1 check passed
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.

3 participants