PAO must-gather e2e test #365

marioferh · 2022-05-19T08:51:39Z

must-gather e2e test

Add e2e must-gather test with pre-collected data.
Untar sysinfo.tgz
Check if generated files are present

Test in local image

openshift-ci · 2022-05-19T12:02:15Z

@marioferh: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test e2e-aws
/test e2e-aws-operator
/test e2e-gcp-pao
/test e2e-no-cluster
/test e2e-upgrade
/test images
/test unit
/test verify

Use /test all to run all jobs.

In response to this:

/retest e2e-no-cluster

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.

marioferh · 2022-06-08T08:27:22Z

/retest-required

ffromani

do we want to add the expanded archive, which is a pretty large tree, or do we want to add the must gather archive as a tgz, so as binary blob?

Additionally, please clarify the (very terse) "Test in local image" line

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go

ffromani · 2022-06-13T08:08:40Z

test/e2e/performanceprofile/functests/6_mustgather_testing/test_suite_mustgather_test.go

+var _ = BeforeSuite(func() {
+	Expect(testclient.ClientsEnabled).To(BeTrue(), "package client not enabled")
+	// create test namespace
+	err := testclient.Client.Create(context.TODO(), namespaces.TestingNamespace)


why do we need a testing namespace?

It's copied from other suites, not sure if needed.

do we have a conclusion? it seems to me the namespace is not really needed here

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go

test/e2e/performanceprofile/functests/6_mustgather_testing/test_suite_mustgather_test.go

yanirq · 2022-06-15T08:59:04Z

The test data folders are composed from long generated paths, maybe we can downsize that for better navigation if we can.
Also, will this test data always be valid for the tests ?

marioferh · 2022-06-15T10:58:21Z

The test data folders are composed from long generated paths, maybe we can downsize that for better navigation if we can. Also, will this test data always be valid for the tests ?

this paths are generated from must-gather

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go

test/e2e/performanceprofile/functests/6_mustgather_testing/test_suite_mustgather_test.go

ffromani

@jmencak Hi Jiri, we want to test must-gather. We add a bunch (quite a lot) of files here as test data. I'm not sure we want to add so many. The alternative would be to add a tar/tgz blob and unpack it when test runs. Any preference/guidance? thanks!

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go

ffromani · 2022-06-17T07:08:10Z

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go

+			Expect(err).ToNot(HaveOccurred(), "failed to read sysinfo.tgz file %s: %v", snapshotName, err)
+		}
+
+		err = Untar(snapshotDir, snapshotName)


how do we do cleanup?

how was this addressed?

here,

cluster-node-tuning-operator/test/e2e/performanceprofile/functests/6_mustgather_testing/test_suite_mustgather_test.go

Line 40 in 7e46efa

snapshotProc := filepath.Join(snapshotDir, "proc")

maybe it could be better but I think is enough for this PR

I think it's fine to do this in the aftersuite. It would be better to untar into a randomly generated temporary folder (https://pkg.go.dev/os#TempDir) and remove the temporary directory straight away when the suite ends.
I think this is a further step we should do for safety and robustness (and to minimize the risk we clog the disk of the CI boxes).
In the future we can move all the snapshot handling at the same level - make temp dir and untar in beforesuite, clean up (just remove the temp dir and all its content) on aftersuite

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go

ffromani · 2022-06-17T07:09:58Z

test/e2e/performanceprofile/functests/6_mustgather_testing/test_suite_mustgather_test.go

+var _ = BeforeSuite(func() {
+	Expect(testclient.ClientsEnabled).To(BeTrue(), "package client not enabled")
+	// create test namespace
+	err := testclient.Client.Create(context.TODO(), namespaces.TestingNamespace)


do we have a conclusion? it seems to me the namespace is not really needed here

marioferh · 2022-06-17T09:26:38Z

@jmencak Hi Jiri, we want to test must-gather. We add a bunch (quite a lot) of files here as test data. I'm not sure we want to add so many. The alternative would be to add a tar/tgz blob and unpack it when test runs. Any preference/guidance? thanks!

I don't understand, I have deleted the files and uploaded sysinfo.tgz. This one is a previous PR to the next one to collect the data from the cluster. Then no files will be uploaded to test/data

marioferh · 2022-06-17T09:27:55Z

`fromanirh 2 hours ago

do we have a conclusion? it seems to me the namespace is not really needed here`

Yes, its like that in other suites, if we need to delete, it should be done in another PR in all suites

ffromani · 2022-06-17T09:50:47Z

`fromanirh 2 hours ago

do we have a conclusion? it seems to me the namespace is not really needed here`

Yes, its like that in other suites, if we need to delete, it should be done in another PR in all suites

ok, let's keep it.

Add e2e must-gather test with pre-collected data. Untar sysinfo.tgz Check if generated files are present Signed-off-by: Mario Fernandez <mariofer@redhat.com>

marioferh · 2022-06-28T20:58:50Z

/retest-required

ffromani

from my pov, I think we need to do a step further in how we handle the untarred snapshot, and than we're good to go. Everything else seems OK or better.

ffromani · 2022-06-29T08:24:23Z

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go

+			Expect(err).ToNot(HaveOccurred(), "failed to read sysinfo.tgz file %s: %v", snapshotName, err)
+		}
+
+		err = Untar(snapshotDir, snapshotName)


I think it's fine to do this in the aftersuite. It would be better to untar into a randomly generated temporary folder (https://pkg.go.dev/os#TempDir) and remove the temporary directory straight away when the suite ends.
I think this is a further step we should do for safety and robustness (and to minimize the risk we clog the disk of the CI boxes).
In the future we can move all the snapshot handling at the same level - make temp dir and untar in beforesuite, clean up (just remove the temp dir and all its content) on aftersuite

marioferh · 2022-06-29T11:38:10Z

@[fromanirh]

(https://github.com/fromanirh) fromanirh [3 hours ago](https://github.com/openshift/cluster-node-tuning-operator/pull/365#discussion_r909343782)

I think it's fine to do this in the aftersuite. It would be better to untar into a randomly generated temporary folder (https://pkg.go.dev/os#TempDir) and remove the temporary directory straight away when the suite ends.
I think this is a further step we should do for safety and robustness (and to minimize the risk we clog the disk of the CI boxes).
In the future we can move all the snapshot handling at the same level -

I did it like that at the very begging as in debug-tools, the issue here is we have hardoded data besides the tar file in a no tmp folder. If we untar the data in a tmp dir we have the data split in two different folders, and copy the there and the delete it is not so elegant.

For that I've changed to this way, and in the next PR when data is generated in the cluster untar all in same folder and then delete all.

ffromani · 2022-06-29T11:41:39Z

ack, let's start this way then

ffromani · 2022-06-29T11:41:44Z

/lgtm

ffromani · 2022-06-29T11:41:50Z

/retest

marioferh · 2022-07-11T08:59:32Z

@fromanirh PTAL please

ffromani · 2022-07-11T09:21:56Z

/approve

openshift-ci · 2022-07-11T09:24:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fromanirh, marioferh

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

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

openshift-ci · 2022-07-11T11:49:24Z

@marioferh: 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/test-infra repository. I understand the commands that are listed here.

Add e2e must-gather test with pre-collected data. Untar sysinfo.tgz Check if generated files are present Signed-off-by: Mario Fernandez <mariofer@redhat.com>

openshift-ci bot requested review from kpouget and yanirq May 19, 2022 08:52

marioferh mentioned this pull request Jun 6, 2022

PAO: update README.txt #369

Merged

ffromani reviewed Jun 13, 2022

View reviewed changes

yanirq reviewed Jun 15, 2022

View reviewed changes

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go Outdated Show resolved Hide resolved

yanirq reviewed Jun 15, 2022

View reviewed changes

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go Outdated Show resolved Hide resolved

yanirq reviewed Jun 15, 2022

View reviewed changes

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go Outdated Show resolved Hide resolved

yanirq reviewed Jun 15, 2022

View reviewed changes

test/e2e/performanceprofile/functests/6_mustgather_testing/mustgather.go Show resolved Hide resolved

yanirq reviewed Jun 15, 2022

View reviewed changes

test/e2e/performanceprofile/functests/6_mustgather_testing/test_suite_mustgather_test.go Show resolved Hide resolved

marioferh mentioned this pull request Jun 15, 2022

must-gather e2e test with cluster collected data #375

Closed

ffromani reviewed Jun 15, 2022

View reviewed changes

ffromani reviewed Jun 17, 2022

View reviewed changes

must-gather e2e test

84bbf61

Add e2e must-gather test with pre-collected data. Untar sysinfo.tgz Check if generated files are present Signed-off-by: Mario Fernandez <mariofer@redhat.com>

ffromani reviewed Jun 29, 2022

View reviewed changes

openshift-ci bot assigned ffromani Jun 29, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2022

openshift-ci bot merged commit 86c37b0 into openshift:master Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PAO must-gather e2e test #365

PAO must-gather e2e test #365

marioferh commented May 19, 2022 •

edited

openshift-ci bot commented May 19, 2022

marioferh commented Jun 8, 2022

ffromani left a comment

ffromani Jun 13, 2022

marioferh Jun 14, 2022

ffromani Jun 17, 2022

yanirq commented Jun 15, 2022

marioferh commented Jun 15, 2022

ffromani left a comment

ffromani Jun 17, 2022

marioferh Jun 17, 2022

ffromani Jun 20, 2022

marioferh Jun 27, 2022

ffromani Jun 29, 2022

ffromani Jun 17, 2022

marioferh commented Jun 17, 2022 •

edited

marioferh commented Jun 17, 2022

ffromani commented Jun 17, 2022

marioferh commented Jun 28, 2022

ffromani left a comment

ffromani Jun 29, 2022

marioferh commented Jun 29, 2022 •

edited

ffromani commented Jun 29, 2022

ffromani commented Jun 29, 2022

ffromani commented Jun 29, 2022

marioferh commented Jul 11, 2022

ffromani commented Jul 11, 2022

openshift-ci bot commented Jul 11, 2022

openshift-ci bot commented Jul 11, 2022

PAO must-gather e2e test #365

PAO must-gather e2e test #365

Conversation

marioferh commented May 19, 2022 • edited

openshift-ci bot commented May 19, 2022

marioferh commented Jun 8, 2022

ffromani 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

yanirq commented Jun 15, 2022

marioferh commented Jun 15, 2022

ffromani 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

marioferh commented Jun 17, 2022 • edited

marioferh commented Jun 17, 2022

ffromani commented Jun 17, 2022

marioferh commented Jun 28, 2022

ffromani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioferh commented Jun 29, 2022 • edited

ffromani commented Jun 29, 2022

ffromani commented Jun 29, 2022

ffromani commented Jun 29, 2022

marioferh commented Jul 11, 2022

ffromani commented Jul 11, 2022

openshift-ci bot commented Jul 11, 2022

openshift-ci bot commented Jul 11, 2022

marioferh commented May 19, 2022 •

edited

marioferh commented Jun 17, 2022 •

edited

marioferh commented Jun 29, 2022 •

edited