-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Cluster-autoscaler: cloud provider interface #1312
Cluster-autoscaler: cloud provider interface #1312
Conversation
NodeGroupForNode(*kube_api.Node) (*NodeGroup, error) | ||
} | ||
|
||
// NodeGroup contains configuration info and functionns to control a set |
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.
typo functions
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.
Done.
9f246ae
to
ee09410
Compare
ee09410
to
1f5d517
Compare
// IncreaseSize increases the size of the node group. To delete a node you need | ||
// to explicitely name it and use CloudProvider. This function should wait until | ||
// the node appears in the Kubernetes cluster. | ||
IncreaseSize(newNodeCount int) error |
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.
Do we want the desired total count, or just the delta?
I think we should only need to tell the NodeGroup
how many instances to add, and not have to be concerned with its total size. Thoughts?
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.
Positive delta. Cluster Autoscaler will not delete nodes without naming them explicitly.
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.
Makes sense. I think the docs should explain that newNodeCount
is the positive delta.
When I first read newNodeCount
I wasn't sure if it meant the delta or the "new total".
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.
Renamed.
1f5d517
to
020db08
Compare
// if occurred. | ||
SampleNode() (*kube_api.Node, error) | ||
|
||
// IncreaseSize increases the size of the node group. To delete a node you need |
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.
To delete a node you need to explicitely name it and use CloudProvider.
Is this by mistake?
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.
Yep, fixed.
@mwielgus As discussed offline, in the PR description please explain the motivation for adding this new interface instead of using an existing cloud provider interface that exists in kubernetes. |
020db08
to
bf5da4c
Compare
LGTM |
https://github.com/kubernetes/kubernetes/tree/master/pkg/cloudprovider doesn't define a concept of NodePool/Group/Mig and doesn't have add/delete node in the main interface (although some implementation have it). However we may reuse some fragments of K8s implementation. We could also expand the kubernetes interface although we are not sure how/if the similar capabilities exist in other cloud providers and how to implement them. Once we have some implementations in contrib we may think about moving them to K8s. |
bf5da4c
to
f98d6d2
Compare
…d-provider-int Cluster-autoscaler: cloud provider interface
…d-provider-int Cluster-autoscaler: cloud provider interface
https://github.com/kubernetes/kubernetes/tree/master/pkg/cloudprovider doesn't define a concept of NodePool/Group/Mig and doesn't have add/delete node in the main interface (although some implementation have it). However we may reuse some fragments of K8s implementation. We could also expand the kubernetes interface although we are not sure how/if the similar capabilities exist in other cloud providers and how to implement them. Once we have some implementations in contrib we may think about moving them to K8s.
Ref: #1311
cc: @piosz @jszczepkowski @fgrzadkowski