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

HPA doesn't work with operator v0.0.5 #160

Closed
connie-ru-wang opened this issue Apr 8, 2021 · 3 comments · Fixed by #163
Closed

HPA doesn't work with operator v0.0.5 #160

connie-ru-wang opened this issue Apr 8, 2021 · 3 comments · Fixed by #163

Comments

@connie-ru-wang
Copy link

Using an HPA with operator v0.0.5 doesn't work because the operator immediately tries to reconcile the deployment/statefulset back to the original replica count. Most likely due to PR #115

The HPA works as expected with operator v0.0.4.

Sample HPA spec from druid cluster.yaml:

      hpAutoscaler:
        maxReplicas: 6
        minReplicas: 3
        scaleTargetRef:
           apiVersion: apps/v1
           kind: Deployment
           name: druid-cluster-brokers
        metrics:
         - type: Resource
           resource:
            name: cpu
            targetAverageUtilization: 50
@AdheipSingh
Copy link
Contributor

@himanshug in order to fix this i see this option as the simplest check, the logic for checkin if hpa spec exists can be checked at multiple places ex: inside the sdkCreateUpdate method, or in the main handler func().

Since as of now we arn't forcing any other update apart from replica this felt simple though this requires the func() obj to be tweaked to accept nodeSpec too. ie isEqualFn func(prev, curr object, nodeSpec *v1alpha1.DruidNodeSpec) bool

Let me know if you have a better approach.

// currently only checks for replica count mismatch, can be extended further
func statefulSetIsEquals(obj1, obj2 object, nodeSpec *v1alpha1.DruidNodeSpec) bool {
	if nodeSpec.HPAutoScaler != nil {
		return false
	} else {
		o1 := obj1.(*appsv1.StatefulSet)
		o2 := obj2.(*appsv1.StatefulSet)
		return *o1.Spec.Replicas == *o2.Spec.Replicas
	}
}

@himanshug
Copy link
Member

hmmm... my first thought is, I was skeptical of introducing that behavior, one simple option is to revert #115 and assume that user is not expected to manually change things setup by operator.

It seems like complexity for not a major benefit.

@AdheipSingh
Copy link
Contributor

@himanshug
can we just keep in pass noopUpdaterFunc() for now and keep the framework you introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants