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

manifests: Add RHEL 9.0 based RHCOS and SCOS #773

Closed
wants to merge 13 commits into from

Conversation

travier
Copy link
Member

@travier travier commented Apr 7, 2022

Add the manifests needed to build RHEL 9.0 based RHCOS and CentOS Stream CoreOS (SCOS)

Based on #803.

# RHEL 9.0 based RHCOS
$ mkdir rhcos-rhel-9.0 && cd rhcos-rhel-9.0 && cosa init https://github.com/travier/os -b scos rhel-9.0

# CentOS Stream CoreOS (SCOS)
$ mkdir scos && cd scos && cosa init https://github.com/travier/os -b scos c9s

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2022
@travier
Copy link
Member Author

travier commented Apr 7, 2022

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2022
@openshift-ci openshift-ci bot requested review from ashcrow and jmarrero April 7, 2022 18:14
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2022
@LorbusChris
Copy link
Member

@travier I'm not sure if the setenforce 0 service for SCOS was intended to be included here, but if it was, could you split it out into a separate commit and add some justification as to why it is required?

@ashcrow ashcrow requested review from sohankunkerkar and HuijingHei and removed request for ashcrow and jmarrero April 7, 2022 19:05
@HuijingHei
Copy link
Contributor

@travier I'm not sure if the setenforce 0 service for SCOS was intended to be included here, but if it was, could you split it out into a separate commit and add some justification as to why it is required?

Refer to https://bugzilla.redhat.com/show_bug.cgi?id=1988164, it would be better to add the bug link to track the issue

@HuijingHei
Copy link
Contributor

HuijingHei commented Apr 8, 2022

Get following error, maybe need to fix this first
"CONFLICT (distinct types): manifest.yaml had different types on each side; renamed one of them so each can be recorded somewhere.\nAutomatic merge failed; fix conflicts and then commit the result."

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Overall looks good to me.

Re the symlink swap thing though... see coreos/coreos-assembler#1459

ci/prow-build-test-qemu.sh Outdated Show resolved Hide resolved
manifest-scos.yaml Outdated Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Apr 13, 2022

Instead of having to update the symlink (which dirties the git repo), we could put it in a subdirectory, e.g. scos/ where scos/manifest.yaml is a symlink to ../manifest-scos.yaml. Then we can use cosa init's support for subdirectories.

@HuijingHei
Copy link
Contributor

/retest

@travier
Copy link
Member Author

travier commented Apr 21, 2022

Instead of having to update the symlink (which dirties the git repo), we could put it in a subdirectory, e.g. scos/ where scos/manifest.yaml is a symlink to ../manifest-scos.yaml. Then we can use cosa init's support for subdirectories.

I've tried that in the latest commit. Not sure it's a great option. It emphasizes making two different cosa init working dir to build both RHCOS and SCOS when I would prefer we push for building both from a single cosa init to make testing a change for both easier. It would also only be used by the CI as swapping the symlink is still the easier option locally. I'm starting to think that we should revive the variant work from Colin.

@travier travier force-pushed the scos branch 3 times, most recently from f975e2d to 8e569dc Compare April 22, 2022 13:22
@travier
Copy link
Member Author

travier commented Apr 22, 2022

coreos/coreos-assembler#2802

Will have to be retried once the above PR made its way into a new cosa build.

@cgwalters
Copy link
Member

It emphasizes making two different cosa init working dir to build both RHCOS and SCOS when I would prefer we push for building both from a single cosa init to make testing a change for both easier.

Hmmmm. At least personally, I have separate working directories for them. How often would one really want to build both in the same working directory? It feels weird because then what happens when you cosa run and kola run is dependent on the state of the last build. Feels like it'd make things less predictable.

This is not a really strongly held opinion but I lean a bit towards pulling off the band-aid and saying clearly: we need to do both and support both equally. And that means there shouldn't be a default. Instead, let's move the rhel bits into e.g. rhel/ and the stream bits into stream/ and require cosa init https://github.com/openshift/os rhel or cosa init https://github.com/openshift/os stream.

@travier
Copy link
Member Author

travier commented Apr 22, 2022

Using explicit directories could be a good idea indeed. I'd say having a simple way to build both from the same checkout is useful when testing a change that will impact both instead of having to sync the change between two clones but it's not that big of a deal so I'm fine either way.

@travier
Copy link
Member Author

travier commented Apr 22, 2022

/retest

@cgwalters
Copy link
Member

cgwalters commented Apr 22, 2022

I'd say having a simple way to build both from the same checkout is useful when testing a change that will impact both instead of having to sync the change between two clones

Ah but locally, you have that today! What I do is e.g.:
cosa init ~/src/github/openshift/os - that will use a git repository I've cloned in my standard place along with other git repos. IOW, the git repo is not actually in the builddir.

So concretely to set up both in this proposal:

for d in rhel stream; do
  mkdir $d
  (cd $d && cosa init ~/src/github/openshift/os $d)
done

And the two will share the same git checkout. If you update e.g. something in the manifest, you can do:

for d in rhel stream; do 
  (cd $d && cosa build && kola run ...)
done

to build and test both.

HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request Jun 10, 2022
HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request Jun 10, 2022
See openshift/os#773
Fix issue
```
$ cosa kola run ostree.basic
kola -p qemu-unpriv --output-dir tmp/kola run ostree.basic
=== RUN   ostree.basic
--- FAIL: ostree.basic (57.14s)
        harness.go:1443: mach.Start() failed: machine "ea43750e-e04b-4632-b966-caff659a5214" failed basic checks: not a supported instance: scos-coreos
FAIL, output in tmp/kola
Error: harness: test suite failed
2022-06-10T09:12:23Z cli: harness: test suite failed
```
HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request Jun 10, 2022
Supported:
- scos-
- scos-coreos
- rhcos-
- rhcos-coreos

See openshift/os#773
Fix issue
```
$ cosa kola run ostree.basic
        harness.go:1443: mach.Start() failed: machine "ea43750e-e04b-4632-b966-caff659a5214" failed basic checks: not a supported instance: scos-coreos
```
- Only build the ostree aci container archive in the build step as it's
  the only artifact used for the layering test.
- Group the remaining tests in two buckets: qemu & metal.
- Disable the now uneeded tests.
Prepare for testing multiple RHEL versions.
Pre-create test names to be able to enable them in Prow before we merge
the corresponding test code.
Those scripts have been merged into prow-entrypoint.sh.
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 10, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cgwalters, travier

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2022
@travier
Copy link
Member Author

travier commented Jun 10, 2022

Needs openshift/release#29371 first

@travier
Copy link
Member Author

travier commented Jun 13, 2022

Needs #803

@HuijingHei
Copy link
Contributor

/retest

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2022

@travier: PR needs rebase.

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2022

@travier: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/build-test-qemu 9594913 link true /test build-test-qemu
ci/prow/test-in-cluster 10365d0 link false /test test-in-cluster
ci/prow/scos-9-build-test-qemu 1d8463d link true /test scos-9-build-test-qemu
ci/prow/rhcos-90-build-test-qemu 1d8463d link true /test rhcos-90-build-test-qemu
ci/prow/validate 1d8463d link true /test validate
ci/prow/scos-9-build-test-metal 1d8463d link true /test scos-9-build-test-metal
ci/prow/rhcos-90-build-test-metal 1d8463d link true /test rhcos-90-build-test-metal
ci/prow/rhcos-86-build-test-metal 1d8463d link true /test rhcos-86-build-test-metal
ci/prow/rhcos-86-build-test-qemu 1d8463d link true /test rhcos-86-build-test-qemu

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.

@travier
Copy link
Member Author

travier commented Jun 20, 2022

Closing in favor of #854

@travier travier closed this Jun 20, 2022
HuijingHei added a commit to HuijingHei/coreos-assembler that referenced this pull request Jul 21, 2022
Supported:
- scos-
- scos-coreos
- rhcos-
- rhcos-coreos

See openshift/os#773
Fix issue
```
$ cosa kola run ostree.basic
        harness.go:1443: mach.Start() failed: machine "ea43750e-e04b-4632-b966-caff659a5214" failed basic checks: not a supported instance: scos-coreos
```
HuijingHei added a commit to coreos/coreos-assembler that referenced this pull request Jul 21, 2022
Supported:
- scos-
- scos-coreos
- rhcos-
- rhcos-coreos

See openshift/os#773
Fix issue
```
$ cosa kola run ostree.basic
        harness.go:1443: mach.Start() failed: machine "ea43750e-e04b-4632-b966-caff659a5214" failed basic checks: not a supported instance: scos-coreos
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants