Install RT kernel from RPMs included in RHCOS #34

slintes · 2020-01-09T12:54:17Z

With this PR we install the RT kernel from the RPMs provided in RHCOS 44.81.
This removes the need for a yum repo.

Not sure if the installation of the RT kernel should be default, but can be disabled on the CRD? Atm it's the other way around, but probably in most cases we want the RT kernel.

TODO:

investigate failing reboot of worker node -> didn't happen again after rebase
add functest
rebase after Implement operator logic #25 and Deploy performance components under CI #27 are merged

cynepco3hahue · 2020-01-09T14:06:15Z

hack/label-worker-rt.sh

+echo "[INFO]:labeling worker nodes"
+for node in $(${OC_TOOL} get nodes --selector='!node-role.kubernetes.io/master' -o name); do
+    ${OC_TOOL} label $node node-role.kubernetes.io/worker-rt=""
+done


nit: new line

cynepco3hahue · 2020-01-09T14:07:47Z

pkg/controller/performanceprofile/components/machineconfig/machineconfig.go

 	ignitionConfig.Systemd = igntypes.Systemd{
 		Units: []igntypes.Unit{
 			{
 				Contents: rtKernelService,
-				Enabled:  pointer.BoolPtr(true),
+				Enabled:  &enableRTKernel,


Should we create at all RT kernel unit if the RT disabled, also pre-tunning also depends on the RT kernel unit, so I am interesting if it will work as expected?

ah yes, good point
I'm wondering if we need the option to not install the RT kernel at all, when other performance features depend on it (no clue if all of them do?)

other than that, pre-tuning uses wants and not requires, so it would be started without rt-kernel (to my best understanding)

AFAIK it is correct, if an unit wants another unit and the latter can't be started, the former should run anyway (see: https://unix.stackexchange.com/questions/388586/systemd-requires-vs-wants - which in turn quotes manpages).
Question is however is if makes sense to do pretuning without rt-kernel, like Marc mentionedin on his grandparent comment.

Signed-off-by: Marc Sluiter <[email protected]>

davidvossel · 2020-01-09T19:38:38Z

weird, the e2e test failed, but it actually looks like it happened after our e2e test suite successfully completed. I'll run it again.

2020/01/09 17:38:37 Container test in pod e2e-gcp completed successfully
2020/01/09 17:38:38 error: unable to gather container logs: [error: Unable to retrieve logs from pod container cli: container "cli" in pod "e2e-gcp" is terminated, error: Unable to retrieve logs from pod container artifacts: container "artifacts" in pod "e2e-gcp" is terminated, error: Unable to retrieve logs from pod container setup: container "setup" in pod "e2e-gcp" is terminated, error: Unable to retrieve logs from pod container teardown: container "teardown" in pod "e2e-gcp" is terminated, error: Unable to retrieve logs from pod container test: container "test" in pod "e2e-gcp" is terminated]
2020/01/09 17:38:38 error: unable to signal to artifacts container to terminate in pod e2e-gcp, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2020/01/09 17:38:38 error: unable to retrieve artifacts from pod e2e-gcp: could not read gzipped artifacts: unable to upgrade connection: container not found ("artifacts")

davidvossel · 2020-01-09T19:38:47Z

/retest

davidvossel

changes look good. I just want to see e2e pass before merging.

davidvossel · 2020-01-09T19:50:24Z

functests/performance/rt-kernel.go

+			if err != nil {
+				return ""
+			}
+			defer func() {


can we use an AfterEach() instead. I'm not 100% sure what happens if you do a ctrl-c to kill execution when a defer is in use. I know that ginkgo will attempt to run the AfterEach() functions even if you try to kill the test execution early though.

fair point, will change it

davidvossel · 2020-01-09T19:52:52Z

build/assets/scripts/rt-kernel.sh

-trap exit_handler EXIT
+# create and mount new container
+podman create --net=none --name "$osContainerName" "$imageID" > /dev/null
+kernelDir=$(podman mount "$osContainerName")


wow, so we're delivering kernel pkgs in containers? interesting.

AFAIU this is used to access the packages already shipped in the system image (so we are not shipping anything anymore). But I may have out of date information.

this is complicated, let me try to summarize:

the installer's bootstrap node installs an "old" os bootimage on cluster nodes, which is just new enough to be able to do everything which is needed on first boot

then the MCO installs a newer version of the os ("early pivot"). That one is delivered via container image. Maybe you noticed that OCP comes with a machine-os-content image: that one contains the os

with RHCOS version 44 the image contains the RT kernel RPMs. They were just put into the root directory of the image

so what I do here (stolen from MCO code): mount the image (which is not possible directly, so create (but don't run) a container from it) which contains the os version which is currently booted, and install the RPMs from the mount path.

longer version: https://github.com/openshift/machine-config-operator/blob/master/docs/OSUpgrades.md and others

Thanks @slintes great summary. I think it would be great to record this as comment in the rt-kernel.sh script itself. WDYT?

I thought REPO_URL was a workaround in 4.3 until we have machineconfig in 4.4 and I see you have a note here as "TEMPORARY WORKAROUND".

Are we having 2 workarounds now for 4.3 ?

The operator is for 4.4 only. There we have the RT kernel RPMs already available in the RHCOS image, so no need for a yum repo anymore, but the MCO is not ready to use it yet, see WIP PR [0]. So this is a new temporary workaround for 4.4, unrelated to 4.3.

[0] openshift/machine-config-operator#1330

@fromanirh ah missed your comment
will do

@fromanirh done

ffromani · 2020-01-10T07:51:47Z

hack/label-worker-rt.sh

@@ -0,0 +1,12 @@
+#!/bin/bash


who calls this? only humans or also the operator proper? I guess the former, asking to be sure.

we use a MachineConfig for installing this script and a systemd unit, and systemd will execute the script then.

oh sorry, just noticed I was looking at the wrong script
I called this one manually on test clusters

ffromani

At glance looks ok, only clarification questions inside

Signed-off-by: Marc Sluiter <[email protected]>

davidvossel

/lgtm

openshift-ci-robot · 2020-01-10T19:46:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, slintes

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~~ [davidvossel]

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

remove unused references to worker-rt label

slintes requested review from davidvossel and cynepco3hahue January 9, 2020 12:54

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2020

openshift-ci-robot requested review from ffromani and simon3z January 9, 2020 12:54

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 9, 2020

cynepco3hahue reviewed Jan 9, 2020

View reviewed changes

slintes changed the title ~~WIP Install RT kernel from RPMs included in RHCOS~~ Install RT kernel from RPMs included in RHCOS Jan 9, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2020

Install RT kernel from RPMs included in RHCOS

82d7cbe

Signed-off-by: Marc Sluiter <[email protected]>

davidvossel reviewed Jan 9, 2020

View reviewed changes

ffromani reviewed Jan 10, 2020

View reviewed changes

slintes added 2 commits January 10, 2020 10:33

Added functest

3ed78b6

Signed-off-by: Marc Sluiter <[email protected]>

Added comment about what is happening in the rt-kernel.sh script

b7dfb10

Signed-off-by: Marc Sluiter <[email protected]>

slintes mentioned this pull request Jan 10, 2020

Added kustomization for performance openshift-kni/cnf-features-deploy#4

Merged

davidvossel approved these changes Jan 10, 2020

View reviewed changes

openshift-ci-robot assigned davidvossel Jan 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2020

openshift-merge-robot merged commit 49b6321 into openshift-kni:master Jan 10, 2020

dshchedr pushed a commit to dshchedr/performance-addon-operators that referenced this pull request Mar 26, 2020

Merge pull request openshift-kni#34 from davidvossel/dead-code

b84cc29

remove unused references to worker-rt label

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install RT kernel from RPMs included in RHCOS #34

Install RT kernel from RPMs included in RHCOS #34

slintes commented Jan 9, 2020 •

edited

Loading

cynepco3hahue Jan 9, 2020

slintes Jan 9, 2020

cynepco3hahue Jan 9, 2020

slintes Jan 9, 2020

slintes Jan 9, 2020

ffromani Jan 10, 2020

davidvossel commented Jan 9, 2020

davidvossel commented Jan 9, 2020

davidvossel left a comment

davidvossel Jan 9, 2020

slintes Jan 10, 2020

davidvossel Jan 9, 2020

ffromani Jan 10, 2020 •

edited

Loading

slintes Jan 10, 2020

ffromani Jan 10, 2020

gsr-shanks Jan 10, 2020

slintes Jan 10, 2020

slintes Jan 10, 2020

slintes Jan 10, 2020

ffromani Jan 10, 2020

slintes Jan 10, 2020

slintes Jan 10, 2020

ffromani left a comment

davidvossel left a comment

openshift-ci-robot commented Jan 10, 2020

Install RT kernel from RPMs included in RHCOS #34

Install RT kernel from RPMs included in RHCOS #34

Conversation

slintes commented Jan 9, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvossel commented Jan 9, 2020

davidvossel commented Jan 9, 2020

davidvossel 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

ffromani Jan 10, 2020 • 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

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

ffromani left a comment

Choose a reason for hiding this comment

davidvossel left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 10, 2020

slintes commented Jan 9, 2020 •

edited

Loading

ffromani Jan 10, 2020 •

edited

Loading