Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ValidatingAdmissionPolicy tests for RayClusters and AppWrappers #300

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChristianZaccaria
Copy link
Contributor

@ChristianZaccaria ChristianZaccaria commented Jan 6, 2025

Description

Part of: https://issues.redhat.com/browse/RHOAIENG-14731

How Has This Been Tested?

For context, the VAP-B resources will be applied by the RHOAI Operator based on the OCP version it is installed on.

In the meantime, this can be tested by manually applying the resources:

  1. Ensure OCP cluster is >=v4.17
  2. oc apply both resources from this directory
  3. Ensure Kueue, CodeFlare-Operator(and AppWrapper controller), and KubeRay are installed.
  4. Run go test -timeout 1m ./tests/odh/validating_admission_policy_test.go ./tests/odh/support.go.

Additional Thoughts:

  • This test must run on an OCP v4.17 or later cluster. Will discuss with @jiripetrlik to find best way to achieve this.
  • Taking into account, eventually the rhoai-operator will be the one applying the VAP-B resources. We could apply them ourselves in the meantime for the tests.
  • There are 2 sleeps in the tests when updating the VAP resource to allow sufficient time for changes to take effect. Thinking of a better way.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from Bobbins228 and szaher January 6, 2025 12:38
Copy link

openshift-ci bot commented Jan 6, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sutaakar for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

vapb.Spec.PolicyName = "none"
_, err = test.Client().Core().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Update(test.Ctx(), vapb, metav1.UpdateOptions{})
test.Expect(err).ToNot(HaveOccurred())
time.Sleep(2 * time.Second) // Wait for the ValidatingAdmissionPolicyBinding to be updated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use test.Eventually here and get the VAP and check for an expected updated value

Copy link
Contributor Author

@ChristianZaccaria ChristianZaccaria Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given this a try just now, with the debugger I can see it does find the resource to be updated with the updated value, but the tests that follow fail as if the resource had not yet been updated. Perhaps, k8s needs time to propagate these changes somewhere else too? - It seems to need an initial couple of seconds after updating the resource.

Copy link
Contributor Author

@ChristianZaccaria ChristianZaccaria Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried implementing:

test.Eventually(func() bool {
  vapb, _ = test.Client().Core().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Get(test.Ctx(), vapb.Name, metav1.GetOptions{})
  return vapb.Spec.PolicyName == "none"
}, 5*time.Second, 2*time.Second).Should(BeTrue())

The issue found is that the 'retry' never gets executed as the updated value is found immediately to be true:
}, 5*time.Second, 2*time.Second).Should(BeTrue())

Using a sleep is providing consistent results. - Given it's a test, perhaps this is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, @Bobbins228 and I looked for a status in both resources in case there was any other indicative of propagation readiness. - No indication found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly needs to propagated?

go.mod Show resolved Hide resolved
Copy link
Contributor

@Bobbins228 Bobbins228 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried out these tests a couple of times and the testing examples look sound.
I am not too sure how we can confirm that the VAP updates correctly rather than using sleep but on the testing side /lgtm

Comment on lines +222 to +223
_, err = test.Client().Core().AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Update(test.Ctx(), vapb, metav1.UpdateOptions{})
test.Expect(err).ToNot(HaveOccurred())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bobbins228 thanks for taking the time to test Mark!

Tried out these tests a couple of times and the testing examples look sound.
I am not too sure how we can confirm that the VAP updates correctly rather than using sleep but on the testing side /lgtm

These lines ensure that the update happens without errors. Eventhough we ensure the update does happen, the test runs so quickly that subsequent tests fail unless we allow a couple of seconds for it to 'settle'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants