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.

Sign up for GitHub

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

Jump to bottom

🌱 Add MachinePools to autoscaler e2e test #10083

Merged
merged 2 commits into from
Apr 8, 2024
Merged

🌱 Add MachinePools to autoscaler e2e test #10083

merged 2 commits into from
Apr 8, 2024

Conversation

willie-yao
Copy link
Contributor

What this PR does / why we need it:
This PR adds MachinePools to the autoscaler e2e tests. This is a follow-up task to enabling MachinePools in ClusterClass.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes part of #10028

/area clusterclass

@k8s-ci-robot k8s-ci-robot added area/clusterclass Issues or PRs related to clusterclass cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 31, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 31, 2024
@willie-yao
Copy link
Contributor Author

/test ?

@k8s-ci-robot
Copy link
Contributor

@willie-yao: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-build-main
  • /test pull-cluster-api-e2e-blocking-main
  • /test pull-cluster-api-e2e-conformance-ci-latest-main
  • /test pull-cluster-api-e2e-conformance-main
  • /test pull-cluster-api-e2e-dualstack-and-ipv6-main
  • /test pull-cluster-api-e2e-main
  • /test pull-cluster-api-e2e-mink8s-main
  • /test pull-cluster-api-e2e-upgrade-1-29-1-30-main
  • /test pull-cluster-api-test-main
  • /test pull-cluster-api-test-mink8s-main
  • /test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-apidiff-main
  • pull-cluster-api-build-main
  • pull-cluster-api-e2e-blocking-main
  • pull-cluster-api-test-main
  • pull-cluster-api-verify-main

In response to this:

/test ?

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.

@willie-yao willie-yao mentioned this pull request Jan 31, 2024
Closed
@willie-yao willie-yao force-pushed the cc-mp-autoscaler branch 4 times, most recently from 46d0b59 to 4ef0996 Compare February 2, 2024 21:22
@willie-yao willie-yao changed the title 🌱 [WIP] Add MachinePools to autoscaler e2e test 🌱 Add MachinePools to autoscaler e2e test Feb 9, 2024
@willie-yao willie-yao changed the title 🌱 Add MachinePools to autoscaler e2e test 🌱 WIP: Add MachinePools to autoscaler e2e test Feb 12, 2024
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 25, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 25, 2024
@willie-yao willie-yao force-pushed the cc-mp-autoscaler branch 2 times, most recently from 387c56e to 3d310d1 Compare March 26, 2024 00:18
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 26, 2024
@willie-yao willie-yao changed the title 🌱 WIP: Add MachinePools to autoscaler e2e test 🌱 Add MachinePools to autoscaler e2e test Mar 26, 2024
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 26, 2024
@willie-yao
Copy link
Contributor Author

@sbueringer Figured out the issues and this should now be ready for review! I'm wondering if there's a more efficient way to test both MD and MP in this flow right now though... I'm having to remove the annotations and delete the previous scale up deployment to test MPs without unpredictable behavior from the autoscaler.

@willie-yao
Copy link
Contributor Author

/test pull-cluster-api-e2e-main

@willie-yao
Copy link
Contributor Author

/test pull-cluster-api-e2e-main

Running a few times to ensure the test isn't flaky.

chrischdi
chrischdi reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b441bbf4d3fddd063e7b8644e64a77ff6e5ced01

@willie-yao
Copy link
Contributor Author

willie-yao commented Apr 4, 2024 •
edited
Loading

/test pull-cluster-api-e2e-main

Edit: unrelated flake, rerunning

@willie-yao
Copy link
Contributor Author

/test pull-cluster-api-e2e-main

1 similar comment
@sbueringer
Copy link
Member

/test pull-cluster-api-e2e-main

sbueringer
sbueringer reviewed Apr 5, 2024
View reviewed changes
Copy link
Member

@sbueringer sbueringer 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 very much for working on this

General approach lgtm, mostly just smaller findings

exp/internal/webhooks/machinepool_test.go Outdated Show resolved Hide resolved
test/framework/autoscaler_helpers.go Outdated Show resolved Hide resolved
test/framework/autoscaler_helpers.go Outdated Show resolved Hide resolved
test/framework/autoscaler_helpers.go Outdated Show resolved Hide resolved
test/framework/autoscaler_helpers.go Outdated Show resolved Hide resolved
test/framework/machinepool_helpers.go Show resolved Hide resolved
test/e2e/autoscaler.go Show resolved Hide resolved
test/e2e/autoscaler.go Outdated Show resolved Hide resolved
test/e2e/autoscaler.go Outdated Show resolved Hide resolved
test/e2e/autoscaler.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2024
@k8s-ci-robot k8s-ci-robot requested a review from chrischdi April 5, 2024 21:16
@willie-yao
Copy link
Contributor Author

/test pull-cluster-api-e2e-main

@sbueringer
Copy link
Member

Really nice work!

Thank you very much!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 0b80e69805e1a1d79e176c10636f8d3c18da6436

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2024
@k8s-ci-robot k8s-ci-robot merged commit 10741fd into kubernetes-sigs:main Apr 8, 2024
21 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.8 milestone Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

@chrischdi chrischdi Awaiting requested review from chrischdi

Assignees

@chrischdi chrischdi

@sbueringer sbueringer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterclass Issues or PRs related to clusterclass cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.
4 participants
@willie-yao @k8s-ci-robot @sbueringer @chrischdi