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

Support ignition in simplified-installer and raw-image #3130

Merged
merged 7 commits into from
Jan 17, 2023

Conversation

runcom
Copy link
Member

@runcom runcom commented Nov 11, 2022

This patchset is the basic work needed to support ignition in r4e - it doesn't include any blueprint or file injection change as I wanted to maintain it easy to review and since this won't break anything, I think it'll be beneficial to merge this before other work to plumb ignition files/config in the blueprints. This PR also includes a commit that enables kargs injection from the installer and raw-image blueprint to effectively enable passing the artifact an ignition.config.url and test it out. Lastly, to make the work bound to the sysroot.ro=true patch, this also includes #3053

Things missing:

@TomasTomecek
Copy link

/packit build

@TomasTomecek
Copy link

sorry about that ^ Friday morning infrastructure problems -_-

@runcom runcom force-pushed the ignition-poc branch 4 times, most recently from fbf95a3 to f74bfbb Compare November 25, 2022 13:11
@runcom runcom added the WIP+test Work in progress but run Gitlab CI. label Nov 25, 2022
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD f74bfbb with the main merge-base 920431a). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3380089400/artifacts/browse

@runcom runcom marked this pull request as ready for review November 25, 2022 16:36
@runcom runcom marked this pull request as draft November 25, 2022 16:38
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD e582295 with the main merge-base 920431a). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3387299640/artifacts/browse

Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 1f6477f with the main merge-base 920431a). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3387797735/artifacts/browse

Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD ad4973d with the main merge-base 920431a). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3388768145/artifacts/browse

@runcom runcom force-pushed the ignition-poc branch 4 times, most recently from ede5e0a to 66b12b1 Compare November 29, 2022 11:17
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 66b12b1 with the main merge-base c6570f6). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3393843007/artifacts/browse

@runcom runcom force-pushed the ignition-poc branch 2 times, most recently from f1919fb to 16d58ab Compare November 29, 2022 14:26
@7flying
Copy link
Member

7flying commented Nov 30, 2022

blueprint customization options on #3161

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Thanks, this all looks good. Some minor comments and one potentially important one below.

I'd also like a more descriptive commit message on the last commit ("wire ignition").

Also, in the kernel options in the manifests I'm seeing ...modprobe.blacklist=vc4,rw,,coreos.no_persist_ip,... (an empty option between rw and coreos.no_persist_ip) but I can't pinpoint where it's coming from.

internal/distro/rhel9/edge.go Show resolved Hide resolved
internal/distro/rhel9/images.go Outdated Show resolved Hide resolved
internal/distro/rhel9/images.go Outdated Show resolved Hide resolved
internal/distro/rhel9/images.go Outdated Show resolved Hide resolved
internal/distro/rhel9/images.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 49f5e5a with the main merge-base 382db42). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3585510205/artifacts/browse

@runcom
Copy link
Member Author

runcom commented Jan 11, 2023

I'd also like a more descriptive commit message on the last commit ("wire ignition").

that's one last WIP I have - I'll polish everything up for a final review anyway

Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD ec9c653 with the main merge-base 382db42). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3586253229/artifacts/browse

Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 90b39de with the main merge-base 382db42). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3587739547/artifacts/browse

@runcom
Copy link
Member Author

runcom commented Jan 11, 2023

tests are green - I'm gonna polish this all tomorrow, pending a tiny decision about ignition.embedded.url and adding a test for raw image (just ignition.firstboot)

@runcom
Copy link
Member Author

runcom commented Jan 12, 2023

Also, in the kernel options in the manifests I'm seeing ...modprobe.blacklist=vc4,rw,,coreos.no_persist_ip,... (an empty option between rw and coreos.no_persist_ip) but I can't pinpoint where it's coming from.

@achilleas-k I found it, comes from customization.GetKernel() and how we append - basically checking for nil there isn't sufficient cause it's never nil - I need to check that Append isn't empty before appending

Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 1c046d0 with the main merge-base 382db42). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3592637535/artifacts/browse

@runcom
Copy link
Member Author

runcom commented Jan 12, 2023

should be good for another review now 🤔 tests are green

runcom and others added 6 commits January 13, 2023 10:04
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
Signed-off-by: Antonio Murdaca <runcom@linux.com>
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
Signed-off-by: Irene Diez <idiez@redhat.com>
Co-authored-by: Irene Diez <idiez@redhat.com>
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
@runcom runcom force-pushed the ignition-poc branch 2 times, most recently from 1f00476 to e16b825 Compare January 13, 2023 09:08
Signed-off-by: Antonio Murdaca <antoniomurdaca@gmail.com>
Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD 8305236 with the main merge-base 25faf5a). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/3599176381/artifacts/browse

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP+test Work in progress but run Gitlab CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants