Add Performance Profile Controler must-gather by marioferh · Pull Request #341 · openshift/must-gather

marioferh · 2022-12-09T11:52:36Z

Description

Launch Performance Profile Controller data collecting in OCP must-gather

Goal

Remove a standalone PAO must-gather image and make it part of the core OCP must-gather.

Before, the command for the client to run must-gather and PPC must-gather.

oc adm must-gather
--image-stream=openshift/must-gather \
--image=[...]/performance-addon-operator-must-gather-rhel8:$VERSION

Now:
oc adm must-gather --image=openshift/must-gather:$VERSION /usr/bin/gather_performance_profile_controller

Type of change

Add PPC scripts
Add two stage build to dockerfile in order to get sysinfo binary needed to collect data by scripts
Add PPC configuration yaml
Tests not included in this repo. Go to NTO-PPC repo.

Needed changes

Change temp ARG REPO=github.com/marioferh/gather-sysinfo to actual repo before merge.

PAO = Performance Addon Operator now is PPC = Performance Profile Controller after this change: openshift/cluster-node-tuning-operator#322

Reconciling with https://github.com/openshift/ocp-build-data/tree/b44f15ec9e84d1831eac81f8c757b3bed985dbeb/images/ose-must-gather.yml

…penshift-4.12-ose-must-gather Updating ose-must-gather images to be consistent with ART

Signed-off-by: Mario Fernandez <mariofer@redhat.com>

openshift-ci · 2022-12-09T11:52:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marioferh
Once this PR has been reviewed and has the lgtm label, please assign sferich888 for approval by writing /assign @sferich888 in a comment. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

sferich888

This looks great; however there is quite a bit I would like to change or have you consider.

sferich888 · 2022-12-09T13:01:52Z

performance-profile-node-gather/clusterrolebinding.yaml

+metadata:
+  name: perf-node-gather-pods-reader
+subjects:
+- kind: ServiceAccount


Isn't this something we provide in its own file?

I don't understand the comment :S

sferich888 · 2022-12-09T13:02:25Z

performance-profile-node-gather/clusterrolebinding.yaml

+  namespace: perf-node-gather
+  apiGroup: ""
+roleRef:
+  kind: ClusterRole


Isn't this something we are providing int its own file?

Dockerfile.rhel7

sferich888 · 2022-12-09T13:06:21Z

collection-scripts/gather_performance_profile_controller

+
+sed -i -e "s#MUST_GATHER_IMAGE#$MUST_GATHER_IMAGE#" $DAEMONSET_MANIFEST
+
+oc create -f $NAMESPACE_MANIFEST


Can we simply all of the oc create commands by using oc apply?

We need to do it with oc create to apply policies after the SA creation:
oc adm policy add-scc-to-user privileged -n $NAMESPACE -z perf-node-gather

collection-scripts/gather_performance_profile_controller

sferich888 · 2022-12-09T13:17:22Z

collection-scripts/gather_performance_profile_controller

+
+sed -i -e "s#MUST_GATHER_IMAGE#$MUST_GATHER_IMAGE#" $DAEMONSET_MANIFEST
+
+oc create -f $NAMESPACE_MANIFEST


Can we 'reuse' the exsisting must-gather namespace; instead of creating a new namespace?

By putting all the collection artifacts in the 'must-gather' namespace; when the oc command that invokes must-gather finishes; a clean up operation is done (removing all of the pods, and other artifacts that we create).

By spawning off new namespaces; it's possible that we could leave around lingering artifacts.

I am facing this issue if I try to reuse same namespace:
Warning FailedCreate 33s (x15 over 115s) daemonset-controller Error creating: pods "perf-node-gather-daemonset-" is forbidden: autoscaling.openshift.io/ManagementCPUsOverride the pod namespace "openshift-must-gather-f2s49" does not allow the workload type management

collection-scripts/gather_performance_profile_controller

sferich888 · 2022-12-09T13:20:32Z

collection-scripts/gather_performance_profile_controller

+done
+wait "${ADM_PIDS[@]}"
+
+oc delete -f $DAEMONSET_MANIFEST


I don't think we need to do this; if we rely on the invoking oc command to clean things up for us.

for everyone?

collection-scripts/gather_performance_profile_controller

marioferh

The new change create sysinfo binary inside the repo to not depend of external repo as before

soltysh · 2022-12-15T16:42:25Z

.ci-operator.yaml

  name: release
  namespace: openshift
-  tag: rhel-8-release-golang-1.18-openshift-4.12
+  tag: rhel-8-release-golang-1.19-openshift-4.12


rhel-8-release-golang-1.19-openshift-4.13 would be the correct one here and everywhere else too.

soltysh · 2022-12-15T16:43:17Z

Dockerfile.rhel7

-FROM registry.ci.openshift.org/ocp/builder:rhel-8-golang-1.18-openshift-4.12 AS builder
+FROM registry.ci.openshift.org/ocp/builder:rhel-8-golang-1.19-openshift-4.12 AS builder
+
+RUN dnf install -y git make go


Why you're doing this, all of those tools are already part of the builder image.

Dockerfile.rhel7

soltysh · 2022-12-15T16:44:35Z

Dockerfile.rhel7

+COPY ${NODE_GATHER_MANIFESTS_DIR} /etc/performance-profile-node-gather
+
+# the binary follows the must-gather convention of helper tools called gather_$SOMETHING
+COPY build/_output/bin/gather-sysinfo /usr/bin/gather_sysinfo


I don't quite understand the lines above, what are they doing?

After generate a binary with make we need to copy it to the image

soltysh · 2022-12-15T16:45:32Z

Makefile

+dist-gather-sysinfo: build-output-dir
+	@if [ ! -x $(TOOLS_BIN_DIR)/gather-sysinfo ]; then\
+		echo "Building gather-sysinfo helper";\
+		env CGO_ENABLED=0 GOOS=$(TARGET_GOOS) GOARCH=$(TARGET_GOARCH) go build -ldflags="-s -w" -mod=vendor -o $(TOOLS_BIN_DIR)/gather-sysinfo ./pkg/tools/gather-sysinfo/;\


Why you're building it? From the email thread it looked like you already have a package ready that you just install?

I do not think we have ever said that. We are currently building it as part of the separate must gather image we ship, but there never was an RPM as far as I know.

soltysh · 2022-12-15T16:46:18Z

collection-scripts/gather_performance_profile_controller

+    read desired ready <<< $line
+    IFS=$'\n'
+
+    if [[ "$desired" != "0" ]] && [[ "$ready" == "$desired" ]]


I second @sferich888 here, it's more reasonable to have some wait period before failing.

soltysh · 2022-12-15T16:47:40Z

collection-scripts/gather_performance_profile_controller

+oc adm policy add-scc-to-user privileged -n $NAMESPACE -z perf-node-gather
+oc create -f $CLUSTER_ROLE_MANIFEST
+oc create -f $CLUSTER_ROLE_BINDING_MANIFEST
+oc create -f $DAEMONSET_MANIFEST


You can't create any of these, b/c must-gather CLI won't be able to clean it up. You should rather re-use the namespace and the privileged SA created for must-gather pod.

soltysh · 2022-12-15T16:48:21Z

go.mod

 go 1.17

-require github.com/openshift/build-machinery-go v0.0.0-20210423112049-9415d7ebd33e
+require (


Like mentioned before, I don't think we want to handle it here, it should be built outside of this repo and only installed here.

That would require an RPM in the package repos. The tool is specific to the must gather data collection though. Also we want the tool to be always in sync with the MG and OCP versions. Building it here is actually much much easier and more reliable.

Based on what I was told it is already built as RPM so that shouldn't be the problem.

openshift-ci · 2022-12-16T11:15:17Z

@marioferh: 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/images	`6cbcd89`	link	true	`/test images`
ci/prow/e2e-aws	`6cbcd89`	link	true	`/test 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.

marioferh · 2022-12-22T16:12:18Z

openshift/oc#1316 oc PR to add required annotations to must-gather annotatoins

openshift-bot · 2023-03-23T01:00:53Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-merge-robot · 2023-03-23T01:01:08Z

PR needs rebase.

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.

sferich888 · 2023-04-19T18:52:32Z

/close

openshift-ci · 2023-04-19T18:52:44Z

@sferich888: Closed this PR.

Details

In response to this:

/close

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.

AOS Automation Release Team and others added 4 commits October 2, 2022 12:44

Updating ose-must-gather images to be consistent with ART

dbb0d6f

Reconciling with https://github.com/openshift/ocp-build-data/tree/b44f15ec9e84d1831eac81f8c757b3bed985dbeb/images/ose-must-gather.yml

Merge pull request openshift#330 from openshift-bot/art-consistency-o…

68c6a82

…penshift-4.12-ose-must-gather Updating ose-must-gather images to be consistent with ART

First draft merge PPC must gather 412

c574111

Signed-off-by: Mario Fernandez <mariofer@redhat.com>

add sysinfo deps

6cbcd89

Signed-off-by: Mario Fernandez <mariofer@redhat.com>

openshift-ci bot requested review from davemulford and sferich888 December 9, 2022 11:52

marioferh force-pushed the merge_ppc_mustgather_two_stages branch from f99984e to cdb4c64 Compare December 9, 2022 11:55

sferich888 suggested changes Dec 9, 2022

View reviewed changes

marioferh force-pushed the merge_ppc_mustgather_two_stages branch from cdb4c64 to e69b638 Compare December 14, 2022 13:12

marioferh commented Dec 14, 2022

View reviewed changes

marioferh force-pushed the merge_ppc_mustgather_two_stages branch 4 times, most recently from 7f46e5b to 9533b74 Compare December 15, 2022 15:10

soltysh suggested changes Dec 15, 2022

View reviewed changes

marioferh force-pushed the merge_ppc_mustgather_two_stages branch from 9533b74 to 6cbcd89 Compare December 16, 2022 11:04

marioferh mentioned this pull request Dec 22, 2022

Add must gather namespace annotation needed for performance profile controller openshift/oc#1316

Closed

MarSik mentioned this pull request Jan 6, 2023

Collect information relevant to PerformanceProfile and low latency tuning #345

Merged

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 23, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2023

openshift-ci bot closed this Apr 19, 2023


		sed -i -e "s#MUST_GATHER_IMAGE#$MUST_GATHER_IMAGE#" $DAEMONSET_MANIFEST

		oc create -f $NAMESPACE_MANIFEST

Comments

Conversation

marioferh commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Goal

Type of change

Needed changes

Uh oh!

openshift-ci bot commented Dec 9, 2022

Uh oh!

sferich888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marioferh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Dec 16, 2022

Uh oh!

marioferh commented Dec 22, 2022

Uh oh!

openshift-bot commented Mar 23, 2023

Uh oh!

openshift-merge-robot commented Mar 23, 2023

Uh oh!

sferich888 commented Apr 19, 2023

Uh oh!

openshift-ci bot commented Apr 19, 2023

Uh oh!

Reviewers

marioferh commented Dec 9, 2022 •

edited

Loading