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.

Sign up for GitHub

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

Jump to bottom

fixed autoscaling bug and updated hpa sample #270

Closed
wants to merge 3 commits into from
Closed

fixed autoscaling bug and updated hpa sample #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1706ec
fixed autoscaling bug and updated hpa sample
asteurer Jul 9, 2024
3566081
made some small refactors
asteurer Jul 10, 2024
7a431f0
removing requirement for minimum replica count
asteurer Jul 10, 2024
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
12 changes: 4 additions & 8 deletions api/v1alpha1/spinapp_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,6 @@ type SpinAppSpec struct {
// Number of replicas to run.
Replicas int32 `json:"replicas,omitempty"`

// EnableAutoscaling indicates whether the app is allowed to autoscale. If
// true then the operator leaves the replica count of the underlying
// deployment to be managed by an external autoscaler (HPA/KEDA). Replicas
// cannot be defined if this is enabled. By default EnableAutoscaling is false.
//
// +kubebuilder:default:=false
EnableAutoscaling bool `json:"enableAutoscaling,omitempty"`

// RuntimeConfig defines configuration to be applied at runtime for this app.
RuntimeConfig RuntimeConfig `json:"runtimeConfig,omitempty"`

Expand Down Expand Up @@ -99,6 +91,9 @@ type SpinAppStatus struct {

// Represents the current number of active replicas on the application deployment.
ReadyReplicas int32 `json:"readyReplicas"`

// Selector for the underlying pods
Selector string `json:"selector"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@endocrimes would we have to make this optional to support runtimes where this selector is meaningless? Or is setting it to the empty string sufficient.

}

// SpinApp is the Schema for the spinapps API
Expand All @@ -108,6 +103,7 @@ type SpinAppStatus struct {
// +kubebuilder:printcolumn:JSONPath=".status.readyReplicas",name=Ready,type=integer
// +kubebuilder:printcolumn:JSONPath=".spec.replicas",name=Desired,type=integer
// +kubebuilder:printcolumn:JSONPath=".spec.executor",name=Executor,type=string
// +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.replicas,selectorpath=.status.selector
type SpinApp struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Expand Down
16 changes: 8 additions & 8 deletions config/crd/bases/core.spinoperator.dev_spinapps.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,14 +194,6 @@ spec:
description: DeploymentAnnotations defines annotations to be applied
to the underlying deployment.
type: object
enableAutoscaling:
default: false
description: |-
EnableAutoscaling indicates whether the app is allowed to autoscale. If
true then the operator leaves the replica count of the underlying
deployment to be managed by an external autoscaler (HPA/KEDA). Replicas
cannot be defined if this is enabled. By default EnableAutoscaling is false.
type: boolean
executor:
description: |-
Executor controls how this app is executed in the cluster.
Expand Down Expand Up @@ -2577,11 +2569,19 @@ spec:
application deployment.
format: int32
type: integer
selector:
description: Selector for the underlying pods
type: string
required:
- readyReplicas
- selector
type: object
type: object
served: true
storage: true
subresources:
scale:
labelSelectorPath: .status.selector
specReplicasPath: .spec.replicas
statusReplicasPath: .status.replicas
status: {}
6 changes: 3 additions & 3 deletions config/samples/hpa.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
spec:
image: ghcr.io/spinkube/spin-operator/cpu-load-gen:20240311-163328-g1121986
executor: containerd-shim-spin
enableAutoscaling: true
replicas: 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bacongobbler says:

Shouldn't this be removed if both minReplicas and maxReplicas are set?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asteurer ?

resources:
limits:
cpu: 500m
Expand All @@ -20,8 +20,8 @@ metadata:
name: spinapp-autoscaler
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
apiVersion: core.spinoperator.dev/v1alpha1
kind: SpinApp
name: hpa-spinapp
minReplicas: 1
maxReplicas: 10
Expand Down
21 changes: 12 additions & 9 deletions internal/controller/spinapp_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"hash/adler32"
"maps"
"strings"

"github.com/pelletier/go-toml/v2"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -187,6 +188,8 @@ func (r *SpinAppReconciler) updateStatus(ctx context.Context, app *spinv1alpha1.
Reason: "DeploymentNotFound",
Message: "Deployment not found",
})

app.Status.Selector = ""
app.Status.ReadyReplicas = 0
} else {
deploymentConditions := deployment.Status.Conditions
Expand All @@ -212,6 +215,13 @@ func (r *SpinAppReconciler) updateStatus(ctx context.Context, app *spinv1alpha1.
})
}
}

var selectorStringArray []string
for key, value := range deployment.Spec.Selector.MatchLabels {
selectorStringArray = append(selectorStringArray, key+"="+value)
}

app.Status.Selector = strings.Join(selectorStringArray, ",")
app.Status.ReadyReplicas = deployment.Status.ReadyReplicas
}

Expand Down Expand Up @@ -381,14 +391,7 @@ func (r *SpinAppReconciler) deleteDeployment(ctx context.Context, app *spinv1alp
// constructDeployment builds an appsv1.Deployment based on the configuration of a SpinApp.
func constructDeployment(ctx context.Context, app *spinv1alpha1.SpinApp, config *spinv1alpha1.ExecutorDeploymentConfig,
generatedRuntimeConfigSecretName, caSecretName string, scheme *runtime.Scheme) (*appsv1.Deployment, error) {
// TODO: Once we land admission webhooks write some validation to make
// replicas and enableAutoscaling mutually exclusive.
var replicas *int32
if app.Spec.EnableAutoscaling {
replicas = nil
} else {
replicas = ptr(app.Spec.Replicas)
}
replicas := app.Spec.Replicas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bacongobbler says:

related to the above comment RE: min/maxReplicas, shouldn't this be nil if either of those are set?

If only minReplicas is set (is that valid?), should this be set to minReplicas as a starting value?
If only maxReplicas is set, should this be set to 1 (or possibly nil, assuming we can scale to zero)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asteurer ?


volumes, volumeMounts, err := ConstructVolumeMountsForApp(ctx, app, generatedRuntimeConfigSecretName, caSecretName)
if err != nil {
Expand Down Expand Up @@ -444,7 +447,7 @@ func constructDeployment(ctx context.Context, app *spinv1alpha1.SpinApp, config
Annotations: annotations,
},
Spec: appsv1.DeploymentSpec{
Replicas: replicas,
Replicas: &replicas,
Selector: &metav1.LabelSelector{
MatchLabels: readyLabels,
},
Expand Down
14 changes: 0 additions & 14 deletions internal/webhook/spinapp_validating.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,6 @@ func (v *SpinAppValidator) validateSpinApp(ctx context.Context, spinApp *spinv1a
if err != nil {
allErrs = append(allErrs, err)
}
if err := validateReplicas(spinApp.Spec); err != nil {
allErrs = append(allErrs, err)
}
if err := validateAnnotations(spinApp.Spec, executor); err != nil {
allErrs = append(allErrs, err)
}
Expand Down Expand Up @@ -103,17 +100,6 @@ func validateExecutor(spec spinv1alpha1.SpinAppSpec, fetchExecutor func(name str
return executor, nil
}

func validateReplicas(spec spinv1alpha1.SpinAppSpec) *field.Error {
if spec.EnableAutoscaling && spec.Replicas != 0 {
return field.Invalid(field.NewPath("spec").Child("replicas"), spec.Replicas, "replicas cannot be set when autoscaling is enabled")
}
if !spec.EnableAutoscaling && spec.Replicas < 1 {
return field.Invalid(field.NewPath("spec").Child("replicas"), spec.Replicas, "replicas must be > 0")
}

return nil
}

func validateAnnotations(spec spinv1alpha1.SpinAppSpec, executor *spinv1alpha1.SpinAppExecutor) *field.Error {
// We can't do any validation if the executor isn't available, but validation
// will fail because of earlier errors.
Expand Down
10 changes: 0 additions & 10 deletions internal/webhook/spinapp_validating_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,6 @@ func TestValidateExecutor(t *testing.T) {
require.Nil(t, fldErr)
}

func TestValidateReplicas(t *testing.T) {
t.Parallel()

fldErr := validateReplicas(spinv1alpha1.SpinAppSpec{})
require.EqualError(t, fldErr, "spec.replicas: Invalid value: 0: replicas must be > 0")

fldErr = validateReplicas(spinv1alpha1.SpinAppSpec{Replicas: 1})
require.Nil(t, fldErr)
}

func TestValidateAnnotations(t *testing.T) {
t.Parallel()

Expand Down