Return generated instance names from autoscaling GCE client#9315
Return generated instance names from autoscaling GCE client#9315pmendelski wants to merge 1 commit intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @pmendelski. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
k8s-ci-robot
added
area/cluster-autoscaler
area/provider/gce
size/M
|
cc @x13n |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
cluster-autoscaler/cloudprovider/gce/autoscaling_gce_client_test.go
Outdated
Show resolved
Hide resolved
|
/assign damikag |
k8s-ci-robot
added
the
lgtm
jbtk
left a comment
jbtk
left a comment
There was a problem hiding this comment.
Is there any further plan to use it? Can you provide more context?
pmendelski
force-pushed
the
mendel-2k-scale-up
branch
from
08e782d to
3386514
Compare
k8s-ci-robot
removed
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pmendelski The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm please change the kind as this is not a cleanup. Maybe a "feature" would be more suitable if we want to support new way of requesting capacity? Also please have a look at the failing test. |
k8s-ci-robot
added
the
do-not-merge/hold
|
Changed type to feature. Test failed on cluster initialization, rebasing and rerunning. |
When there are multiple `createInstances` requests sent in a sequence, the next request must be aware of the instance names used by the previous request. Without this change sending a sequence of create instances requests would result in the instance name duplication error.
pmendelski
force-pushed
the
mendel-2k-scale-up
branch
from
3386514 to
9e8e584
Compare
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
|
@pmendelski: The following test failed, say
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. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
| } | ||
| m.cache.InvalidateMigTargetSize(mig.GceRef()) | ||
| return m.GceService.CreateInstances(mig.GceRef(), baseName, delta, instancesNames) | ||
| _, err = m.GceService.CreateInstances(mig.GceRef(), baseName, delta, instancesNames) |
There was a problem hiding this comment.
Shouldn't you call it twice when delta>1k?
There was a problem hiding this comment.
I didn't need it, but I can add it.
7 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
We need this change to send a sequence of
createInstancesrequests to GCE.In case of a sequence of requests, the next request must be aware of the instance names used by the previous request. Without this change sending a sequence of create instances requests would result in the instance name duplication error.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: