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

NO-JIRA: tests/replace-rt-kernel: fix and improve #1506

Merged
merged 2 commits into from
May 10, 2024

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented May 10, 2024

It's a bit odd to have this test pull the c9s.repo yum repo from the
master branch of openshift/os. We don't want to be in a situation where
a change in master breaks older branches.

But also, c9s.repo could at any point in time refer to either the
latest compose output or the official mirrors. But we always want to
use the official mirrors for this test since that has multiple package
versions and we need to be able to select a kernel version which is not
equal to ours.

We could curl the official centos.repo file (and actually, in the
SCOS case, it's already in the image), but that file doesn't include
additional repos like rt and nfv which we need for our test.

Instead, just bake the config into the test data. That way we don't need
to curl anything at all, and the repo definition is naturally bound to
the test and can be modified together atomically.

A few additional changes:

  1. only do all the repo preparations stuff on the first boot
  2. add logic to pick a kernel that's different from the one we're on if
    we're already CentOS-based (this fixes [rhel-9.4 variant] ext.config.rpm-ostree.replace-rt-kernel fails #1383)
  3. add a stub for c10s for when we're ready

Closes: #1383

@openshift-ci openshift-ci bot requested review from jmarrero and ravanelli May 10, 2024 18:01
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2024
@jlebon
Copy link
Member Author

jlebon commented May 10, 2024

I've tested this on master (RHCOS and SCOS variants), release-4.15 (RHCOS), and release-4.14 (RHCOS).

It's a bit odd to have this test pull the `c9s.repo` yum repo from the
master branch of openshift/os. We don't want to be in a situation where
a change in master breaks older branches.

But also, `c9s.repo` could at any point in time refer to either the
latest compose output or the official mirrors. But we always want to
use the official mirrors for this test since that has multiple package
versions and we need to be able to select a kernel version which is not
equal to ours.

We could curl the official `centos.repo` file (and actually, in the
SCOS case, it's already in the image), but that file doesn't include
additional repos like `rt` and `nfv` which we need for our test.

Instead, just bake the config into the test data. That way we don't need
to curl anything at all, and the repo definition is naturally bound to
the test and can be modified together atomically.

A few additional changes:
1. only do all the repo preparations stuff on the first boot
2. add logic to pick a kernel that's different from the one we're on if
   we're already CentOS-based (this fixes openshift#1383)
3. add a stub for c10s for when we're ready

Closes: openshift#1383
Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

lgtm

@jmarrero
Copy link
Member

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2024
Copy link
Member

Choose a reason for hiding this comment

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

should we mention somewhere in here where we got this from and how to update it in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Will add a comment in a follow-up.

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

Copy link
Contributor

openshift-ci bot commented May 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dustymabe, jlebon, jmarrero

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 [dustymabe,jlebon,jmarrero]

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

Copy link
Contributor

openshift-ci bot commented May 10, 2024

@jlebon: 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-sigs/prow repository. I understand the commands that are listed here.

@jlebon jlebon changed the title tests/replace-rt-kernel: fix and improve NO-JIRA: tests/replace-rt-kernel: fix and improve May 10, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 10, 2024
@openshift-ci-robot
Copy link

@jlebon: This pull request explicitly references no jira issue.

In response to this:

It's a bit odd to have this test pull the c9s.repo yum repo from the
master branch of openshift/os. We don't want to be in a situation where
a change in master breaks older branches.

But also, c9s.repo could at any point in time refer to either the
latest compose output or the official mirrors. But we always want to
use the official mirrors for this test since that has multiple package
versions and we need to be able to select a kernel version which is not
equal to ours.

We could curl the official centos.repo file (and actually, in the
SCOS case, it's already in the image), but that file doesn't include
additional repos like rt and nfv which we need for our test.

Instead, just bake the config into the test data. That way we don't need
to curl anything at all, and the repo definition is naturally bound to
the test and can be modified together atomically.

A few additional changes:

  1. only do all the repo preparations stuff on the first boot
  2. add logic to pick a kernel that's different from the one we're on if
    we're already CentOS-based (this fixes [rhel-9.4 variant] ext.config.rpm-ostree.replace-rt-kernel fails #1383)
  3. add a stub for c10s for when we're ready

Closes: #1383

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 8e6ac83 into openshift:master May 10, 2024
7 checks passed
@jlebon
Copy link
Member Author

jlebon commented May 10, 2024

/cherrypick release-4.15 release 4.14

@openshift-cherrypick-robot

@jlebon: #1506 failed to apply on top of branch "release-4.15":

Applying: denylist: drop ext.config.rpm-ostree.replace-rt-kernel
Using index info to reconstruct a base tree...
M	kola-denylist.yaml
Falling back to patching base and 3-way merge...
Auto-merging kola-denylist.yaml
CONFLICT (content): Merge conflict in kola-denylist.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 denylist: drop ext.config.rpm-ostree.replace-rt-kernel
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-4.15 release 4.14

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-sigs/prow repository.

jlebon added a commit to jlebon/os that referenced this pull request Aug 7, 2024
Follow-up to 15c7bf9 ("tests/replace-rt-kernel: fix and improve"). This
was pointed out during code review in openshift#1506.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rhel-9.4 variant] ext.config.rpm-ostree.replace-rt-kernel fails
5 participants