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

Capacity prediction based on physical memory #1643

Merged
merged 2 commits into from
Feb 11, 2019
Merged

Capacity prediction based on physical memory #1643

merged 2 commits into from
Feb 11, 2019

Conversation

jkaniuk
Copy link
Contributor

@jkaniuk jkaniuk commented Feb 1, 2019

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 1, 2019
@jkaniuk
Copy link
Contributor Author

jkaniuk commented Feb 1, 2019

/cc aleksandra-malinowska
/cc MaciekPytel
/cc losipiuk

@jkaniuk jkaniuk force-pushed the capacity-prediction branch 3 times, most recently from 3017a20 to 4792a03 Compare February 1, 2019 11:55
@jkaniuk jkaniuk force-pushed the capacity-prediction branch from 4792a03 to be59105 Compare February 1, 2019 13:44
@jkaniuk jkaniuk force-pushed the capacity-prediction branch 3 times, most recently from 3d35b87 to d119e09 Compare February 1, 2019 14:16
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 1, 2019
@jkaniuk
Copy link
Contributor Author

jkaniuk commented Feb 1, 2019

/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 Feb 1, 2019
@jkaniuk jkaniuk force-pushed the capacity-prediction branch 3 times, most recently from 56b1f93 to d1231f1 Compare February 5, 2019 08:22
@jkaniuk jkaniuk force-pushed the capacity-prediction branch from d1231f1 to b178b82 Compare February 7, 2019 12:53
@aleksandra-malinowska
Copy link
Contributor

/lgtm

How did you test it?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2019
@jkaniuk jkaniuk force-pushed the capacity-prediction branch from b178b82 to a54b581 Compare February 8, 2019 08:32
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2019
@jkaniuk jkaniuk force-pushed the capacity-prediction branch from 40b3782 to f054c53 Compare February 8, 2019 16:04
@losipiuk
Copy link
Contributor

Skimmed. Seems fine.
/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 Feb 11, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: losipiuk

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 11, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2019
aleksandra-malinowska
aleksandra-malinowska reviewed Feb 11, 2019
View reviewed changes
cluster-autoscaler/core/utils_test.go
assert.False(t, found)

// Invalidate part of cache in two different ways
provider1.DeleteNodeGroup("ng1")
Copy link
Contributor

@aleksandra-malinowska aleksandra-malinowska Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

nit: doesn't the below call (nodeGroup.Delete()) also call this internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, with small differences. Those two different way do delete a group test more those mocks than anything else. I think that it is still beneficial to leave it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I'm following - Delete() doesn't call DeleteNodeGroup() if and only if callback failed; we aren't checking any error in the below call, do we expect it to fail?

@jkaniuk
Copy link
Contributor Author

jkaniuk commented Feb 11, 2019 •
edited
Loading

Regarding tests:

  • spinning up cluster with empty autoscaling node-pool
  • adding pod with capacity 1Mi bigger than computed according to new allocatable calculations minus 220Mi (DeamonSets)
  • confirming that will not spin up a node
  • removing pod
  • adding pod with capacity computed according to new allocatable calculation minus 220Mi (DeamonSets)
  • confirming that will spin up a node
  • pod will not fit as real allocatable is slightly less (1-2MB)
  • waiting for node to be taken down as unneeded
  • confirming that no new node is added to that nodepool because of that pod (so cache is working)
  • deleting node pool
  • confirming that cluster-autoscaler handles node pool removal
  • adding new node pool with the same name
  • confirming that new node once again is spin up for given pod and new node pool (so cache is cleared)

Separately, capacity and allocatable calculations were checked against many machine-types and GKE versions 1.11 and 1.12

@jkaniuk
Copy link
Contributor Author

jkaniuk commented Feb 11, 2019

/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 Feb 11, 2019
@aleksandra-malinowska
Copy link
Contributor

/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 11, 2019
@k8s-ci-robot k8s-ci-robot merged commit da2e73d into kubernetes:master Feb 11, 2019
@aleksandra-malinowska aleksandra-malinowska mentioned this pull request Feb 12, 2019
Closed
jkaniuk added a commit to jkaniuk/autoscaler that referenced this pull request Feb 13, 2019
@jkaniuk
Cherry-pick of kubernetes#1643: Account for kernel reserved memory in…
d7cfcbd 
… capacity calculations
jkaniuk added a commit to jkaniuk/autoscaler that referenced this pull request Feb 13, 2019
@jkaniuk
Cherry-pick of kubernetes#1643: Cache exemplar ready node for each no…
9f6b974 
…de group
@jkaniuk jkaniuk mentioned this pull request Feb 13, 2019
Merged
jkaniuk added a commit to jkaniuk/autoscaler that referenced this pull request Feb 14, 2019
@jkaniuk
Cherry-pick of kubernetes#1643: Cache exemplar ready node for each no…
5886308 
…de group
jkaniuk added a commit to jkaniuk/autoscaler that referenced this pull request Feb 14, 2019
@jkaniuk
Cherry-pick of kubernetes#1643: Account for kernel reserved memory in…
d5e633f 
… capacity calculations
@jkaniuk jkaniuk mentioned this pull request Feb 14, 2019
Merged
k8s-ci-robot added a commit that referenced this pull request Feb 14, 2019
@k8s-ci-robot
Merge pull request #1682 from jkaniuk/capacity-prediction-1.12
9410e2b 
Cherry-pick of #1643: Capacity prediction based on physical memory
k8s-ci-robot added a commit that referenced this pull request Feb 14, 2019
@k8s-ci-robot
Merge pull request #1689 from jkaniuk/capacity-prediction-1.3
729ba95 
Cherry-pick of #1643: Capacity prediction based on physical memory
jkaniuk added a commit to jkaniuk/autoscaler that referenced this pull request Feb 15, 2019
@jkaniuk
Cherry-pick of kubernetes#1643: Account for kernel reserved memory in…
2e6319d 
… capacity calculations
jkaniuk added a commit to jkaniuk/autoscaler that referenced this pull request Feb 15, 2019
@jkaniuk
Cherry-pick of kubernetes#1643: Cache exemplar ready node for each no…
c7e9815 
…de group
@jkaniuk jkaniuk mentioned this pull request Feb 15, 2019
Merged
k8s-ci-robot added a commit that referenced this pull request Feb 15, 2019
@k8s-ci-robot
Merge pull request #1695 from jkaniuk/capacity-prediction-1.13
c70f0d9 
Cherry-pick of #1643: Capacity prediction based on physical memory
@jkaniuk jkaniuk deleted the capacity-prediction branch March 22, 2019 16:46
yaroslava-serdiuk pushed a commit to yaroslava-serdiuk/autoscaler that referenced this pull request Feb 22, 2024
@trasc
[multikueue] Add garbage collector. (kubernetes#1643)
2326377 
* [multikueue] Add garbage collector.

* Fix after rebase

* Review Remarks

* Extend API description.

* Make Origin a global config.

* Review Remarks

* Use GCInterval in config

* Review remarks

* Rename gc timeout default const

* Review Remarks and UT scenario cleanup.

* [config] Make multikueue Origin pointer

* Review remarks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleksandra-malinowska aleksandra-malinowska aleksandra-malinowska left review comments

@losipiuk losipiuk Awaiting requested review from losipiuk

@piosz piosz Awaiting requested review from piosz

@MaciekPytel MaciekPytel Awaiting requested review from MaciekPytel

Assignees

@losipiuk losipiuk

@aleksandra-malinowska aleksandra-malinowska

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jkaniuk @aleksandra-malinowska @losipiuk @k8s-ci-robot