Skip to content

Commit

Permalink
all code review comments addressed
Browse files Browse the repository at this point in the history
  • Loading branch information
@elankath
elankath committed Feb 3, 2025
1 parent b3f9167 commit 7d1d0d7
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 46 deletions.
60 changes: 30 additions & 30 deletions cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func buildStaticallyDiscoveringProvider(mcmManager *McmManager, resourceLimiter
return mcm, nil
}

// Cleanup stops the go routine that is handling the current view of the NodeGroupImpl in the form of a cache
// Cleanup stops the go routine that is handling the current view of the nodeGroup in the form of a cache
func (mcm *mcmCloudProvider) Cleanup() error {
mcm.mcmManager.Cleanup()
return nil
Expand All @@ -119,11 +119,11 @@ func (mcm *mcmCloudProvider) Name() string {
// NodeGroups returns all node groups configured for this cloud provider.
func (mcm *mcmCloudProvider) NodeGroups() []cloudprovider.NodeGroup {
result := make([]cloudprovider.NodeGroup, 0, len(mcm.mcmManager.nodeGroups))
for _, nodeGroup := range mcm.mcmManager.nodeGroups {
if nodeGroup.maxSize == 0 {
for _, ng := range mcm.mcmManager.nodeGroups {
if ng.maxSize == 0 {
continue
}
result = append(result, nodeGroup)
result = append(result, ng)
}
return result
}
Expand All @@ -145,19 +145,19 @@ func (mcm *mcmCloudProvider) NodeGroupForNode(node *apiv1.Node) (cloudprovider.N
return nil, nil
}

md, err := mcm.mcmManager.GetNodeGroupImpl(mInfo.Key)
ng, err := mcm.mcmManager.getNodeGroup(mInfo.Key)
if err != nil {
return nil, err
}

key := types.NamespacedName{Namespace: md.Namespace, Name: md.Name}
key := types.NamespacedName{Namespace: ng.Namespace, Name: ng.Name}
_, isManaged := mcm.mcmManager.nodeGroups[key]
if !isManaged {
klog.V(4).Infof("Skipped node %v, it's not managed by this controller", node.Spec.ProviderID)
return nil, nil
}

return md, nil
return ng, nil
}

// HasInstance returns whether a given node has a corresponding instance in this cloud provider
Expand Down Expand Up @@ -227,8 +227,8 @@ func (mcm *mcmCloudProvider) GetNodeGpuConfig(*apiv1.Node) *cloudprovider.GpuCon
return nil
}

// NodeGroupImpl implements NodeGroup interface.
type NodeGroupImpl struct {
// nodeGroup implements NodeGroup interface.
type nodeGroup struct {
types.NamespacedName

mcmManager *McmManager
Expand All @@ -239,47 +239,47 @@ type NodeGroupImpl struct {
}

// MaxSize returns maximum size of the node group.
func (ngImpl *NodeGroupImpl) MaxSize() int {
func (ngImpl *nodeGroup) MaxSize() int {
return ngImpl.maxSize
}

// MinSize returns minimum size of the node group.
func (ngImpl *NodeGroupImpl) MinSize() int {
func (ngImpl *nodeGroup) MinSize() int {
return ngImpl.minSize
}

// TargetSize returns the current TARGET size of the node group. It is possible that the
// number is different from the number of nodes registered in Kubernetes.
func (ngImpl *NodeGroupImpl) TargetSize() (int, error) {
func (ngImpl *nodeGroup) TargetSize() (int, error) {
size, err := ngImpl.mcmManager.GetMachineDeploymentSize(ngImpl.Name)
return int(size), err
}

// Exist checks if the node group really exists on the cloud provider side. Allows to tell the
// theoretical node group from the real one.
// TODO: Implement this to check if machine-deployment really exists.
func (ngImpl *NodeGroupImpl) Exist() bool {
func (ngImpl *nodeGroup) Exist() bool {
return true
}

// Create creates the node group on the cloud provider side.
func (ngImpl *NodeGroupImpl) Create() (cloudprovider.NodeGroup, error) {
func (ngImpl *nodeGroup) Create() (cloudprovider.NodeGroup, error) {
return nil, cloudprovider.ErrAlreadyExist
}

// Autoprovisioned returns true if the node group is autoprovisioned.
func (ngImpl *NodeGroupImpl) Autoprovisioned() bool {
func (ngImpl *nodeGroup) Autoprovisioned() bool {
return false
}

// Delete deletes the node group on the cloud provider side.
// This will be executed only for autoprovisioned node groups, once their size drops to 0.
func (ngImpl *NodeGroupImpl) Delete() error {
func (ngImpl *nodeGroup) Delete() error {
return cloudprovider.ErrNotImplemented
}

// IncreaseSize of the Machinedeployment.
func (ngImpl *NodeGroupImpl) IncreaseSize(delta int) error {
func (ngImpl *nodeGroup) IncreaseSize(delta int) error {
klog.V(0).Infof("Received request to increase size of machine deployment %s by %d", ngImpl.Name, delta)
if delta <= 0 {
return fmt.Errorf("size increase must be positive")
Expand All @@ -304,7 +304,7 @@ func (ngImpl *NodeGroupImpl) IncreaseSize(delta int) error {
// request for new nodes that have not been yet fulfilled. Delta should be negative.
// It is assumed that cloud provider will not delete the existing nodes if the size
// when there is an option to just decrease the target.
func (ngImpl *NodeGroupImpl) DecreaseTargetSize(delta int) error {
func (ngImpl *nodeGroup) DecreaseTargetSize(delta int) error {
klog.V(0).Infof("Received request to decrease target size of machine deployment %s by %d", ngImpl.Name, delta)
if delta >= 0 {
return fmt.Errorf("size decrease size must be negative")
Expand All @@ -326,7 +326,7 @@ func (ngImpl *NodeGroupImpl) DecreaseTargetSize(delta int) error {
}

// Refresh cordons the Nodes corresponding to the machines that have been marked for deletion in the TriggerDeletionByMCM annotation on the MachineDeployment
func (ngImpl *NodeGroupImpl) Refresh() error {
func (ngImpl *nodeGroup) Refresh() error {
mcd, err := ngImpl.mcmManager.GetMachineDeploymentObject(ngImpl.Name)
if err != nil {
return err
Expand Down Expand Up @@ -359,12 +359,12 @@ func (ngImpl *NodeGroupImpl) Refresh() error {
}

// Belongs checks if the given node belongs to this NodeGroup and also returns its MachineInfo for its corresponding Machine
func (ngImpl *NodeGroupImpl) Belongs(node *apiv1.Node) (belongs bool, mInfo *machineInfo, err error) {
func (ngImpl *nodeGroup) Belongs(node *apiv1.Node) (belongs bool, mInfo *machineInfo, err error) {
mInfo, err = ngImpl.mcmManager.getMachineInfo(node)
if err != nil || mInfo == nil {
return
}
targetMd, err := ngImpl.mcmManager.GetNodeGroupImpl(mInfo.Key)
targetMd, err := ngImpl.mcmManager.getNodeGroup(mInfo.Key)
if err != nil {
return
}
Expand All @@ -380,7 +380,7 @@ func (ngImpl *NodeGroupImpl) Belongs(node *apiv1.Node) (belongs bool, mInfo *mac

// DeleteNodes deletes the nodes from the group. It is expected that this method will not be called
// for nodes which are not part of ANY machine deployment.
func (ngImpl *NodeGroupImpl) DeleteNodes(nodes []*apiv1.Node) error {
func (ngImpl *nodeGroup) DeleteNodes(nodes []*apiv1.Node) error {
klog.V(0).Infof("for NodeGroup %q, Received request to delete nodes:- %v", ngImpl.Name, getNodeNames(nodes))
size, err := ngImpl.mcmManager.GetMachineDeploymentSize(ngImpl.Name)
if err != nil {
Expand All @@ -407,7 +407,7 @@ func (ngImpl *NodeGroupImpl) DeleteNodes(nodes []*apiv1.Node) error {
}

// deleteMachines annotates the corresponding MachineDeployment with machine names of toBeDeletedMachineInfos, reduces the desired replicas of the corresponding MachineDeployment and cordons corresponding nodes belonging to toBeDeletedMachineInfos
func (ngImpl *NodeGroupImpl) deleteMachines(toBeDeletedMachineInfos []machineInfo) error {
func (ngImpl *nodeGroup) deleteMachines(toBeDeletedMachineInfos []machineInfo) error {
if len(toBeDeletedMachineInfos) == 0 {
return nil
}
Expand Down Expand Up @@ -446,7 +446,7 @@ func (ngImpl *NodeGroupImpl) deleteMachines(toBeDeletedMachineInfos []machineInf
}

// AcquireScalingMutex acquires the scalingMutex associated with this NodeGroup and returns a function that releases the scalingMutex that is expected to be deferred by the caller.
func (ngImpl *NodeGroupImpl) AcquireScalingMutex(operation string) (releaseFn func()) {
func (ngImpl *nodeGroup) AcquireScalingMutex(operation string) (releaseFn func()) {
ngImpl.scalingMutex.Lock()
klog.V(3).Infof("%s has acquired scalingMutex of NodeGroup %q", operation, ngImpl.Name)
releaseFn = func() {
Expand Down Expand Up @@ -476,17 +476,17 @@ func getNodeNamesFromMachines(machines []*v1alpha1.Machine) []string {
}

// Id returns MachineDeployment id.
func (ngImpl *NodeGroupImpl) Id() string {
func (ngImpl *nodeGroup) Id() string {
return ngImpl.Name
}

// Debug returns a debug string for the Asg.
func (ngImpl *NodeGroupImpl) Debug() string {
func (ngImpl *nodeGroup) Debug() string {
return fmt.Sprintf("%s (%d:%d)", ngImpl.Id(), ngImpl.MinSize(), ngImpl.MaxSize())
}

// Nodes returns a list of all nodes that belong to this node group.
func (ngImpl *NodeGroupImpl) Nodes() ([]cloudprovider.Instance, error) {
func (ngImpl *nodeGroup) Nodes() ([]cloudprovider.Instance, error) {
instances, err := ngImpl.mcmManager.GetInstancesForMachineDeployment(ngImpl.Name)
if err != nil {
return nil, fmt.Errorf("failed to get the cloudprovider.Instance for machines backed by the MachineDeployment %q, error: %v", ngImpl.Name, err)
Expand All @@ -506,7 +506,7 @@ func (ngImpl *NodeGroupImpl) Nodes() ([]cloudprovider.Instance, error) {
// GetOptions returns NodeGroupAutoscalingOptions that should be used for this particular
// NodeGroup. Returning a nil will result in using default options.
// Implementation optional.
func (ngImpl *NodeGroupImpl) GetOptions(defaults config.NodeGroupAutoscalingOptions) (*config.NodeGroupAutoscalingOptions, error) {
func (ngImpl *nodeGroup) GetOptions(defaults config.NodeGroupAutoscalingOptions) (*config.NodeGroupAutoscalingOptions, error) {
options := defaults
mcdAnnotations, err := ngImpl.mcmManager.GetMachineDeploymentAnnotations(ngImpl.Name)
if err != nil {
Expand Down Expand Up @@ -542,7 +542,7 @@ func (ngImpl *NodeGroupImpl) GetOptions(defaults config.NodeGroupAutoscalingOpti
}

// TemplateNodeInfo returns a node template for this node group.
func (ngImpl *NodeGroupImpl) TemplateNodeInfo() (*schedulerframework.NodeInfo, error) {
func (ngImpl *nodeGroup) TemplateNodeInfo() (*schedulerframework.NodeInfo, error) {

nodeTemplate, err := ngImpl.mcmManager.GetMachineDeploymentNodeTemplate(ngImpl.Name)
if err != nil {
Expand All @@ -560,7 +560,7 @@ func (ngImpl *NodeGroupImpl) TemplateNodeInfo() (*schedulerframework.NodeInfo, e
}

// AtomicIncreaseSize is not implemented.
func (ngImpl *NodeGroupImpl) AtomicIncreaseSize(delta int) error {
func (ngImpl *nodeGroup) AtomicIncreaseSize(delta int) error {
return cloudprovider.ErrNotImplemented
}

Expand Down
8 changes: 4 additions & 4 deletions cluster-autoscaler/cloudprovider/mcm/mcm_cloud_provider_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func TestDeleteNodes(t *testing.T) {
trackers.ControlMachine.SetFailAtFakeResourceActions(entry.setup.controlMachineFakeResourceActions)
}

md, err := buildNodeGroupImplFromSpec(entry.setup.nodeGroups[0], m)
md, err := buildNodeGroupFromSpec(entry.setup.nodeGroups[0], m)
g.Expect(err).To(BeNil())

err = md.DeleteNodes([]*corev1.Node{entry.action.node})
Expand Down Expand Up @@ -322,7 +322,7 @@ func TestIdempotencyOfDeleteNodes(t *testing.T) {
m, trackers, hasSyncedCacheFns := createMcmManager(t, stop, testNamespace, setupObj.nodeGroups, controlMachineObjects, targetCoreObjects, nil)
defer trackers.Stop()
waitForCacheSync(t, stop, hasSyncedCacheFns)
md, err := buildNodeGroupImplFromSpec(setupObj.nodeGroups[0], m)
md, err := buildNodeGroupFromSpec(setupObj.nodeGroups[0], m)
g.Expect(err).To(BeNil())

err = md.DeleteNodes(newNodes(1, "fakeID"))
Expand Down Expand Up @@ -517,7 +517,7 @@ func TestNodes(t *testing.T) {
trackers.ControlMachine.SetFailAtFakeResourceActions(entry.setup.controlMachineFakeResourceActions)
}

md, err := buildNodeGroupImplFromSpec(entry.setup.nodeGroups[0], m)
md, err := buildNodeGroupFromSpec(entry.setup.nodeGroups[0], m)
g.Expect(err).To(BeNil())

returnedInstances, err := md.Nodes()
Expand Down Expand Up @@ -669,7 +669,7 @@ func TestGetOptions(t *testing.T) {
defer trackers.Stop()
waitForCacheSync(t, stop, hasSyncedCacheFns)

md, err := buildNodeGroupImplFromSpec(entry.setup.nodeGroups[0], m)
md, err := buildNodeGroupFromSpec(entry.setup.nodeGroups[0], m)
g.Expect(err).To(BeNil())

options, err := md.GetOptions(ngAutoScalingOpDefaults)
Expand Down
20 changes: 10 additions & 10 deletions cluster-autoscaler/cloudprovider/mcm/mcm_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ type McmManager struct {
namespace string
interrupt chan struct{}
discoveryOpts cloudprovider.NodeGroupDiscoveryOptions
nodeGroups map[types.NamespacedName]*NodeGroupImpl
nodeGroups map[types.NamespacedName]*nodeGroup
deploymentLister v1appslister.DeploymentLister
machineClient machineapi.MachineV1alpha1Interface
machineDeploymentLister machinelisters.MachineDeploymentLister
Expand Down Expand Up @@ -265,7 +265,7 @@ func createMCMManagerInternal(discoveryOpts cloudprovider.NodeGroupDiscoveryOpti
m := &McmManager{
namespace: namespace,
interrupt: make(chan struct{}),
nodeGroups: make(map[types.NamespacedName]*NodeGroupImpl),
nodeGroups: make(map[types.NamespacedName]*nodeGroup),
deploymentLister: deploymentLister,
machineClient: controlMachineClient,
machineClassLister: machineClassLister,
Expand Down Expand Up @@ -316,7 +316,7 @@ func (m *McmManager) generateMachineDeploymentMap() error {
// addNodeGroup adds node group defined in string spec. Format:
// minNodes:maxNodes:namespace.machineDeploymentName
func (m *McmManager) addNodeGroup(spec string) error {
nodeGroup, err := buildNodeGroupImplFromSpec(spec, m)
nodeGroup, err := buildNodeGroupFromSpec(spec, m)
if err != nil {
return err
}
Expand Down Expand Up @@ -384,8 +384,8 @@ func CreateMcmManager(discoveryOpts cloudprovider.NodeGroupDiscoveryOptions) (*M
return createMCMManagerInternal(discoveryOpts, defaultRetryInterval, defaultMaxRetryTimeout)
}

// GetNodeGroupImpl returns the NodeGroupImpl for the given fully-qualified machine name.
func (m *McmManager) GetNodeGroupImpl(machineKey types.NamespacedName) (*NodeGroupImpl, error) {
// getNodeGroup returns the NodeGroup for the given fully-qualified machine name.
func (m *McmManager) getNodeGroup(machineKey types.NamespacedName) (*nodeGroup, error) {
if machineKey.Name == "" {
// Considering the possibility when Machine has been deleted but due to cached Node object it appears here.
return nil, fmt.Errorf("node does not Exists")
Expand Down Expand Up @@ -452,7 +452,7 @@ func (m *McmManager) GetMachineDeploymentSize(nodeGroupName string) (int64, erro
}

// SetMachineDeploymentSize sets the desired size for the backing MachineDeployment of the given nodeGroup.
func (m *McmManager) SetMachineDeploymentSize(ctx context.Context, nodeGroup *NodeGroupImpl, size int64) (bool, error) {
func (m *McmManager) SetMachineDeploymentSize(ctx context.Context, nodeGroup *nodeGroup, size int64) (bool, error) {
md, err := m.GetMachineDeploymentObject(nodeGroup.Name)
if err != nil {
return true, err
Expand Down Expand Up @@ -1039,19 +1039,19 @@ func buildGenericLabels(template *nodeTemplate, nodeName string) map[string]stri
return result
}

func buildNodeGroupImplFromSpec(value string, mcmManager *McmManager) (*NodeGroupImpl, error) {
func buildNodeGroupFromSpec(value string, mcmManager *McmManager) (*nodeGroup, error) {
spec, err := dynamic.SpecFromString(value, true)
if err != nil {
return nil, fmt.Errorf("failed to parse node group spec: %v", err)
}
s := strings.Split(spec.Name, ".")
Namespace, Name := s[0], s[1]
nodeGroup := buildNodeGroupImpl(mcmManager, spec.MinSize, spec.MaxSize, Namespace, Name)
nodeGroup := buildNodeGroup(mcmManager, spec.MinSize, spec.MaxSize, Namespace, Name)
return nodeGroup, nil
}

func buildNodeGroupImpl(mcmManager *McmManager, minSize int, maxSize int, namespace string, name string) *NodeGroupImpl {
return &NodeGroupImpl{
func buildNodeGroup(mcmManager *McmManager, minSize int, maxSize int, namespace string, name string) *nodeGroup {
return &nodeGroup{
mcmManager: mcmManager,
minSize: minSize,
maxSize: maxSize,
Expand Down
4 changes: 2 additions & 2 deletions cluster-autoscaler/cloudprovider/mcm/test_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func newMachineDeployments(
machineDeployment := &v1alpha1.MachineDeployment{
TypeMeta: metav1.TypeMeta{
APIVersion: "machine.sapcloud.io",
Kind: "NodeGroupImpl",
Kind: "MachineDeployment",
},
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("machinedeployment-%d", i+1),
Expand Down Expand Up @@ -282,7 +282,7 @@ func createMcmManager(
discoveryOpts: cloudprovider.NodeGroupDiscoveryOptions{
NodeGroupSpecs: nodeGroups,
},
nodeGroups: make(map[types.NamespacedName]*NodeGroupImpl),
nodeGroups: make(map[types.NamespacedName]*nodeGroup),
deploymentLister: appsControlSharedInformers.Deployments().Lister(),
machineClient: fakeTypedMachineClient,
machineDeploymentLister: machineDeployments.Lister(),
Expand Down

0 comments on commit 7d1d0d7

Please sign in to comment.