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

TAS: introduce sanity e2e test #3284

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

mimowo
Copy link
Contributor

@mimowo mimowo commented Oct 22, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

To make sure TAS works e2e, including computing the assignment and ungatibg the pods.

Which issue(s) this PR fixes:

Part of #2724

Special notes for your reviewer:

I'm happy to add more scenarios in this PR or on follow ups.

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 22, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 22, 2024
Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit c859074
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/671a087f5ebfdb0008ca605f

@mimowo
Copy link
Contributor Author

mimowo commented Oct 22, 2024

/cc @gabesaba @PBundyra @tenzen-y

@mimowo
Copy link
Contributor Author

mimowo commented Oct 22, 2024

/test pull-kueue-test-multikueue-e2e-main
again: go: sigs.k8s.io/[email protected]: sigs.k8s.io/[email protected]: Get "https://proxy.golang.org/sigs.k8s.io/kind/@v/v0.24.0.zip": dial tcp 142.250.191.241:443: connect: no route to host

@mimowo mimowo changed the title TAS: introduce baseline e2e test TAS: introduce sanity e2e test Oct 22, 2024
@tenzen-y
Copy link
Member

I will pass the first review to @gabesaba

{
Count: 1,
Values: []string{
"kind-worker",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this value come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the cluster yamls config https://github.com/mimowo/kueue/blob/main/hack%2Fkind-cluster.yaml#L19 specifies names of the worker nodes. I think the prefix 'kind-' is added by default, might be the cluster name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I check experimentally that the prefix comes from the cluster name, so I created a new kind cluster mycluster, and the nodes are named: mycluster-worker, mycluster-worker2. So the index must also be added by kind.

Copy link
Contributor

@gabesaba gabesaba left a comment

Choose a reason for hiding this comment

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

/lgtm

test/e2e/singlecluster/tas_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 79e94b1ea3547e8990cfc4ff6f774fcd0d149d69

@tenzen-y
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2024
@mimowo mimowo force-pushed the tas-baseline-e2e-tests branch from 01a321b to f2db7e9 Compare October 24, 2024 07:26
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2024
@k8s-ci-robot k8s-ci-robot requested a review from gabesaba October 24, 2024 07:26
@mimowo
Copy link
Contributor Author

mimowo commented Oct 24, 2024

I consider this a sanity test to verify basic scenarios. Most importantly to test that the assignment is set and the topology ungater lets the Pods to start and complete. Maybe also we could extend it in the future to cover some cases in #3292.

However, ultimately I think for Beta it would be have a dedicated CI Job just for TAS where we can setup a more complex node structure. Similarly as we have a dedicated CI job for multikueue: pull-kueue-test-multikueue-e2e-main. WDYT @tenzen-y @PBundyra ?

@tenzen-y
Copy link
Member

I consider this a sanity test to verify basic scenarios. Most importantly to test that the assignment is set and the topology ungater lets the Pods to start and complete. Maybe also we could extend it in the future to cover some cases in #3292.

However, ultimately I think for Beta it would be have a dedicated CI Job just for TAS where we can setup a more complex node structure. Similarly as we have a dedicated CI job for multikueue: pull-kueue-test-multikueue-e2e-main. WDYT @tenzen-y @PBundyra ?

That makes sense. We will set up multiple Nodes with different node labels to simulate TAS situations, right?

@mimowo
Copy link
Contributor Author

mimowo commented Oct 24, 2024

That makes sense. We will set up multiple Nodes with different node labels to simulate TAS situations, right?

Yes, we can figure out details later, but I think we could have, for example, 8 nodes divided into 2 blocks and 4 racks.

EDIT: added the point to the spreadsheet. I will also update the KEP after alpha.

@tenzen-y
Copy link
Member

That makes sense. We will set up multiple Nodes with different node labels to simulate TAS situations, right?

Yes, we can figure out details later, but I think we could have, for example, 8 nodes divided into 2 blocks and 4 racks.

EDIT: added the point to the spreadsheet. I will also update the KEP after alpha.

That sounds great.

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Basically, lgtm

test/e2e/singlecluster/tas_test.go Outdated Show resolved Hide resolved
test/e2e/singlecluster/tas_test.go Outdated Show resolved Hide resolved
test/e2e/singlecluster/tas_test.go Show resolved Hide resolved
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thank you!
/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 0f7aaf1831d85adb6bcbe041ad8239789c0b361b

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, tenzen-y

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 69cc05e into kubernetes-sigs:main Oct 24, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.9 milestone Oct 24, 2024
PBundyra pushed a commit to PBundyra/kueue that referenced this pull request Nov 5, 2024
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants