Skip to content

Conversation

@sinnykumari
Copy link
Contributor

@sinnykumari sinnykumari commented Jul 31, 2020

It seems the kernel-rt-core package may change uname -a
content. In older package it included substring PREEMPT RT
which we were using to determine that current booted kernel
in rt kernel.

This changed recently to PREEMPT_RT, due to which tests started
failing. Hopefully just checking for PREEMPT is sufficient.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2020
It seems the kernel-rt-core package may change uname -a
content. In older package it included substring PREEMPT RT
which we were using to determine that current booted kernel
in rt kernel. This changed recently to PREEMPT_RT, due to
which tests started failing.

To test if node is on rt-kernel, check that kernel-rt-core
package is installed.
@runcom runcom changed the title test/e2e: search for PREEMPT string to check for rt-kernel test/e2e: check that kernel-rt-core pacakge is installed Jul 31, 2020
@runcom
Copy link
Member

runcom commented Jul 31, 2020

let's wait and see how it goes with this

/approve

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.

LGTM as is


kernelInfo := execCmdOnNode(t, cs, infraNode, "uname", "-a")
if !strings.Contains(kernelInfo, "PREEMPT RT") {
kernelInfo := execCmdOnNode(t, cs, infraNode, "chroot", "/rootfs", "rpm", "-qa", "kernel-rt-core")
Copy link
Member

Choose a reason for hiding this comment

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

The -a there is unnecessary (but fine as is, we can address it in a followup). We should also probably make execCmdOnNode do the chroot /rootfs internally because basically everything else in this function is targeting the real root too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-a is not necessary but -a explicitly says query/verify all packages and that made me use it :)
yeah, you are right. We can look at execCmdOnNode() to do chroot /rootfs

Copy link
Member

Choose a reason for hiding this comment

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

the funny thing is the other uses of execCmdOnNode play from outside the chroot by just string matching on files within /rootfs - but I agree we can implement chroot in the function to make it transparent that commands are executed on the node's root directly

kernelInfo = execCmdOnNode(t, cs, infraNode, "uname", "-a")
if strings.Contains(kernelInfo, "PREEMPT RT") {
kernelInfo = execCmdOnNode(t, cs, infraNode, "chroot", "/rootfs", "rpm", "-qa", "kernel-rt-core")
if strings.Contains(kernelInfo, "kernel-rt-core") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if strings.Contains(kernelInfo, "kernel-rt-core") {
if strings.hasPrefix(kernelInfo, "kernel-rt-core") {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! let's fix this in a followup PR to unblock tests and pending prs.

@cgwalters
Copy link
Member

This can't affect anything except our own tests, no reason to waste resources on testing the install with this
/override ci/prow/e2e-aws
/override ci/prow/e2e-aws-scaleup-rhel7

@openshift-ci-robot
Copy link
Contributor

@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/e2e-aws, ci/prow/e2e-aws-scaleup-rhel7

Details

In response to this:

This can't affect anything except our own tests, no reason to waste resources on testing the install with this
/override ci/prow/e2e-aws
/override ci/prow/e2e-aws-scaleup-rhel7

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.

@sinnykumari
Copy link
Contributor Author

can someone add lgtm label

@sinnykumari
Copy link
Contributor Author

skip isn't friendly when test is running

@sinnykumari
Copy link
Contributor Author

/skip

@openshift-ci-robot
Copy link
Contributor

@sinnykumari: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-ovn-step-registry 373ddae link /test e2e-ovn-step-registry
ci/prow/okd-e2e-aws 373ddae link /test okd-e2e-aws

Full PR test history. Your PR dashboard.

Details

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.

@sinnykumari
Copy link
Contributor Author

/skip

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom, sinnykumari

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [cgwalters,runcom,sinnykumari]

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

@sinnykumari
Copy link
Contributor Author

/lgtm

Thanks 🎉

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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants