AWS: Use smallest instance type found in MixedInstancesPolicy #3205
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
otterley
force-pushed
the
use-smallest-instance-type-1.16
branch
from
5bfff2b to
f70f753
Compare
otterley
force-pushed
the
use-smallest-instance-type-1.16
branch
from
f70f753 to
9a0f73a
Compare
|
/assign @Jeffwan |
| @@ -17,7 +17,7 @@ fi | |||
|
|
|||
| SCRIPT_NAME=$(basename "$0") | |||
| K8S_FORK="[email protected]:kubernetes/kubernetes.git" | |||
| K8S_REV="master" | |||
| K8S_REV="release-1.16" | |||
There was a problem hiding this comment.
This can be overrided from argument. You don't need to make this change.
There was a problem hiding this comment.
I did this to replicate the work already done in the 1.15 branch (see #2971). This is a PR against the 1.16 branch, not master.
| @@ -3,7 +3,6 @@ go 1.12 | |||
|
|
|||
| require ( | |||
| github.com/rancher/go-rancher v0.1.0 | |||
| github.com/google/go-querystring v1.0.0 | |||
There was a problem hiding this comment.
I don't see any pkg deletion. Do you needs this change?
There was a problem hiding this comment.
When I ran hack/update-vendor.sh, it said that this was removed. I had to remove it to get the script to finish successfully.
| // Use smallest instance of all the instance types. Otherwise, we run | ||
| // the risk of launching an instance that is too small for the pending | ||
| // pod to actually fit in. | ||
| if len(asg.MixedInstancesPolicy.instanceTypesOverrides) > 0 { |
There was a problem hiding this comment.
em. I understand the point that CA uses smallest instance for simulation. The problem is most of the time, ASG will spawn new instances based on override sequence order. If the instance is not even close, that would be a big problem.
One case is c5.4xlarge, c5.2xlarge, c5.xlarge. It will use c5.xlarge as template, it probably spin up 4 instances. however, one c5.4xlarge can fit all pending pods.
There was a problem hiding this comment.
Sorry, I'm not understanding. Why would it be a problem?
First, we do make it pretty clear in the README PR (#3198) that customers shouldn't be placing instance types of such vastly different sizes in the same ASG.
But let's suppose for the sake of argument that they do; and a c5.4xlarge comes up. But so what? The pending pod fits, so there's no harm. The worst case is that there could be a bit of temporary waste in spinning up more instances than is strictly needed, but in that case, the excess instances will eventually be scaled in.
The question, I think, is what do we care about more: wasted spend or a pod not fitting? Should we allow customers to specify that preference? And what should we do in the case of a pod not fitting? I think preventing placement delays is more important.
Also, the example you gave had specific ordering. The result would be different if it were c5.xlarge, c5.2xlarge, and c5.4xlarge. The fact that it's order-dependent is a pretty strong argument in favor of merging, because otherwise it makes the situation even harder to reason about.
k8s-ci-robot
added
the
needs-rebase
|
@otterley: PR needs rebase. 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. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Inactivity warning. Please proceed with the PR or it will be closed. |
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
|
/cc @ellistarn @jaypipes please have review this change. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Closing due to inactivity. Feel free to reopen if needed. |
When determining the capacity of an Auto Scaling Group that has a Mixed Instances Policy, use the smallest instance of all the instance types in it. Otherwise, we run the risk of launching an instance that is too small for a pending pod to actually fit in.