RHEL 8.5: main image types #1536

achilleas-k · 2021-07-08T11:22:33Z

This pull request includes:

adequate testing for the new functionality or fixed issue
adequate documentation informing people about the change such as
- create a file in news/unreleased directory if this change should be mentioned in the release news
- submit a PR for the guides repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

~~Draft PR to get CI running base tests on RHEL 8.5. Some failures are expected:~~
~~- AMI not implemented. I intend to add the base image after #1525 is merged.~~
~~- Still generating test manifests for all non-x86_64 architectures.~~

TODO:

AMI base image:
- Update packages match official ks (COMPOSER-840)
Unit tests for new stages
Test manifests for s390x
Test manifests for ppc64le
Allow force-installing excluded packages when specified in the blueprint

Changes to the AMI image type and AMI variants listed in COMPOSER-840 will be added in a later PR.

atodorov · 2021-07-08T13:50:06Z

Depsolve error missing packages: nss, otherwise looks good to me.

atodorov

Minor change requested.

However Base tests still failing with:
DNF error occured: MarkingErrors: Error occurred when marking packages for installation: Problems in request:\nmissing packages: nss

It's regression.sh which fails.

.gitlab-ci.yml

achilleas-k · 2021-07-12T09:26:38Z

Minor change requested.

However Base tests still failing with:
DNF error occured: MarkingErrors: Error occurred when marking packages for installation: Problems in request:\nmissing packages: nss

It's regression.sh which fails.

Yeah, sorry. Wasn't ignoring your earlier mention, I just wanted to get everything else in before trying to figure out the source of that failure.

Thanks for the pointer to regression.sh. I see it now. Since 8.5 image types are heavily rewritten from the older ones, Martin's patch to force-include excluded packages isn't included.

achilleas-k · 2021-07-12T17:24:12Z

Marking this as ready for review now. Still could use some test manifests for s390x and ppc64le but I want to mark it as ready and ask for reviews.

achilleas-k · 2021-07-13T09:28:41Z

It should be possible to install packages that have dependencies in the excluded list now. I'm going to extend the regression test to check for this scenario as well.

thozza

Nice work. I added a few cosmetic comments. In addition, I got two ideas during the review:

I feel like we should define the commonly used package set names as constants and use those in the code, instead of string literals.
I wonder whether most of the functions in the internal/distro/rhel85/stage_options.go could be reused by other distro definitions. Some of the functions would have to take an additional argument or two, but they look like something we would want to use elsewhere as well. So maybe we could move them to the internal/osbuild2 package?

thozza · 2021-07-13T12:00:24Z

internal/distro/rhel85/stage_options.go

+	}
+}
+
+func findBootPartition(pt *disk.PartitionTable) uint {


Wouldn't this make more sense as a disk.PartitionTable method in the internal/disk package? I mean in case this is something we would need for other distro definitions as well...

Agreed. It's a generally useful utility. Moving.

thozza · 2021-07-13T12:17:40Z

internal/osbuild2/qemu_stage_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewQemukStage(t *testing.T) {


There is an extra "k" in TestNewQemukStage

achilleas-k · 2021-07-13T14:32:40Z

I feel like we should define the commonly used package set names as constants and use those in the code, instead of string literals.

That's a good idea. In my head the initial design was that package set names should have no special meaning. The idea was that the distro uses arbitrary strings to refer to package sets, returns them for depsolving from the PacakageSets() method, and then the distro itself is responsible for interpreting the names.
But it's clear now that we will always have package sets with special meaning (build, boot, packages, ...), so it's definitely a good idea to define those strings in one place.

I wonder whether most of the functions in the internal/distro/rhel85/stage_options.go could be reused by other distro definitions. Some of the functions would have to take an additional argument or two, but they look like something we would want to use elsewhere as well. So maybe we could move them to the internal/osbuild2 package?

The functions in the stage_options.go file are the followup to the xStageOptions() methods from previous image type definitions (e.g., rpmStageOptions()) so they ended up in the same place for 8.5. I agree though. They would be a lot more useful if they could be generalised and added to each stage in the osbuild2 package as analogues to the NewXStage() function. Right now I'm not sure how much work it would take to move them over, since while writing them I knew they were only meant to be used in the rhel85 package and there are several 8.5-specific hard-coded values.

I'll open an issue for it as a reminder.

ondrejbudai

First things first - this is pure awesomeness!

Second things second - if typos are the only issues of this PR, let's merge it and fix them in a follow-up. It just felt wrong not to point them out.

Let's move to more serious stuff though: I noticed that integration and API tests are not enabled for RHEL 8.5 by this PR. We want to make sure that we always merge fully tested PRs. I would like to ask what's the plan with them? As always, I'm more than happy to help with enabling them. :)

Great work again, I'm looking forward to porting all of these to RHEL 9.0. :-)

ondrejbudai · 2021-07-13T08:58:49Z

internal/distro/rhel85/pipelines.go

+
+// mkfsStages generates a list of org.osbuild.mkfs.* stages based on a
+// partition table description for a single device node
+func mkfsStages(pt *disk.PartitionTable, device *osbuild.Device) []*osbuild2.Stage {


I wonder if this is something that can be shared across distributions and can live in internal/disk.

The same applies to all methods that return stages/input/devices for partitioning stuff.

(if we decide to go this way, this is surely something for a follow-up)

I agree. I think we should make a push for this at some point along with #1550.

ondrejbudai · 2021-07-13T20:21:08Z

test/cases/regression-excluded-dependency.sh

+# Get OS data.
+source /etc/os-release
+
+# Provision the software under tet.


tet => test

This is definitely not your fault, we have this typo everywhere. :D

ondrejbudai · 2021-07-13T20:22:41Z

test/cases/regression-excluded-dependency.sh

+# Provision the software under tet.
+/usr/libexec/osbuild-composer-test/provision.sh
+
+if [[ "${ID}-${VERSION_ID}" != "rhel-8.5" ]]; then


It would be great to have this future-proof and run this on rhel >= 8.5. Please, do not solve it in this PR, this is just a reminder for us that we should think about this.

ondrejbudai · 2021-07-13T20:22:55Z

test/cases/regression-excluded-dependency.sh

+description = "A base system with redhat-lsb-core"
+version = "0.0.1"
+
+# The nss package is excluded in the RHEL8.4 image type and is required by the


Fixed. Thanks!

ondrejbudai · 2021-07-13T20:24:17Z

internal/distro/rhel85/distro.go

@@ -213,7 +213,10 @@ func (t *imageType) PackageSets(bp blueprint.Blueprint) map[string]rpmmd.Package
 	if timezone != nil {
 		bpPackages = append(bpPackages, "chrony")
 	}
-	mergedSets["packages"] = mergedSets["packages"].Append(rpmmd.PackageSet{Include: bpPackages})
+
+	// repsolve bp packages separately


Fixed (actually, wanted to say 'depsolve'). Thanks!

it's actually vpc, not vhd Signed-off-by: Ondřej Budai <[email protected]>

Previously, the first boot stage was added twice if the subscription customization was enabled. This doesn't work because the first boot stage cannot be specified twice in one pipeline. Also, it didn't make much sense because it just duplicated the effort so I just removed one of the two stages. Signed-off-by: Ondřej Budai <[email protected]>

Signed-off-by: Ondřej Budai <[email protected]>

Nightly composes are more stable because they contain only builds attached to advisories whereas development composes contain latest brew builds. Use nightlies because they should be stabler. Signed-off-by: Ondřej Budai <[email protected]>

Originally, a copy of an architecture instance was always created when it was added to a distro definition using the `addArches()` method. However in reality, only a subset of structure members were copied, which could create unexpected behavior and issues. This behavior is identical to the behavior when image types are added to an architecture. However the situation with image types differs in one aspect, specifically that a single image type definition is usually reused by multiple architecture definitions, while an architecture definition is always used only by a single distribution definition. Due to the fact that the image type contains a reference to the architecture to which it has been added, the creation of a copy can not be reasonably avoided. On the other hand, adding a copy of an architecture to a distribution definition is not necessary. Downside of creating copies of the architecture is that the image types associated with it referred always to the original architecture definition instance and not to the copy. So while references in the direction of Distro -> Arch -> Image Type were correct and working, the other direction was broken. Image Type -> (original) Arch -> (nil) Distro. Modify `distribution.AddArches()` method to directly add the passed architecture instances to the distribution definition, instead of adding their copies. Signed-off-by: Tomas Hozza <[email protected]>

Refactor data structures used for test cases in the `TestFilenameFromType()` to have more descriptive names. Signed-off-by: Tomas Hozza <[email protected]>

EDGE image types are defined under a different name for RHEL-8.5, specifically they don't contain the "rhel-" prefix any more. To ensure backward compatibility, add image type aliases for all EDGE image types with the "rhel-" prefix. Image type aliases are used only when getting a specific imageType instance by its name. When listing all available image types for an architecture, only the current image type names are returned, without any aliases. This prevents the image types from being exposed multiple times under different names via Weldr API. Extend the distro unit tests to test image type aliases. Signed-off-by: Tomas Hozza <[email protected]>

osbuild has recently got support for specifying mounts as an array. This commit takes advantage of it and uses this new format. This allows us to specify the order of mounts which is important because we cannot mount /boot/efi before / is mounted. Signed-off-by: Ondřej Budai <[email protected]>

Previously, /boot/efi mount was specified before /. This obviously doesn't work because we need to mount / firstly. This commit adds explicit ordering of the mounts. Signed-off-by: Ondřej Budai <[email protected]>

Use the package set key constants defined in distro in the pipeline definitions as well.

Blueprint packages are now defined and passed into Manifest() separately. The main osPipelines() already has an argument for explicitly passing the blueprint packages. Added the same for the ostree pipelines.

Since partitions without a filesystem are skipped, we need to dynamically append to create the mounts array instead of pre-allocating to the number of partitions.

When not flushed, the line often doesn't get printed until after the job is done. Printing it before the job is useful for knowing the progress of a multi-job run.

The specific combination isn't broken in RHEL 8.5. The condition check was added accidentally when copying the tests from RHEL 8.4.

Instead of inspecting the tarball directly, extract it and use ostree to verify the ref and commit ID. Adds some data to the CI artifacts directory: - Build manifest - Tarball file list for s3 edge commit with s3 upload - Build metadata

If there's no kernel in the main package set, the standard/default kernel will be added while depsolving. This causes issues when an alternative kernel is selected in the blueprint. Both kernels will be installed (one from the blueprint and one from the main OS set) which causes issues with ostree image types.

Requires support for new features not yet released in RHEL 8.5

The edge-installer build root requires the installer build package set.

ondrejbudai

AWESOME! Thanks everyone for this!

achilleas-k force-pushed the rhel85/main-image-types branch from e15c6f5 to d49d5fc Compare July 8, 2021 11:29

achilleas-k force-pushed the rhel85/main-image-types branch 3 times, most recently from 5db518e to 155ce00 Compare July 8, 2021 23:54

achilleas-k mentioned this pull request Jul 9, 2021

tests: make azure.sh easier to run locally #1511

Merged

4 tasks

achilleas-k force-pushed the rhel85/main-image-types branch 2 times, most recently from faedb5c to cfd9016 Compare July 9, 2021 17:02

atodorov requested changes Jul 12, 2021

View reviewed changes

.gitlab-ci.yml Outdated Show resolved Hide resolved

achilleas-k force-pushed the rhel85/main-image-types branch 2 times, most recently from 687c6a9 to 445b573 Compare July 12, 2021 17:21

achilleas-k requested a review from thozza July 12, 2021 17:21

achilleas-k marked this pull request as ready for review July 12, 2021 17:22

achilleas-k force-pushed the rhel85/main-image-types branch from 445b573 to c35d354 Compare July 12, 2021 17:25

teg previously approved these changes Jul 12, 2021

View reviewed changes

atodorov previously approved these changes Jul 13, 2021

View reviewed changes

achilleas-k dismissed stale reviews from atodorov and teg via c7c9f16 July 13, 2021 12:27

achilleas-k force-pushed the rhel85/main-image-types branch 2 times, most recently from c7c9f16 to 106cfbf Compare July 13, 2021 12:31

thozza previously approved these changes Jul 13, 2021

View reviewed changes

achilleas-k dismissed thozza’s stale review via 01a2b51 July 13, 2021 13:05

achilleas-k force-pushed the rhel85/main-image-types branch from 106cfbf to 01a2b51 Compare July 13, 2021 13:05

achilleas-k mentioned this pull request Jul 13, 2021

Add functions that generalise stage option struct creation #1550

Open

achilleas-k force-pushed the rhel85/main-image-types branch from 6607a06 to ccd2257 Compare July 13, 2021 15:12

ondrejbudai reviewed Jul 13, 2021

View reviewed changes

ondrejbudai and others added 24 commits July 24, 2021 00:10

distro/rhel85: fix name of exported pipeline

2286940

it's actually vpc, not vhd Signed-off-by: Ondřej Budai <[email protected]>

test/libvirt: run the uefi test also for 8.5

88a7791

Signed-off-by: Ondřej Budai <[email protected]>

distro/rhel85: refactor TestFilenameFromType test

22784d0

Refactor data structures used for test cases in the `TestFilenameFromType()` to have more descriptive names. Signed-off-by: Tomas Hozza <[email protected]>

distro/rhel85: sort mounts

43d6809

Previously, /boot/efi mount was specified before /. This obviously doesn't work because we need to mount / firstly. This commit adds explicit ordering of the mounts. Signed-off-by: Ondřej Budai <[email protected]>

distro/rhel85: package set key consts in pipelines

8dbaefe

Use the package set key constants defined in distro in the pipeline definitions as well.

distro/rhel85: add blueprint pkgs to ostree pipeline

5ff2513

Blueprint packages are now defined and passed into Manifest() separately. The main osPipelines() already has an argument for explicitly passing the blueprint packages. Added the same for the ostree pipelines.

distro/rhel85: init mounts array to 0 elements

7513b09

Since partitions without a filesystem are skipped, we need to dynamically append to create the mounts array instead of pre-allocating to the number of partitions.

osbuild2: base device options are optional

ce9ddf5

tools: flush print in test case generator

21f9263

When not flushed, the line often doesn't get printed until after the job is done. Printing it before the job is useful for knowing the progress of a multi-job run.

distro/rhel85: remove broken arch-imgType combo check

f962fd5

The specific combination isn't broken in RHEL 8.5. The condition check was added accidentally when copying the tests from RHEL 8.4.

test/api.sh: more robust edge commit validation

d9c63d7

Instead of inspecting the tarball directly, extract it and use ostree to verify the ref and commit ID. Adds some data to the CI artifacts directory: - Build manifest - Tarball file list for s3 edge commit with s3 upload - Build metadata

test/api.sh: suppress shellcheck for sudo redirect

f9e21c4

Schutzfile: bump osbuild for RHEL 8.5

afa0edb

Requires support for new features not yet released in RHEL 8.5

distro/rhel85: fix edge-installer build packages

003ce1f

The edge-installer build root requires the installer build package set.

test/data: generate x86_64 test manifests

72f173b

test/data: generate aarch64 test manifests

c96d1b1

news: add entry about RHEL 8.5 images

5d27b5b

schutzbot/deploy.sh: dependency bug workaround

57de030

achilleas-k force-pushed the rhel85/main-image-types branch from f76740c to 57de030 Compare July 23, 2021 22:11

ondrejbudai approved these changes Jul 24, 2021

View reviewed changes

ondrejbudai merged commit c27ca81 into osbuild:main Jul 24, 2021

achilleas-k deleted the rhel85/main-image-types branch July 25, 2021 13:11

This was referenced Jul 27, 2021

Fix differences between 8.4 and 8.5 image definitions #1594

Merged

RHEL 8.5: fixes to image definitions #1603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHEL 8.5: main image types #1536

RHEL 8.5: main image types #1536

achilleas-k commented Jul 8, 2021 •

edited

Loading

atodorov commented Jul 8, 2021

atodorov left a comment

achilleas-k commented Jul 12, 2021

achilleas-k commented Jul 12, 2021 •

edited

Loading

achilleas-k commented Jul 13, 2021

thozza left a comment

thozza Jul 13, 2021 •

edited

Loading

achilleas-k Jul 13, 2021

thozza Jul 13, 2021

achilleas-k Jul 13, 2021

achilleas-k commented Jul 13, 2021

ondrejbudai left a comment

ondrejbudai Jul 13, 2021

achilleas-k Jul 14, 2021

ondrejbudai Jul 13, 2021

ondrejbudai Jul 13, 2021

ondrejbudai Jul 13, 2021

achilleas-k Jul 14, 2021

ondrejbudai Jul 13, 2021

achilleas-k Jul 14, 2021

ondrejbudai left a comment

RHEL 8.5: main image types #1536

RHEL 8.5: main image types #1536

Conversation

achilleas-k commented Jul 8, 2021 • edited Loading

atodorov commented Jul 8, 2021

atodorov left a comment

Choose a reason for hiding this comment

achilleas-k commented Jul 12, 2021

achilleas-k commented Jul 12, 2021 • edited Loading

achilleas-k commented Jul 13, 2021

thozza left a comment

Choose a reason for hiding this comment

thozza Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achilleas-k commented Jul 13, 2021

ondrejbudai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejbudai left a comment

Choose a reason for hiding this comment

achilleas-k commented Jul 8, 2021 •

edited

Loading

achilleas-k commented Jul 12, 2021 •

edited

Loading

thozza Jul 13, 2021 •

edited

Loading