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

s390x: Add builder config for RHCOS #679

Merged
merged 2 commits into from
Mar 1, 2023
Merged

Conversation

jschintag
Copy link
Contributor

Add the builder config for the s390x RHCOS builder. Add ignition files for both so that they can be fetched during installation.

Signed-off-by: Jan Schintag [email protected]

@jschintag
Copy link
Contributor Author

I am unsure wether or not we want the ignition files in here. They are needed for the LPAR installation, since i need to fetch them over http(s) and this is the easiest way for me. However i guess technically they count as artifacts and not source code.

@dustymabe
Copy link
Member

Thanks for working on this @jschintag - I'll look at it on Monday when I'm back.

@jschintag
Copy link
Contributor Author

No problem @dustymabe, i generally don't expect reviews on Fridays :)

@jschintag jschintag force-pushed the s390x-lpar branch 2 times, most recently from 035875a to 69ff4e7 Compare January 24, 2023 13:15
@jschintag
Copy link
Contributor Author

Since i noticed this one was still open, i added the changes to initialize the secure execution volume during installation here.

@dustymabe
Copy link
Member

@jschintag I think for now let's leave the generated *.ign files out of the git repo and just update the README with further instructions.

I understand why it's useful to have them there but I think it causes more problems than it solves. If we did end up wanting to store them in git I think we should probably front that with a github action or some CI to validate that the *ign files are consistent with the butane sources for each PR.

@jschintag
Copy link
Contributor Author

@dustymabe Yes, i wasn't sure about the ignition files either. I removed them.

@jschintag I think for now let's leave the generated *.ign files out of the git repo and just update the README with further instructions.

Since we already have an internal how-to on how to deploy the RHCOS Builder, with instructions for the specific hardware, i think it does not make sense do add that bit here.

ignition:
config:
merge:
- source: http://foo/bar/builder-common.ign
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use local here instead of source to use a locally generated file.
See

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, since in order to install it on LPAR, we need to fetch the files over http(s). That is why i have the placeholder here.

@dustymabe
Copy link
Member

I think in the latest update we lost setting the hostname to something different. Not sure if that was intended or not.

@jschintag
Copy link
Contributor Author

I think in the latest update we lost setting the hostname to something different. Not sure if that was intended or not.

Yes, i lost it intentionally since it is not really needed to keep the hostname of the machine special.

Comment on lines 3 to 5
# - Merge in the builder-common.ign Ignition file
# - Allow the builder user to log in with the associated ssh key
# - Set a hostname
# - Create and initialize the secex-data volume
Copy link
Member

Choose a reason for hiding this comment

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

This needs updating:

Suggested change
# - Merge in the builder-common.ign Ignition file
# - Allow the builder user to log in with the associated ssh key
# - Set a hostname
# - Create and initialize the secex-data volume
# - Merge in the coreos-s390x-builder.ign Ignition file
# - Create and initialize the secex-data volume

Might have to be split across the two commits you have in this PR.

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 updated the description accordingly and split it between the 2 commits

Add the builder config for the s390x RHCOS builder.

Signed-off-by: Jan Schintag <[email protected]>
Add a script to initialize secex-data volume during installation.
This is achieved by having the tarball stored on a second disk.

Also run a podman container that mounts the volume to keep it from being
pruned. See: containers/podman#17051

Signed-off-by: Jan Schintag <[email protected]>
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@dustymabe dustymabe merged commit 9251e20 into coreos:main Mar 1, 2023
@jschintag jschintag deleted the s390x-lpar branch March 1, 2023 15:45
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.

2 participants