MGMT-12054: Support discovery kernel arguments for full-iso and minimal-iso

[ori-amizur]

When streaming the discovery ISO file, the kernel arguments are included as part of some files: grub.cfg and isolinux.cfg

Each of these files contains an area where the kernel arguments can be embedded. This are can be identified by the regex "(\n#+) COREOS_KARG_EMBED_AREA". The arguments should overwrite this are from the beginning and should not pass the last # (hash sign). To do that, an overlay was created.

In new Openshift versions, there is a file that contains the file names, while in old versions the file does not exist. If the file exists, the files to embed the kernel arguments are taken from this config file. Otherwise, the default files [/EFI/redhat/grub.cfg, /isolinux/isolinux.cfg] are used.

Description

How was this code tested?

Assignees

/cc @carbonin
/cc @filanov

Links

Checklist

• Title and description added to both, commit and PR
• Relevant issues have been associated
• Reviewers have been listed
• 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)

[codecov]

Codecov Report

Merging #99 (d7bb8b0) into main (d7fecdd) will increase coverage by 0.40%.
The diff coverage is 67.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
+ Coverage   68.29%   68.70%   +0.40%     
==========================================
  Files          17       18       +1     
  Lines         858      965     +107     
==========================================
+ Hits          586      663      +77     
- Misses        172      192      +20     
- Partials      100      110      +10     

Impacted Files	Coverage Δ
pkg/isoeditor/kargs.go	61.11% <61.11%> (ø)
internal/handlers/assisted_service_client.go	63.20% <62.96%> (-0.09%)	⬇️
pkg/isoeditor/stream.go	53.84% <73.33%> (+2.12%)	⬆️
pkg/isoeditor/isoutil.go	63.63% <80.00%> (+5.57%)	⬆️
internal/handlers/iso.go	88.23% <100.00%> (+1.13%)	⬆️

[filanov]

/assign @avishayt

[cgwalters]

Ahhh wait, is there a reason this code isn't using coreos-installer? I am not totally sure our existing kargs area is stable public API.

[carbonin]

@cgwalters I think there are still a few gaps.

The last feature gap IIRC was that the minimal ISO generated by coreos-installer doesn't let us set up our static networking (posisibly for multiple interfaces) and proxy information before pulling the rootfs.

I recall talking to @jlebon about this a while back

[carbonin]

But it may very well be time to meet up again to discuss if everything is covered now.
I definitely wouldn't mind moving to coreos-installer if it covered all our use cases.

[carbonin]

hmmm also even if coreos-installer supports streaming the output iso to stdout we'd still be missing range request support I think.

I think it would be good to move this discussion out of this PR though. I'll open an issue for us to consider coreos-installer.

[jlebon]

Yeah, we discussed this in the past. The use of COREOS_KARG_EMBED_AREA here is not ideal, and we're hoping that coreos-installer can satisfy AI use cases in the future. We have support for network configuration now (see https://coreos.github.io/coreos-installer/cmd/iso/#coreos-installer-iso-network-embed), but the proxy bit is still missing (tracked at coreos/coreos-installer#794). Would be good to discuss the range request bit in more details somewhere.

[carbonin]

Opened https://issues.redhat.com/browse/MGMT-12340 to track this.
I tried to add a much as I could think of to that. Let's move the discussion there

[carbonin]

In new Openshift versions, there is a file that contains the file names, while in old versions the file does not exist. If the file exists, the files to embed the kernel arguments are taken from this config file.

As a separate enhancement we should probably expand our use of this file to use it to determine where to add the rootfs parameter when creating the minimal iso.

This would probably solve some problems (or at least minimize the amount we would need to fix) for the IBM folks doing power and Z integration.
cc @mtarsel @jacobemery

[carbonin]

Just a few test comments. Looks good otherwise though.

[carbonin]

I'm going to reply to your old comment here just so it's not outdated, but TLDR is this should be Describe("kargsFiles" because it's grouping tests of that function. It's good to have the function name right in the test.

---

In reply to #99 (comment):

BTW, go does not allow reference from non-test code to test code.

This is interesting. I would expect this if the tests were in a different package, but it's surprising to me that it seems to omit *_test.go files when compiling main.
That said, if you reference things defined in test files from real ones and run the tests, it works.
To me it's unintuitive and feels like "magic" so I'd like to avoid defining stuff at the top level in tests anyway.

I think Describe is unexpected within Context although it is allowed.

I don't think so at all. Describe and Context are functionally identical. They only differ semantically. Generally I use Describe to group tests of a particular function and Context when I need shared setup for tests of multiple functions.
If you need shared setup only for the tests of a single function, I'd put it in that function's Describe block.

https://github.com/openshift/assisted-image-service/blob/d7fecdd94b334150af7daf53b9fca04722d32582/pkg/isoeditor/isoutil_test.go is a good example where multiple Describes need the same context.

The text with karg helpers looks unclear to me.

The exact text isn't super important in this case since every test is in the same context. It might as well stay "kargs tests"

TLDR is Describe is for grouping It blocks for the same function and Context is for denoting shared setup/teardown.

[ori-amizur]

That said, if you reference things defined in test files from real ones and run the tests, it works.

No it doesn't

I usually use Describe as the most external function. That's the most common use in the assisted service. There are many few external Context functions.

If you think it is very significant I will change.

[carbonin]

No it doesn't

I wouldn't have asserted something like this if I didn't test it 😉
I have a module written that reproduces this behavior, but that said I did also find golang/go#10184 (comment) which explains how it could never cause a real problem.

If you think it is very significant I will change.

Not very significant, but I wrote this all up mostly to just explain how I think about writing tests. If my descriptions and examples haven't impacted how you want to write these up that's okay. As I said Context and Describe are functionally equivalent.

The only thing I would very much like to see is the container (Context or Describe) that is grouping tests for a particular function be named after that function. That way it is very clear what is being tested when a test fails.

[ori-amizur]

I wouldn't have asserted something like this if I didn't test it
I have a module written that reproduces this behavior, but that said I did also find golang/go#10184 (comment) which explains how it could never cause a real problem.

Well that's strange because I tested it too.
In the same package, I tried to reference in non-test file a const that is defined in test file. The symbol was invisible.
Specifically tried on the const you made the initial comment on.

[ori-amizur]

/hold

[ori-amizur]

/unhold

[openshift-ci]

@ori-amizur: 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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, ori-amizur

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 [carbonin,ori-amizur]

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