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

🌱 MD: improve replica defaulting for autoscaler #7990

Merged
merged 1 commit into from
Feb 7, 2023
Merged

🌱 MD: improve replica defaulting for autoscaler #7990

merged 1 commit into from
Feb 7, 2023

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Jan 24, 2023 •
edited
Loading

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:

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 #7293

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 24, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 24, 2023
@sbueringer sbueringer changed the title [WIP][DNR][POC] MD: improve replica defaulting for autoscaler [WIP][DNR][POC] 🌱 MD: improve replica defaulting for autoscaler Jan 24, 2023
@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from a85a102 to 776d4eb Compare January 24, 2023 17:18
@sbueringer sbueringer changed the title [WIP][DNR][POC] 🌱 MD: improve replica defaulting for autoscaler [WIP][DNR] 🌱 MD: improve replica defaulting for autoscaler Jan 24, 2023
@sbueringer
Copy link
Member Author

/test ?

@k8s-ci-robot
Copy link
Contributor

@sbueringer: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-build-main
  • /test pull-cluster-api-e2e-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
  • /test pull-cluster-api-e2e-full-main
  • /test pull-cluster-api-e2e-informing-ipv6-main
  • /test pull-cluster-api-e2e-informing-main
  • /test pull-cluster-api-e2e-workload-upgrade-1-26-latest-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-informing-ipv6-main
  • pull-cluster-api-e2e-informing-main
  • pull-cluster-api-e2e-main
  • pull-cluster-api-test-main
  • pull-cluster-api-test-mink8s-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.

@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-26-latest-main

@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch 4 times, most recently from 466942e to f498d4b Compare January 25, 2023 11:19
@sbueringer sbueringer mentioned this pull request Jan 25, 2023
Closed
@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from f498d4b to b502d46 Compare January 25, 2023 11:57
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from b502d46 to 91c1f13 Compare January 25, 2023 15:00
@sbueringer
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2023
@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from 91c1f13 to a9ced03 Compare February 1, 2023 14:37
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2023
@sbueringer sbueringer changed the title [WIP][DNR] 🌱 MD: improve replica defaulting for autoscaler 🌱 MD: improve replica defaulting for autoscaler Feb 1, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2023
@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch 2 times, most recently from fcaf79b to 3255901 Compare February 1, 2023 14:55
@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 Feb 1, 2023
@sbueringer
Copy link
Member Author

/assign @elmiko @fabriziopandini

@k8s-ci-robot
Copy link
Contributor

@abhijit-dev82: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@sbueringer
Copy link
Member Author

Apologies for the confusion , my query was on Day-2 OP.

On unsetting the replicas field, wont the cluster size shrink and hence disrupt the application running on it. And once it is removed how will the value be reverted to on removing autoscaler ?

Just to mirror it here as well. Quickly talked about it. If during unsetting the replica field the min-size/max-size annotations are set as well, there won't be a disruption as the replica field will just stay the same (as unsetting replicas + defaulting happens in one update-MD call)

@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from 9d87605 to 1e85904 Compare February 6, 2023 12:38
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2023
@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from 1e85904 to 6520263 Compare February 6, 2023 12:42
@sbueringer
Copy link
Member Author

@fabriziopandini @enxebre @elmiko @abhijit-dev82

Removed the default-replicas annotation

@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from 6520263 to f198a76 Compare February 6, 2023 12:45
@fabriziopandini fabriziopandini mentioned this pull request Feb 6, 2023
Closed
elmiko
elmiko reviewed Feb 6, 2023
View reviewed changes
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

thanks @sbueringer , i think just one dangling reference to the default-replicas then should be good for me

api/v1beta1/machinedeployment_webhook.go Outdated Show resolved Hide resolved
@sbueringer
MD: improve replica defaulting for autoscaler
0061757 
Signed-off-by: Stefan Büringer buringerst@vmware.com
@sbueringer sbueringer force-pushed the pr-autoscaler-defaulting branch from f198a76 to 0061757 Compare February 6, 2023 15:43
@sbueringer
Copy link
Member Author

Thx for catching that, fixed!

@k8s-ci-robot
Copy link
Contributor

@sbueringer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-apidiff-main 0061757 link false /test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

elmiko
elmiko reviewed Feb 6, 2023
View reviewed changes
Copy link
Contributor

@elmiko elmiko 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 Feb 6, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4eee904be770f8407cccbca5806815320037f233

@abhijit-dev82
Copy link
Contributor

As discussed we need an bug to track that adding back the "replicas" field should be allowed when the autoscaler annotations are removed.

@sbueringer
Copy link
Member Author

sbueringer commented Feb 7, 2023 •
edited
Loading

As discussed we need an bug to track that adding back the "replicas" field should be allowed when the autoscaler annotations are removed.

I wouldn't call it a bug rather a feature, but feel free to open an issue for it.

@fabriziopandini
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Feb 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit aecc368 into kubernetes-sigs:main Feb 7, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Feb 7, 2023
@sbueringer sbueringer deleted the pr-autoscaler-defaulting branch February 8, 2023 04:39
@sbueringer sbueringer mentioned this pull request Feb 8, 2023
Closed
This was referenced Mar 1, 2023
Closed
Open
@sbueringer sbueringer mentioned this pull request Jul 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elmiko elmiko elmiko left review comments

@enxebre enxebre enxebre left review comments

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@elmiko elmiko

@fabriziopandini fabriziopandini

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. 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.4
Development

Successfully merging this pull request may close these issues.

MachineDeployment.spec.replicas defaulting should take into account autoscaler min/max size if defined
6 participants
@sbueringer @k8s-ci-robot @enxebre @abhijit-dev82 @fabriziopandini @elmiko