e2e: mustgather: add missing deployer initialization #1115

shajmakh · 2024-12-13T10:37:54Z

Without the deployer initialization, deployment.deployer is nil and causes the test to panic. Fix that by calling deploy.NewForPlatform().

shajmakh · 2024-12-13T10:38:12Z

/cherry-pick release-4.18

openshift-cherrypick-robot · 2024-12-13T10:38:15Z

@shajmakh: once the present PR merges, I will cherry-pick it on top of release-4.18 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.18

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.

ffromani · 2024-12-13T11:06:24Z

test/e2e/must-gather/must_gather_suite_test.go

@@ -71,6 +71,7 @@ var _ = ginkgo.BeforeSuite(func() {
 		return
 	}
 	ginkgo.By("Setting up the cluster")
+	deployment.Deployer = deploy.NewForPlatform(configuration.Plat)


thanks, this is a nice catch. I think I like more a fix like

$ git diff diff --git a/test/e2e/must-gather/must_gather_suite_test.go b/test/e2e/must-gather/must_gather_suite_test.go index 838cdc42..2da25052 100644 --- a/test/e2e/must-gather/must_gather_suite_test.go +++ b/test/e2e/must-gather/must_gather_suite_test.go @@ -62,12 +62,12 @@ var _ = ginkgo.BeforeSuite(func() { mustGatherTag = getStringValueFromEnv(envVarMustGatherTag, defaultMustGatherTag) ginkgo.By(fmt.Sprintf("Using must-gather image %q tag %q", mustGatherImage, mustGatherTag)) - if _, ok := os.LookupEnv("E2E_NROP_INFRA_SETUP_SKIP"); ok { - ginkgo.By("Fetching up cluster data") + var err error + deployment, err = deploy.GetDeploymentWithSched(context.TODO()) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) - var err error - deployment, err = deploy.GetDeploymentWithSched(context.TODO()) - gomega.Expect(err).ToNot(gomega.HaveOccurred()) + if _, ok := os.LookupEnv("E2E_NROP_INFRA_SETUP_SKIP"); ok { + ginkgo.Expect(deployment.NroSchedObj).ToNot(gomega.BeNil(), "infra setup skip but Scheduler instance not found") return } ginkgo.By("Setting up the cluster")

Do you think the above could work?

nice idea, thanks for raising! I updated GetDeploymentWithSched() a bit to initialize the deployer so even if setup is skipped it won't hurt. let me know if you meant something else please

Without the deployer initialization, deployment.deployer is nil and causes the test to panic. Fix that by calling deploy.NewForPlatform(). Signed-off-by: Shereen Haj <[email protected]>

openshift-ci · 2024-12-13T14:49:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shajmakh

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:

~~test/OWNERS~~ [shajmakh]

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

ffromani

your approach can work! let's polish it up

ffromani · 2024-12-13T15:20:49Z

test/utils/deploy/deploy.go

@@ -71,7 +71,10 @@ func NewForPlatform(plat platform.Platform) Deployer {
 }

 func GetDeploymentWithSched(ctx context.Context) (NroDeploymentWithSched, error) {
-	sd := NroDeploymentWithSched{}
+	sd := NroDeploymentWithSched{
+		Deployer: NewForPlatform(configuration.Plat),


this seems correct

we need to set it first thing in case the cluster didn't have the scheduler and fails to get it

ffromani · 2024-12-13T15:20:59Z

test/e2e/must-gather/must_gather_suite_test.go

@@ -62,12 +62,13 @@ var _ = ginkgo.BeforeSuite(func() {
 	mustGatherTag = getStringValueFromEnv(envVarMustGatherTag, defaultMustGatherTag)
 	ginkgo.By(fmt.Sprintf("Using must-gather image %q tag %q", mustGatherImage, mustGatherTag))

+	ginkgo.By("Fetching up cluster data")


this seems wrong

indeed. I'll make it "collect available data" does that sound better?

ffromani · 2024-12-13T15:21:22Z

test/e2e/must-gather/must_gather_suite_test.go

+	ginkgo.By("Fetching up cluster data")
+	var err error
+	// the error might be not nil we'll decide if that's fine or not depending on E2E_NROP_INFRA_SETUP_SKIP
+	deployment, err = deploy.GetDeploymentWithSched(context.TODO())


this should never fail anyway so let's check err not occurred

it can fail in case the cluster doesn't have the scheduler and we try to fetch it:
https://github.com/openshift-kni/numaresources-operator/blob/main/test/utils/deploy/deploy.go#L79

I think the question is why the sudden change? why are we always running the deployment, even if E2E_NROP_INFRA_SETUP_SKIP exists.

my take is that we always want to have an instance of the current scheduler object. If the scheduler does already exist (skip setup), then we should just fetch the existing instance and move on; otherwise we need to setup it and then fetch the setupped instance. This is probably not too evident from the code and we likely need a bit larger refactoring.

ffromani · 2024-12-16T07:14:23Z

/retest

ffromani · 2024-12-16T07:56:43Z

test/e2e/must-gather/must_gather_suite_test.go

-		var err error
-		deployment, err = deploy.GetDeploymentWithSched(context.TODO())
-		gomega.Expect(err).ToNot(gomega.HaveOccurred())
+		gomega.Expect(err).ToNot(gomega.HaveOccurred(), "infra setup skip but Scheduler instance not found")


doesn't make sense to conditionally check the error. If we need to do GetDeploymentWithSched it must not fail.

ffromani · 2024-12-17T09:40:48Z

/close

we merged #1120

but kudos to find and fix the issue here. #1120 takes a step further and was merged only because handoff of PRs.

shajmakh · 2024-12-17T10:52:31Z

/close

we merged #1120

but kudos to find and fix the issue here. #1120 takes a step further and was merged only because handoff of PRs.

Thanks for handling this. much appreciated!

openshift-ci bot requested review from mrniranjan and Tal-or December 13, 2024 10:38

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2024

shajmakh mentioned this pull request Dec 13, 2024

kni: nrop: kickoff 4.19 branch openshift/release#59761

Closed

ffromani reviewed Dec 13, 2024

View reviewed changes

shajmakh mentioned this pull request Dec 13, 2024

kni: nrop: run must-gather CI always openshift/release#59867

Open

shajmakh force-pushed the mg-deployer-fix branch 2 times, most recently from 173bd39 to 432bf77 Compare December 13, 2024 14:44

e2e: mustgather: add missing deployer initialization

9cd43f1

Without the deployer initialization, deployment.deployer is nil and causes the test to panic. Fix that by calling deploy.NewForPlatform(). Signed-off-by: Shereen Haj <[email protected]>

shajmakh force-pushed the mg-deployer-fix branch from 432bf77 to 9cd43f1 Compare December 13, 2024 14:49

ffromani reviewed Dec 13, 2024

View reviewed changes

ffromani reviewed Dec 16, 2024

View reviewed changes

ffromani mentioned this pull request Dec 16, 2024

e2e: must-gather: fix the setup process #1120

Merged

ffromani closed this Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: mustgather: add missing deployer initialization #1115

e2e: mustgather: add missing deployer initialization #1115

shajmakh commented Dec 13, 2024

shajmakh commented Dec 13, 2024

openshift-cherrypick-robot commented Dec 13, 2024

ffromani Dec 13, 2024

shajmakh Dec 13, 2024

openshift-ci bot commented Dec 13, 2024

ffromani left a comment

ffromani Dec 13, 2024

shajmakh Dec 14, 2024

ffromani Dec 13, 2024

shajmakh Dec 14, 2024

ffromani Dec 13, 2024

shajmakh Dec 14, 2024 •

edited

Loading

Tal-or Dec 15, 2024

ffromani Dec 16, 2024

ffromani commented Dec 16, 2024

ffromani Dec 16, 2024

ffromani commented Dec 17, 2024

shajmakh commented Dec 17, 2024

e2e: mustgather: add missing deployer initialization #1115

e2e: mustgather: add missing deployer initialization #1115

Conversation

shajmakh commented Dec 13, 2024

shajmakh commented Dec 13, 2024

openshift-cherrypick-robot commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 13, 2024

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

shajmakh Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffromani commented Dec 16, 2024

Choose a reason for hiding this comment

ffromani commented Dec 17, 2024

shajmakh commented Dec 17, 2024

shajmakh Dec 14, 2024 •

edited

Loading