Rewrite addon-resizer nanny #128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
|
/assign @mwielgus |
mwielgus
reviewed
Jun 20, 2017
addon-resizer/Makefile
Outdated
| @@ -25,7 +25,7 @@ | |||
| OUT_DIR = build | |||
| PACKAGE = k8s.io/contrib/addon-resizer | |||
| PREFIX = gcr.io/google_containers | |||
| TAG = 1.7 | |||
| TAG = 1.8 | |||
There was a problem hiding this comment.
it is a significant rewrite - lets bump it to 2.0
mwielgus
reviewed
Jun 20, 2017
addon-resizer/nanny/estimator.go
Outdated
|
|
||
| log "github.com/golang/glog" |
There was a problem hiding this comment.
imports:
core-go
blank line
kubernetes
blank line
3rd-party
There was a problem hiding this comment.
And your point is...? :-) Note that nanny_lib was already using glog.
There was a problem hiding this comment.
Oh, you meant the order. Fixed.
x13n
force-pushed
the
master
branch
2 times, most recently
from
db94731
to
a4440c3
Compare
mwielgus
reviewed
Jun 20, 2017
addon-resizer/nanny/estimator.go
Outdated
| Resources []Resource | ||
| // ResourceListPair is a pair of ResourceLists, denoting a range. | ||
| type ResourceListPair struct { | ||
| lower, upper api.ResourceList |
There was a problem hiding this comment.
what exactly is lower/upper? is it min/max? please document.
mwielgus
reviewed
Jun 20, 2017
addon-resizer/nanny/estimator.go
Outdated
| return calculateResources(numNodes, e.Resources) | ||
| // EstimatorResult is the result of the resource Estimation, used by Estimator struct. | ||
| type EstimatorResult struct { | ||
| RecommendedRange, AcceptableRange ResourceListPair |
There was a problem hiding this comment.
All public fields must have documentation.
One field per line.
mwielgus
reviewed
Jun 20, 2017
addon-resizer/nanny/estimator.go
Outdated
| ScaleFactor float64 | ||
| // Estimator is a struct used for estimating accepted and recommended resource requirements. | ||
| type Estimator struct { | ||
| Resources []Resource |
mwielgus
reviewed
Jun 20, 2017
| pollPeriod = time.Millisecond * time.Duration(*flag.Int("poll-period", 10000, "The time, in milliseconds, to poll the dependent container.")) | ||
| estimator = flag.String("estimator", "linear", "The estimator to use. Currently supported: linear, exponential") | ||
| pollPeriodMillis = flag.Int("poll-period", 10000, "The time, in milliseconds, to poll the dependent container.") | ||
| estimator = flag.String("estimator", "linear", "[DEPRECATED] No-op. Can be set to: linear, exponential") |
mwielgus
reviewed
Jun 20, 2017
| memoryPerNode = flag.String("extra-memory", "0Mi", "The amount of memory to add per node.") | ||
| baseStorage = flag.String("storage", noValue, "The base storage resource requirement.") | ||
| storagePerNode = flag.String("extra-storage", "0Gi", "The amount of storage to add per node.") | ||
| threshold = flag.Int("threshold", 0, "[DEPRECATED] Please use recommendation-offset and acceptance-offset istead.\nA number between 0-100. The dependent's resources are rewritten when they deviate from expected by more than threshold.") |
mwielgus
reviewed
Jun 20, 2017
| ) | ||
|
|
||
| func checkPercentageBounds(flagName string, flagValue int) { |
There was a problem hiding this comment.
checkPercentageFlagBounds
mwielgus
reviewed
Jun 20, 2017
| checkPercentageBounds("acceptance-offset", *acceptanceOffset) | ||
|
|
||
| pollPeriod := time.Millisecond * time.Duration(*pollPeriodMillis) | ||
| log.Infof("Poll period: %+v", pollPeriod) | ||
| log.Infof("Watching namespace: %s, pod: %s, container: %s.", *podNamespace, *podName, *containerName) | ||
| log.Infof("cpu: %s, extra_cpu: %s, memory: %s, extra_memory: %s, storage: %s, extra_storage: %s", *baseCPU, *cpuPerNode, *baseMemory, *memoryPerNode, *baseStorage, *storagePerNode) |
There was a problem hiding this comment.
What about printing bounds?
mwielgus
reviewed
Jun 20, 2017
| Resources: resources, | ||
| ScaleFactor: 1.5, | ||
| } | ||
| if *estimator == "linear" || *estimator == "exponential" { |
There was a problem hiding this comment.
AFAIR this was about to be a noop.
mwielgus
reviewed
Jun 20, 2017
addon-resizer/nanny/nanny_lib.go
Outdated
| func shouldOverwriteResources(estimatorResult *EstimatorResult, limits, reqs api.ResourceList) *api.ResourceRequirements { | ||
| for _, list := range []api.ResourceList{limits, reqs} { | ||
| for _, resourceType := range []api.ResourceName{api.ResourceCPU, api.ResourceMemory, api.ResourceStorage} { | ||
| p := checkResource(estimatorResult, list, resourceType) |
There was a problem hiding this comment.
what is p? Please use full names.
|
Please explain how the PR was tested. |
x13n
force-pushed
the
master
branch
2 times, most recently
from
9ff39be
to
7df7cde
Compare
|
Apart from unit tests, I ran a manual integration test, setting up my own Kubernetes cluster on GCE and resizing it in a way described in the doc (example scenario) linked in the first post & verifying changes occur as expected. |
|
Doesn't compile. |
There will be a single estimator class, providing two ranges: acceptable and recommended range. As long as current pod requirements and limits fall into acceptable range, nothing happens. Once either limits or requirements fall out of acceptable range, they are both updated to lower (when upscaling) or higher (when downscaling) end of recommended range. This approach prevents flapping, which took place in previous implementation, when cluster size oscillated around certain values. Also, fix the code to actually use poll-period flag.
|
Does compile. |
|
Fixes kubernetes/kubernetes#30446. |
|
LGTM |
mwielgus
added
the
lgtm
JoelSpeed
pushed a commit
to JoelSpeed/autoscaler
that referenced
this pull request
May 13, 2020
…rry-pick-125-to-release-4.4 [release-4.4] BUG 1805153: Ensure DeleteNodes doesn't delete a node twice
yaroslava-serdiuk
pushed a commit
to yaroslava-serdiuk/autoscaler
that referenced
this pull request
Feb 22, 2024
Fix RBAC after adding Priority support
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There will be a single estimator class, providing two ranges: acceptable
and recommended range. As long as current pod requirements and limits fall into
acceptable range, nothing happens. Once either limits or requirements
fall out of acceptable range, they are both updated to lower (when
upscaling) or higher (when downscaling) end of recommended range. This
approach prevents flapping, which took place in previous implementation,
when cluster size oscillated around certain values.
More details: https://docs.google.com/a/google.com/document/d/1T0A7GHwNU_w6gpq_eCN166aRth6usbRvxgJJqSZeESU/edit?usp=sharing (shared with kubernetes-sig-instrumentation and kubernetes-sig-autoscaling)
Also, fix the code to actually use poll-period flag.
(PR moved from kubernetes-retired/contrib#2623)