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

Info file parameter support #1896

Merged
merged 34 commits into from
Feb 19, 2025
Merged

Info file parameter support #1896

merged 34 commits into from
Feb 19, 2025

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Jan 14, 2025

Problem

The linuxrc provides quite useful option to specify remote "info" file that contain additional parameters. It helps when kernel command line is restricted by amount of characters or if writing to it is annoying and it is better to point to that info file.

Solution

Implement support for agama.info. Also implement filtering of agama specific kernel options and write it to /etc/agama/kernel.cmdline.conf so yast2-bootloader can use it for proposal.

Note: agama.info does not support all linuxrc info features like it quire correct name agama.info= without any additional dots, dashes or different letter case. It also currently support only single info parameter and it cannot be recursive, so additional info parameters are not expanded. And last but not least it supports only url that is supported by curl.

Testing

  • tested manually with own iso and passing live.password= param.

TODO:

@jreidinger jreidinger changed the title create also file with kernel only files for bootloader proposal Info file parameter support Jan 20, 2025
@coveralls
Copy link

coveralls commented Jan 20, 2025

Pull Request Test Coverage Report for Build 13390704125

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 3 files are covered.
  • 50 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.01%) to 70.787%

Files with Coverage Reduction New Missed Lines %
service/lib/agama/cmdline_args.rb 1 96.67%
service/service/lib/agama/cmdline_args.rb 1 96.67%
service/lib/agama/registration.rb 2 97.5%
service/lib/agama/storage/finisher.rb 2 98.55%
service/service/lib/agama/registration.rb 2 97.5%
service/service/lib/agama/storage/finisher.rb 2 98.55%
service/lib/agama/dbus/software/product.rb 5 86.27%
service/service/lib/agama/dbus/software/product.rb 5 86.27%
service/lib/agama/manager.rb 15 81.89%
service/service/lib/agama/manager.rb 15 81.89%
Totals Coverage Status
Change from base Build 13369642019: 0.01%
Covered Lines: 17723
Relevant Lines: 25037

💛 - Coveralls

INFO_CONTENT="${2:-/etc/agama.d/cmdline.info.conf}"

expand_info_arg() {
INFO_URL=$(sed -n 's/\(.*[[:space:]]\|^\)agama\.info=\([^[:space:]]\+\).*/\2/p' "$TARGET")
Copy link
Contributor

Choose a reason for hiding this comment

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

This picks up only one info file. Linuxrc supports multiple info arguments. Then it downloads and merges all info files.

This might be pretty useful. You can have a generic info file and an optional debugging one which additionally enables some debug features. This avoids duplicating the common parts between the info files. Just use the common one and if needed easily add the debugging one.

Copy link
Contributor

Choose a reason for hiding this comment

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

And linuxrc supports nested info files, you can use info= in info file as well... 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know about both of those linuxrc features and it is possible to implement it, I just do not see much usage for it. What I see so far from bug reports is that there is just single info file param that contain required parameters. In the end there is not so much parameters you need to use and usually it is more like debug.info and if needed some production.info file.
I plan to document this limitation to old linuxrc behavior and if there is interest in extending it, we can always do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also allow different locations not sure if the same supported by curl... so something to document too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, also true

Co-authored-by: Knut Alejandro Anderssen González <kanderssen@suse.de>
@jreidinger jreidinger marked this pull request as ready for review January 21, 2025 10:42
## Problem

As there is no linuxrc in Agama we want to explore the possibility of
translating linuxrc network-related boot arguments into the dracut
equivalent (which is the supported and recommended method for the new
installation media).

- https://jira.suse.com/browse/AGM-38
- https://trello.com/c/iCICenNT

## Solution

A basic translation of the **ifcfg** boot argument has being implemented
as a prof of concept but as the nm-initrd-generator parses de cmdline as
a cmdline hook it is too early for doing some kind of checks about the
devices present in the system and trying to match against the device
name and the mac address therefore any kind of pattern except the
**'*'** wildcard.

**Supported examples**

- ifcfg=*=dhcp
  ip=dhcp

- ifcfg=eth0=dhcp
  ip=eth0:dhcp

- ifcfg=eth0.10=192.168.0.100/24,192.168.0.1
  vlan=eth0.10:eth0 ip=192.168.0.100::192.168.0.1:24::eth0.10

- ifcfg="eth0=192.168.0.33/24 10.0.0.100/24,192.168.0.1,192.168.0.1
10.0.0.1,suse.de"
ip=192.168.0.33::192.168.0.1:24::eth0 nameserver=192.168.0.1
nameserver=10.0.0.1 ip=10.0.0.100:::24::eth0

The parser is also added as a cmdline hook, we explored the possibility
to call it as a **initqueue/settle** hook, in that case we could try to
call the **nm_generate_connections** function directly passing it the
**getcmdline** result. At that point we can check the devices present in
**/sys/class/net/** doing some filtering based in the name or mac
address but for example the try function requires to run the
configuration in order to check if the interface was configured or not
for continuing trying... for that kind of support we might need to add
it to NetworkManager and the nm-initrd-generator.

## Testing

- *Tested manually*
jreidinger and others added 2 commits February 18, 2025 12:41
Co-authored-by: Knut Alejandro Anderssen González <kanderssen@suse.de>
jreidinger and others added 3 commits February 18, 2025 13:36
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks good. Just some minor comments.

Description=Agama kernel cmdline processing

# have to be after network to be able to download info files
# TODO: what to do in air gap scenario where we still need process cmdline?
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is still and issue, we should record it somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, we need to test it. It is not just here, but also all password services is after network up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done as #2023

return 0
fi

# TODO: should we use also --location-trusted if info file url contain user and password?
Copy link
Contributor

Choose a reason for hiding this comment

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

Another item for a follow-up card.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done as #2024

Copy link
Contributor

@teclator teclator left a comment

Choose a reason for hiding this comment

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

LGTM

@jreidinger jreidinger merged commit 395beef into master Feb 19, 2025
10 checks passed
@jreidinger jreidinger deleted the info_param branch February 19, 2025 09:42
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.

5 participants