Add tests for ignoring scheduler processing #121783

atwamahmoud · 2023-11-07T18:53:17Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Adds 3 E2E tests to verify behaviour of
Bypassing scheduler processing

The tests are labeled with ClusterScaleUpBypassScheduler feature so they can be easily ignored/focused since without enabling the feature described in
Bypassing scheduler processing the tests will fail.

The tests use a scheduler name of non-existing-bypassed-scheduler.

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2023-11-07T18:53:36Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2023-11-07T18:53:36Z

Hi @atwamahmoud. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

BigDarkClown · 2023-11-29T16:57:02Z

Have you confirmed that these tests are running correctly?

BigDarkClown · 2023-11-29T16:55:17Z

test/e2e/autoscaling/cluster_size_autoscaling.go

+		cleanupFunc := ReserveMemoryWithSchedulerName(ctx, f, "memory-reservation", replicaCount, reservedMemory, false, 1, schedulerName)
+		defer cleanupFunc()
+		// Verify that cluster size is the same
+		ginkgo.By(fmt.Sprintf("Waiting for scale up hoping it won't happen, sleep for %s", scaleUpTimeout.String()))


IIUC we are not actually sleeping here, while we should.

Yes, mb I forgot to keep it consistent with the prev test, Should be alright now

atwamahmoud · 2023-11-29T19:19:20Z

Have you confirmed that these tests are running correctly?

Yes, they behave as expected

BigDarkClown · 2023-11-30T11:23:46Z

/lgtm

k8s-ci-robot · 2023-11-30T11:23:54Z

LGTM label has been added.

Git tree hash: 9e6c3d0c5911585537969632f81a2f0bab0d06fd

BigDarkClown · 2023-11-30T13:59:32Z

/lgtm

k8s-ci-robot · 2023-11-30T13:59:39Z

LGTM label has been added.

Git tree hash: 4554a5b58d04576342b6f0dfcbd31ee845c5e431

BigDarkClown · 2023-11-30T14:00:15Z

test/e2e/feature/feature.go

