-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Disable increaseSize when the node group is under initialilzation. #3242
Disable increaseSize when the node group is under initialilzation. #3242
Conversation
The unit test for |
@@ -327,6 +327,11 @@ func (scaleSet *ScaleSet) TargetSize() (int, error) { | |||
|
|||
// IncreaseSize increases Scale Set size | |||
func (scaleSet *ScaleSet) IncreaseSize(delta int) error { | |||
if scaleSet.curSize == -1 { | |||
klog.V(1).Infof("the scale set %s is under initialization, would skip the operation", scaleSet.Name) | |||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should return an error still so autoscaler can backoff and try to resize a different agentpool.
In a super rare scenario where you have many nodepools - some of them are initialized and some aren't. You still want to to be able to scale whatever was initialized.
@@ -327,6 +327,11 @@ func (scaleSet *ScaleSet) TargetSize() (int, error) { | |||
|
|||
// IncreaseSize increases Scale Set size | |||
func (scaleSet *ScaleSet) IncreaseSize(delta int) error { | |||
if scaleSet.curSize == -1 { | |||
klog.V(1).Infof("the scale set %s is under initialization, would skip the operation", scaleSet.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
klog.V(1).Infof("the scale set %s is under initialization, would skip the operation", scaleSet.Name) | |
klog.V(1).Infof("the scale set %s is under initialization, skipping the increase size operation", scaleSet.Name) |
klog.V(1).Infof("the scale set %s is under initialization, would skip the operation", scaleSet.Name) | ||
return nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also move this check under the line where it gets the currSize.
The
size, err := scaleSet.GetScaleSetSize()
if err != nil {
return err
}
happens under a mutex so it should be safer.
abe22ae
to
e3ff7f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments, otherwise looks good
@@ -336,6 +336,10 @@ func (scaleSet *ScaleSet) IncreaseSize(delta int) error { | |||
return err | |||
} | |||
|
|||
if scaleSet.curSize == -1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if scaleSet.curSize == -1 { | |
if size == -1 { |
@@ -302,6 +303,10 @@ func (as *AgentPool) IncreaseSize(delta int) error { | |||
as.mutex.Lock() | |||
defer as.mutex.Unlock() | |||
|
|||
if as.curSize == -1 { | |||
return fmt.Errorf("the availability set %s is under initialization, skipping the operation", as.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skipping the "increase size" or "scale up" operation - whatever works so its easier to figure out from logs without having to look at the source code
e3ff7f2
to
13cecd2
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the quick fix.
could you fix the unit test failures?
13cecd2
to
81cb1a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, marwanad 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 |
@nilo19 let's cherry-pick this to older releases |
Cherry-pick #3242: Disable increaseSize when the node group is under initialilzation.
Cherry-pick #3242: Disable increaseSize when the node group is under initialilzation.
Cherry-pick #3242: Disable increaseSize when the node group is under initialilzation.
Cherry-pick #3242: Disable increaseSize when the node group is under initialilzation.
Disable increaseSize when the node group is under initialilzation.
fixes: #3240
/kind bug
/area provider/azure