Skip to content
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

Bug 1906933: Add upstream patch for update group identifier #184

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cluster-autoscaler/cloudprovider/clusterapi/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,12 @@ resources depending on the type of cluster-api mechanism that you are using.

There are two annotations that control how a cluster resource should be scaled:

* `cluster.k8s.io/cluster-api-autoscaler-node-group-min-size` - This specifies
* `cluster.x-k8s.io/cluster-api-autoscaler-node-group-min-size` - This specifies
the minimum number of nodes for the associated resource group. The autoscaler
will not scale the group below this number. Please note that currently the
cluster-api provider will not scale down to zero nodes.

* `cluster.k8s.io/cluster-api-autoscaler-node-group-max-size` - This specifies
* `cluster.x-k8s.io/cluster-api-autoscaler-node-group-max-size` - This specifies
the maximum number of nodes for the associated resource group. The autoscaler
will not scale the group above this number.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,12 @@ func (c *machineController) findMachineByProviderID(providerID normalizedProvide
if node == nil {
return nil, nil
}
return c.findMachine(node.Annotations[machineAnnotationKey])

machineID, ok := node.Annotations[machineAnnotationKey]
if !ok {
machineID = node.Annotations[deprecatedMachineAnnotationKey]
}
return c.findMachine(machineID)
}

func isFailedMachineProviderID(providerID normalizedProviderID) bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -514,24 +514,32 @@ func deleteTestConfigs(t *testing.T, controller *machineController, testConfigs

func TestControllerFindMachine(t *testing.T) {
type testCase struct {
description string
name string
namespace string
lookupSucceeds bool
description string
name string
namespace string
useDeprecatedAnnotation bool
lookupSucceeds bool
}

var testCases = []testCase{{
description: "lookup fails",
lookupSucceeds: false,
name: "machine-does-not-exist",
namespace: "namespace-does-not-exist",
description: "lookup fails",
lookupSucceeds: false,
useDeprecatedAnnotation: false,
name: "machine-does-not-exist",
namespace: "namespace-does-not-exist",
}, {
description: "lookup fails in valid namespace",
lookupSucceeds: false,
name: "machine-does-not-exist-in-existing-namespace",
description: "lookup fails in valid namespace",
lookupSucceeds: false,
useDeprecatedAnnotation: false,
name: "machine-does-not-exist-in-existing-namespace",
}, {
description: "lookup succeeds",
lookupSucceeds: true,
description: "lookup succeeds",
lookupSucceeds: true,
useDeprecatedAnnotation: false,
}, {
description: "lookup succeeds with deprecated annotation",
lookupSucceeds: true,
useDeprecatedAnnotation: true,
}}

test := func(t *testing.T, tc testCase, testConfig *testConfig) {
Expand Down Expand Up @@ -570,6 +578,19 @@ func TestControllerFindMachine(t *testing.T) {
if tc.namespace == "" {
tc.namespace = testConfig.machines[0].GetNamespace()
}
if tc.useDeprecatedAnnotation {
for i := range testConfig.machines {
n := testConfig.nodes[i]
annotations := n.GetAnnotations()
val, ok := annotations[machineAnnotationKey]
if !ok {
t.Fatal("node did not contain machineAnnotationKey")
}
delete(annotations, machineAnnotationKey)
annotations[deprecatedMachineAnnotationKey] = val
n.SetAnnotations(annotations)
}
}
test(t, tc, testConfig)
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ const (
// This default for the maximum number of pods comes from the machine-config-operator
// see https://github.com/openshift/machine-config-operator/blob/2f1bd6d99131fa4471ed95543a51dec3d5922b2b/templates/worker/01-worker-kubelet/_base/files/kubelet.yaml#L19
defaultMaxPods = 250

// deprecatedMachineDeleteAnnotationKey should not be removed until minimum cluster-api support is v1alpha3
deprecatedMachineDeleteAnnotationKey = "cluster.k8s.io/delete-machine"
// TODO: determine what currently relies on deprecatedMachineAnnotationKey to determine when it can be removed
deprecatedMachineAnnotationKey = "cluster.k8s.io/machine"
)

type nodegroup struct {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,9 @@ func TestNodeGroupDeleteNodes(t *testing.T) {
if _, found := machine.GetAnnotations()[machineDeleteAnnotationKey]; !found {
t.Errorf("expected annotation %q on machine %s", machineDeleteAnnotationKey, machine.GetName())
}
if _, found := machine.GetAnnotations()[deprecatedMachineDeleteAnnotationKey]; !found {
t.Errorf("expected annotation %q on machine %s", deprecatedMachineDeleteAnnotationKey, machine.GetName())
}
}

gvr, err := ng.scalableResource.GroupVersionResource()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,12 @@ func (r unstructuredScalableResource) UnmarkMachineForDeletion(machine *unstruct
}

annotations := u.GetAnnotations()
if _, ok := annotations[machineDeleteAnnotationKey]; ok {
delete(annotations, machineDeleteAnnotationKey)
u.SetAnnotations(annotations)
_, updateErr := r.controller.managementClient.Resource(r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{})

return updateErr
}
delete(annotations, machineDeleteAnnotationKey)
delete(annotations, deprecatedMachineDeleteAnnotationKey)
u.SetAnnotations(annotations)
_, updateErr := r.controller.managementClient.Resource(r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{})

return nil
return updateErr
}

func (r unstructuredScalableResource) MarkMachineForDeletion(machine *unstructured.Unstructured) error {
Expand All @@ -156,6 +153,7 @@ func (r unstructuredScalableResource) MarkMachineForDeletion(machine *unstructur
}

annotations[machineDeleteAnnotationKey] = time.Now().String()
annotations[deprecatedMachineDeleteAnnotationKey] = time.Now().String()
u.SetAnnotations(annotations)

_, updateErr := r.controller.managementClient.Resource(r.controller.machineResource).Namespace(u.GetNamespace()).Update(context.TODO(), u, metav1.UpdateOptions{})
Expand Down
16 changes: 12 additions & 4 deletions cluster-autoscaler/cloudprovider/clusterapi/clusterapi_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@ import (
)

const (
nodeGroupMinSizeAnnotationKey = "machine.openshift.io/cluster-api-autoscaler-node-group-min-size"
nodeGroupMaxSizeAnnotationKey = "machine.openshift.io/cluster-api-autoscaler-node-group-max-size"
clusterNameLabel = "machine.openshift.io/cluster-name"
deprecatedClusterNameLabel = "cluster.k8s.io/cluster-name"
deprecatedNodeGroupMinSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-min-size"
deprecatedNodeGroupMaxSizeAnnotationKey = "cluster.k8s.io/cluster-api-autoscaler-node-group-max-size"
nodeGroupMinSizeAnnotationKey = "machine.openshift.io/cluster-api-autoscaler-node-group-min-size"
nodeGroupMaxSizeAnnotationKey = "machine.openshift.io/cluster-api-autoscaler-node-group-max-size"
clusterNameLabel = "machine.openshift.io/cluster-name"
deprecatedClusterNameLabel = "cluster.k8s.io/cluster-name"

cpuKey = "machine.openshift.io/vCPU"
memoryKey = "machine.openshift.io/memoryMb"
Expand Down Expand Up @@ -70,6 +72,9 @@ type normalizedProviderID string
// value is not of type int.
func minSize(annotations map[string]string) (int, error) {
val, found := annotations[nodeGroupMinSizeAnnotationKey]
if !found {
val, found = annotations[deprecatedNodeGroupMinSizeAnnotationKey]
}
if !found {
return 0, errMissingMinAnnotation
}
Expand All @@ -86,6 +91,9 @@ func minSize(annotations map[string]string) (int, error) {
// value is not of type int.
func maxSize(annotations map[string]string) (int, error) {
val, found := annotations[nodeGroupMaxSizeAnnotationKey]
if !found {
val, found = annotations[deprecatedNodeGroupMaxSizeAnnotationKey]
}
if !found {
return 0, errMissingMaxAnnotation
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,24 @@ func TestUtilParseScalingBounds(t *testing.T) {
},
min: 0,
max: 1,
}, {
description: "deprecated min/max annotations still work, result is min 0, max 1",
annotations: map[string]string{
deprecatedNodeGroupMinSizeAnnotationKey: "0",
deprecatedNodeGroupMaxSizeAnnotationKey: "1",
},
min: 0,
max: 1,
}, {
description: "deprecated min/max annotations do not take precedence over non-deprecated annotations, result is min 1, max 2",
annotations: map[string]string{
deprecatedNodeGroupMinSizeAnnotationKey: "0",
deprecatedNodeGroupMaxSizeAnnotationKey: "1",
nodeGroupMinSizeAnnotationKey: "1",
nodeGroupMaxSizeAnnotationKey: "2",
},
min: 1,
max: 2,
}} {
t.Run(tc.description, func(t *testing.T) {
machineSet := unstructured.Unstructured{
Expand Down