@@ -129,6 +129,7 @@ var (
 	Windows                                 = framework.WithFeature(framework.ValidFeatures.Add("Windows"))
 	WindowsHostProcessContainers            = framework.WithFeature(framework.ValidFeatures.Add("WindowsHostProcessContainers"))
 	WindowsHyperVContainers                 = framework.WithFeature(framework.ValidFeatures.Add("WindowsHyperVContainers"))
+	ClusterScaleUpBypassScheduler     = framework.WithFeature(framework.ValidFeatures.Add("ClusterScaleUpBypassScheduler"))


Incorrect tabulation?

Yes, mb. Should be ok now

aojea · 2024-01-10T20:07:41Z

/ok-to-test
please squash it into cone commit @atwamahmoud

aojea · 2024-01-11T10:21:45Z

test/e2e/autoscaling/cluster_size_autoscaling.go

+		ginkgo.DeferCleanup(ReserveMemoryWithSchedulerName(ctx, f, "memory-reservation", replicaCount, reservedMemory, false, 1, nonExistingBypassedSchedulerName))
+		// Verify that cluster size is the same
+		ginkgo.By(fmt.Sprintf("Waiting for scale up hoping it won't happen, sleep for %s", scaleUpTimeout.String()))
+		time.Sleep(scaleUpTimeout)


why I a sleep?

can't we do an active loop with a wait.Poll per example?

~~Mainly to be consistent with other test(s) that doesn't expect a scale up/down, in this test (and the next one) we're expecting no scale-up/down.~~
for example this test https://github.com/kubernetes/kubernetes/pull/121783/files/6073d1cd3d58384d12a750bda749ff1922812be3#diff-4f7cc8ec3b56aa879a019633705140fea00aafda1495d321704f92bd31ec6468R1001 doesn't expect a scale down so it sleeps for a while and then checks for the size

~~We can update it however to use a Poll up to a timeout and update other tests that sleep as well~~

Updated to use gomega.Consistently instead of sleeping

aojea · 2024-01-15T17:55:21Z

test/e2e/autoscaling/cluster_size_autoscaling.go

+		sizeFunc := func(size int) bool {
+			return size == nodeCount
+		}
+		gomega.Consistently(ctx, func() error {


Consistently or Eventually? https://onsi.github.io/gomega/#eventually

aojea · 2024-01-15T18:12:07Z

/hold cancel
/approve

Technically look correct, someone should review the test logic as I;m not familiar with this feature

k8s-ci-robot · 2024-01-15T18:12:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, atwamahmoud

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

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

atwamahmoud · 2024-01-16T13:04:22Z

/retest

BigDarkClown · 2024-01-17T15:06:36Z

/lgtm

k8s-ci-robot · 2024-01-17T15:06:43Z

LGTM label has been added.

Git tree hash: 20c92c675ac2502c6ac11d3f90c6cb310e808ead

pohly · 2024-02-13T11:26:54Z

test/e2e/autoscaling/cluster_size_autoscaling.go

+		}
+		framework.ExpectNoError(WaitForClusterSizeFuncWithUnready(ctx, f.ClientSet, sizeFunc, scaleUpTimeout, 0))
+	})
+	f.It("shouldn't scale up when unprocessed pod is created and is going to be schedulable", feature.ClusterScaleUpBypassScheduler, func(ctx context.Context) {


This test should have been marked as "slow" because it blocks for 5 minutes (= scaleUpTimeout).

pohly · 2024-02-13T11:27:39Z

test/e2e/feature/feature.go

@@ -132,6 +132,7 @@ var (
 	Windows                                 = framework.WithFeature(framework.ValidFeatures.Add("Windows"))
 	WindowsHostProcessContainers            = framework.WithFeature(framework.ValidFeatures.Add("WindowsHostProcessContainers"))
 	WindowsHyperVContainers                 = framework.WithFeature(framework.ValidFeatures.Add("WindowsHyperVContainers"))
+	ClusterScaleUpBypassScheduler           = framework.WithFeature(framework.ValidFeatures.Add("ClusterScaleUpBypassScheduler"))


In the future please keep this in alphabetical order (#123260 will make that more obvious).

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 7, 2023

k8s-ci-robot requested review from mwielgus and SataQiu November 7, 2023 18:58

BigDarkClown reviewed Nov 29, 2023

View reviewed changes

k8s-ci-robot assigned BigDarkClown Nov 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

k8s-ci-robot requested a review from BigDarkClown November 30, 2023 13:55

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2023

BigDarkClown reviewed Nov 30, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jan 10, 2024

atwamahmoud force-pushed the ignore-scheduler-tests branch from 685cbc0 to f9388c8 Compare January 10, 2024 20:02

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jan 10, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2024

Adds tests for bypassing scheduler processing

6073d1c

atwamahmoud force-pushed the ignore-scheduler-tests branch from f9388c8 to 6073d1c Compare January 10, 2024 20:25

aojea reviewed Jan 11, 2024

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 12, 2024

Use gomega.Consistently instead of sleeping

73565cd

atwamahmoud force-pushed the ignore-scheduler-tests branch from 4209f7c to 73565cd Compare January 12, 2024 16:26

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2024

aojea reviewed Jan 15, 2024

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2024

atwamahmoud added 2 commits January 16, 2024 10:45

Revert using ginkgo.DeferCleanup as it doesn't run in the correct order

80fbc4c

Handles clean up errors

ec89154

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2024

k8s-ci-robot merged commit b26f6df into kubernetes:master Jan 17, 2024
16 checks passed

k8s-ci-robot added this to the v1.30 milestone Jan 17, 2024

pohly reviewed Feb 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for ignoring scheduler processing #121783

Add tests for ignoring scheduler processing #121783

atwamahmoud commented Nov 7, 2023 •

edited

k8s-ci-robot commented Nov 7, 2023

k8s-ci-robot commented Nov 7, 2023

BigDarkClown commented Nov 29, 2023

BigDarkClown Nov 29, 2023

atwamahmoud Nov 29, 2023

atwamahmoud commented Nov 29, 2023

BigDarkClown commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

BigDarkClown commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

BigDarkClown Nov 30, 2023

atwamahmoud Nov 30, 2023

aojea commented Jan 10, 2024

aojea Jan 11, 2024

atwamahmoud Jan 11, 2024 •

edited

aojea Jan 15, 2024

aojea commented Jan 15, 2024

k8s-ci-robot commented Jan 15, 2024

atwamahmoud commented Jan 16, 2024

BigDarkClown commented Jan 17, 2024

k8s-ci-robot commented Jan 17, 2024

pohly Feb 13, 2024

pohly Feb 13, 2024

Add tests for ignoring scheduler processing #121783

Add tests for ignoring scheduler processing #121783

Conversation

atwamahmoud commented Nov 7, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Nov 7, 2023

k8s-ci-robot commented Nov 7, 2023

BigDarkClown commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atwamahmoud commented Nov 29, 2023

BigDarkClown commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

BigDarkClown commented Nov 30, 2023

k8s-ci-robot commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jan 10, 2024

Choose a reason for hiding this comment

atwamahmoud Jan 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Jan 15, 2024

k8s-ci-robot commented Jan 15, 2024

atwamahmoud commented Jan 16, 2024

BigDarkClown commented Jan 17, 2024

k8s-ci-robot commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atwamahmoud commented Nov 7, 2023 •

edited

atwamahmoud Jan 11, 2024 •

edited