Simultaneously support versions v2 and v2beta2 of Autoscaling

[kevinearls]

Signed-off-by: Kevin Earls [email protected]

This is to resolve #943. Choose which version of autoscaling to use depending on what is available at runtime. We need to be able to support both v2 and v2beta2

[kevinearls]

@pavolloffay Please review. BTW should I need to ask for a review if it already says "open-telemetry/operator-approvers was requested for review". I don't want to be spamming you unnecessarily.

[pavolloffay]

Returning this just like this does not seem right. It should be returned only if that version is present in the cluster.

[pavolloffay]

can we dry this method? I don't like that there is too much repetition/copy-paste for two different versions

[pavolloffay]

Something like the following could work:

func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error {
	autoscalingVersion := params.Config.AutoscalingVersion()
	var existing client.Object
	if autoscalingVersion == config.AutoscalingVersionV2Beta2 {
		existing = &autoscalingv2beta2.HorizontalPodAutoscaler{}
	} else {
		existing = &autoscalingv2.HorizontalPodAutoscaler{}
	}

	for _, obj := range expected {
		desired, _ := meta.Accessor(obj)

		if err := controllerutil.SetControllerReference(&params.Instance, desired, params.Scheme); err != nil {
			return fmt.Errorf("failed to set controller reference: %w", err)
		}

		nns := types.NamespacedName{Namespace: desired.GetNamespace(), Name: desired.GetName()}
		err := params.Client.Get(ctx, nns, existing)
		if k8serrors.IsNotFound(err) {
			if err := params.Client.Create(ctx, obj.(client.Object)); err != nil {
				return fmt.Errorf("failed to create: %w", err)
			}
			params.Log.V(2).Info("created", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace())
			continue
		} else if err != nil {
			return fmt.Errorf("failed to get %w", err)
		}

		existing.SetOwnerReferences(desired.GetOwnerReferences())
		setAutoscalerSpec(params.Instance, existing)

		annos := existing.GetAnnotations()
		for k, v := range desired.GetAnnotations() {
			annos[k] = v
		}
		existing.SetAnnotations(annos)
		labels := existing.GetLabels()
		for k, v := range desired.GetLabels() {
			labels[k] = v
		}
		existing.SetLabels(labels)

		patch := client.MergeFrom(existing)

		if err := params.Client.Patch(ctx, existing, patch); err != nil {
			return fmt.Errorf("failed to apply changes: %w", err)
		}

		params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace)
	}

	return nil
}

[pavolloffay]

@kevinearls did you try this de-duplication?

[pavolloffay]

We could include which versions are "appropriate".

[pavolloffay]

instread of returning runtime.Object wouldn't it be better to return client.Object? That way we don't have to cast this in the reconcile.

[pavolloffay]

no need for else branch

[pavolloffay]

@kevinearls did you try this de-duplication?

[kevinearls]

@pavolloffay Yes, I have now added the de duplication code. The original code you posted required some changes.

[pavolloffay]

nit: define these as enum (a golang type)

[kevinearls]

@pavolloffay Can you point me to an example of what you want here? Googling "golang enums" returns a bunch of disparate answers

[kevinearls]

@pavolloffay Can you review this? I'd like to get this merged before you go on PTO.

[pavolloffay]

nit: move this closer to the place where it is used - line 192

[pavolloffay]

nit: remove todo?