Check if workerGroupSpec is not empty before accessing it #549

ChristianZaccaria · 2024-05-08T15:23:52Z

Issue link

Jira: https://issues.redhat.com/browse/RHOAIENG-6614

What changes have been made

The failing Ray Tests were caused by an index out of range error in the webhook logic. The workerGroupSpec was attempted to be accessed when it doesn't exist.
Check is now made to verify it's not empty before accessing it.

Verification steps

Have all components deployed including this version of the CFO and Kueue.
Deploy KubeRay-Operator in the same namespace as referenced in the DSCI default-dsci
In the KubeRay repo, run the previously failing test:
go test -timeout 30m -v ./test/e2e/rayjob_lightweight_test.go ./test/e2e/support.go
Test should pass.

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- Testing is not required for this change

pkg/controllers/raycluster_webhook.go

sutaakar · 2024-05-09T06:33:44Z

@ChristianZaccaria Can you please provide a unit test verifying the changes?

ChristianZaccaria · 2024-05-10T11:25:29Z

@sutaakar I added unit tests covering these changes. Please let me know if there are other test cases I should be covering here.

Also, I am thinking of adding a RayCluster template in the test support file and perhaps append items to it in the tests to reduce the amount of code in the test file. Is this a good approach or what is the best practices on this? Thank you! :)

pkg/controllers/raycluster_webhook_test.go

sutaakar · 2024-05-10T11:43:42Z

@ChristianZaccaria Creating one valid RayCluster CR and then modify it is a possible approach, may increase test readability.

pkg/controllers/raycluster_webhook_test_support.go

ChristianZaccaria · 2024-05-10T16:41:40Z

pkg/controllers/raycluster_webhook_test.go

+	warnings, err := rcWebhook.ValidateCreate(test.Ctx(), runtime.Object(validRayCluster))
+	t.Run("Expected no warnings or errors on call to ValidateCreate function", func(t *testing.T) {
+		g := gomega.NewWithT(t)
+		g.Expect(warnings).To(gomega.BeNil())
+		g.Expect(err).To(gomega.BeNil())
+	})


@sutaakar I only then realised that this basically covers all test cases as it will fail if the elements to be validated are not in the defined RayCluster in the test. Perhaps the tests after this one are not needed?

Also, If I do the same for the Default function, would all the other tests/assertions not be required too?

Open to thoughts on this before continuing, thanks!

Thinking perhaps the assertions made for the Default (mutating) function is required to compare outputs.

For the validating functions such as ValidateCreate, as long as no errors are returned (meaning all elements exist in the defined RayCluster), the validation should succeed. - Making any other assertion tests not necessary. Correct me if I'm wrong

For ValidateCreate and ValidateUpdate it is enough to have one positive test, providing negative tests to validate incorrect modification and its detection.

Or possibly to have several tests, providing valid RayClusters, i.e. more elements in workerGroupSpecs and such.

Hopefully, latest changes in this PR reflects the above. - One positive test case, providing negative tests for manipulated fields. Thanks!

sutaakar · 2024-05-13T07:36:28Z

pkg/controllers/raycluster_webhook_test.go

+	t.Run("Expected no errors on call to Default function", func(t *testing.T) {
+		g := gomega.NewWithT(t)
+		g.Expect(err).To(gomega.BeNil())
+	})
+
+	t.Run("Expected required OAuth proxy container for the head group", func(t *testing.T) {
+		g := gomega.NewWithT(t)
+		g.Expect(func() bool {
+			for _, container := range validRayCluster.Spec.HeadGroupSpec.Template.Spec.Containers {
+				if container.Name == oauthProxyContainerName {
+					return true
+				}
+			}
+			return false
+		}()).To(gomega.BeTrue(), "Expected the OAuth proxy container to be present in the list of head group containers")
+	})


This boilerplate isn't needed, you can leverage test directly as it embeds gomega:

test.Expect(err).NotTo(HaveOccurred(), "Expected no errors on call to Default function") test.Expect(validRayCluster.Spec.HeadGroupSpec.Template.Spec.Containers). To(ContainElement(WithTransform(ContainerName, Equal(oauthProxyContainerName))), "Expected the OAuth proxy container to be present in the list of head group containers")

With helper functions in the bottom (to be relocated into codeflare-common):

func ContainerName(container corev1.Container) string { return container.Name }

Also this approach avoids BeTrue() checks for element with specific value, which often don't provide meaningful context.

Test can also check number of elements, making the assertion more structured:

test.Expect(validRayCluster.Spec.HeadGroupSpec.Template.Spec.Containers). To( And( HaveLen(1), ContainElement(WithTransform(ContainerName, Equal(oauthProxyContainerName))), ), "Expected the OAuth proxy container to be present in the list of head group containers", )

That was very enlightening, thanks Karel for sharing such great expertise!

ChristianZaccaria · 2024-05-20T14:02:38Z

@sutaakar latest changes include one positive test for each "main" function, and several negative tests to cover the full functionality of the raycluster_webhook.go file.

sutaakar · 2024-05-21T06:45:51Z

pkg/controllers/raycluster_webhook_test.go

+	invalidRayCluster := validRayCluster.DeepCopy()
+
+	t.Run("Negative: Expected errors on call to ValidateUpdate function due to EnableIngress set to True", func(t *testing.T) {
+		invalidRayCluster.Spec.HeadGroupSpec.EnableIngress = &trueBool


Nitpick, there is a dedicate funtion to create pointer for values:

Suggested change

invalidRayCluster.Spec.HeadGroupSpec.EnableIngress = &trueBool

invalidRayCluster.Spec.HeadGroupSpec.EnableIngress = support.Ptr(true)

sutaakar · 2024-05-21T06:48:34Z

@ChristianZaccaria Can you also adjust test-unit Makefile target to run the new unit test (currently it doesn't run anything). That way the unit test will be executed for PRs.

Otherwise this PR looks great, good job.

ChristianZaccaria · 2024-05-21T08:50:32Z

@sutaakar I made the last few changes, tests are now running as part of this PR check in the Unit tests workflow. Thank you!

ChristianZaccaria · 2024-05-21T08:52:03Z

I made a PR to move the core functions used here to the codeflare-common repository. Should we merge that and then make the changes here or what is the appropriate approach? Thanks @sutaakar

sutaakar · 2024-05-21T09:52:39Z

/lgtm

I made a project-codeflare/codeflare-common#48 used here to the codeflare-common repository. Should we merge that and then make the changes here or what is the appropriate approach?

That is up to you, can be merged now and refactored later or it can wait.

…lity

sutaakar · 2024-05-28T08:34:13Z

/lgtm

sutaakar · 2024-05-29T10:06:28Z

/approve

openshift-ci · 2024-05-29T10:06:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sutaakar

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

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

ChristianZaccaria requested a review from sutaakar May 8, 2024 15:23

openshift-ci bot requested review from Fiona-Waters and KPostOffice May 8, 2024 15:23

sutaakar reviewed May 9, 2024

View reviewed changes

pkg/controllers/raycluster_webhook.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label May 9, 2024

sutaakar reviewed May 10, 2024

View reviewed changes

pkg/controllers/raycluster_webhook_test.go Outdated Show resolved Hide resolved

sutaakar reviewed May 10, 2024

View reviewed changes

pkg/controllers/raycluster_webhook_test_support.go Outdated Show resolved Hide resolved

ChristianZaccaria force-pushed the webhook-index-range branch from 258d3a4 to 89d0450 Compare May 10, 2024 16:39

openshift-merge-robot removed the needs-rebase label May 10, 2024

ChristianZaccaria commented May 10, 2024

View reviewed changes

ChristianZaccaria force-pushed the webhook-index-range branch 2 times, most recently from b80e2bd to ecafab2 Compare May 10, 2024 17:35

sutaakar reviewed May 13, 2024

View reviewed changes

ChristianZaccaria requested a review from sutaakar May 20, 2024 14:50

sutaakar reviewed May 21, 2024

View reviewed changes

ChristianZaccaria force-pushed the webhook-index-range branch from 6423b3b to 21ef9ac Compare May 21, 2024 08:23

ChristianZaccaria mentioned this pull request May 21, 2024

Add additional core functions project-codeflare/codeflare-common#48

Merged

4 tasks

ChristianZaccaria requested a review from sutaakar May 21, 2024 08:52

openshift-ci bot assigned sutaakar May 21, 2024

openshift-ci bot added the lgtm label May 21, 2024

ChristianZaccaria added 2 commits May 27, 2024 10:40

Check if workerGroupSpec is not empty before accessing it

9fe55ad

Add positive and negative test cases for RayCluster Webhook functiona…

730ef14

…lity

Run controller tests with test-unit make target

3b89d26

ChristianZaccaria force-pushed the webhook-index-range branch from 21ef9ac to f4bb394 Compare May 27, 2024 09:40

openshift-ci bot removed the lgtm label May 27, 2024

Use ResourceName generic function for raycluster_webhook tests

954fa07

ChristianZaccaria force-pushed the webhook-index-range branch from f4bb394 to 954fa07 Compare May 28, 2024 08:02

openshift-ci bot added the lgtm label May 28, 2024

openshift-ci bot added the approved label May 29, 2024

openshift-merge-bot bot merged commit 4ba15ae into project-codeflare:main May 29, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if workerGroupSpec is not empty before accessing it #549

Check if workerGroupSpec is not empty before accessing it #549

ChristianZaccaria commented May 8, 2024

sutaakar commented May 9, 2024

ChristianZaccaria commented May 10, 2024

sutaakar commented May 10, 2024

ChristianZaccaria May 10, 2024 •

edited

Loading

ChristianZaccaria May 10, 2024 •

edited

Loading

sutaakar May 13, 2024 •

edited

Loading

sutaakar May 13, 2024 •

edited

Loading

ChristianZaccaria May 20, 2024

sutaakar May 13, 2024

sutaakar May 13, 2024 •

edited

Loading

sutaakar May 13, 2024

ChristianZaccaria May 20, 2024

ChristianZaccaria commented May 20, 2024

sutaakar May 21, 2024

sutaakar commented May 21, 2024

ChristianZaccaria commented May 21, 2024

ChristianZaccaria commented May 21, 2024

sutaakar commented May 21, 2024

sutaakar commented May 28, 2024

sutaakar commented May 29, 2024

openshift-ci bot commented May 29, 2024

	invalidRayCluster.Spec.HeadGroupSpec.EnableIngress = &trueBool
	invalidRayCluster.Spec.HeadGroupSpec.EnableIngress = support.Ptr(true)

Check if workerGroupSpec is not empty before accessing it #549

Check if workerGroupSpec is not empty before accessing it #549

Conversation

ChristianZaccaria commented May 8, 2024

Issue link

What changes have been made

Verification steps

Checks

sutaakar commented May 9, 2024

ChristianZaccaria commented May 10, 2024

sutaakar commented May 10, 2024

ChristianZaccaria May 10, 2024 • edited Loading

Choose a reason for hiding this comment

ChristianZaccaria May 10, 2024 • edited Loading

Choose a reason for hiding this comment

sutaakar May 13, 2024 • edited Loading

Choose a reason for hiding this comment

sutaakar May 13, 2024 • edited Loading

Choose a reason for hiding this comment

ChristianZaccaria May 20, 2024

Choose a reason for hiding this comment

sutaakar May 13, 2024

Choose a reason for hiding this comment

sutaakar May 13, 2024 • edited Loading

Choose a reason for hiding this comment

sutaakar May 13, 2024

Choose a reason for hiding this comment

ChristianZaccaria May 20, 2024

Choose a reason for hiding this comment

ChristianZaccaria commented May 20, 2024

sutaakar May 21, 2024

Choose a reason for hiding this comment

sutaakar commented May 21, 2024

ChristianZaccaria commented May 21, 2024

ChristianZaccaria commented May 21, 2024

sutaakar commented May 21, 2024

sutaakar commented May 28, 2024

sutaakar commented May 29, 2024

openshift-ci bot commented May 29, 2024

ChristianZaccaria May 10, 2024 •

edited

Loading

ChristianZaccaria May 10, 2024 •

edited

Loading

sutaakar May 13, 2024 •

edited

Loading

sutaakar May 13, 2024 •

edited

Loading

sutaakar May 13, 2024 •

edited

Loading