Skip to content

Commit

Permalink
feat: update crp default func (Azure#734)
Browse files Browse the repository at this point in the history
  • Loading branch information
britaniar authored Apr 10, 2024
1 parent 649696e commit 8ef044a
Show file tree
Hide file tree
Showing 7 changed files with 255 additions and 61 deletions.
12 changes: 0 additions & 12 deletions apis/placement/v1beta1/clusterresourceplacement_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,9 @@ const (
// that the CRP controller can react to CRP deletions if necessary.
ClusterResourcePlacementCleanupFinalizer = fleetPrefix + "crp-cleanup"

// RevisionHistoryLimitDefaultValue is the default value of RevisionHistoryLimit.
RevisionHistoryLimitDefaultValue = int32(10)

// SchedulerCRPCleanupFinalizer is a finalizer addd by the scheduler to CRPs, to make sure
// that all bindings derived from a CRP can be cleaned up after the CRP is deleted.
SchedulerCRPCleanupFinalizer = fleetPrefix + "scheduler-cleanup"

// DefaultMaxUnavailableValue is the default value of MaxUnavailable in the rolling update config.
DefaultMaxUnavailableValue = "25%"

// DefaultMaxSurgeValue is the default value of MaxSurge in the rolling update config.
DefaultMaxSurgeValue = "25%"

// DefaultUnavailablePeriodSeconds is the default period of time we consider a newly applied workload as unavailable.
DefaultUnavailablePeriodSeconds = 60
)

// +genclient
Expand Down
45 changes: 0 additions & 45 deletions apis/placement/v1beta1/default.go

This file was deleted.

3 changes: 2 additions & 1 deletion pkg/controllers/clusterresourceplacement/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"go.goms.io/fleet/pkg/utils/annotations"
"go.goms.io/fleet/pkg/utils/condition"
"go.goms.io/fleet/pkg/utils/controller"
"go.goms.io/fleet/pkg/utils/defaulter"
"go.goms.io/fleet/pkg/utils/labels"
"go.goms.io/fleet/pkg/utils/resource"
)
Expand Down Expand Up @@ -141,7 +142,7 @@ func (r *Reconciler) deleteClusterResourceSnapshots(ctx context.Context, crp *fl
// clusterSchedulingPolicySnapshot status and work status.
// If the error type is ErrUnexpectedBehavior, the controller will skip the reconciling.
func (r *Reconciler) handleUpdate(ctx context.Context, crp *fleetv1beta1.ClusterResourcePlacement) (ctrl.Result, error) {
revisionLimit := fleetv1beta1.RevisionHistoryLimitDefaultValue
revisionLimit := int32(defaulter.DefaultRevisionHistoryLimitValue)
crpKObj := klog.KObj(crp)
oldCRP := crp.DeepCopy()
if crp.Spec.RevisionHistoryLimit != nil {
Expand Down
5 changes: 3 additions & 2 deletions pkg/controllers/clusterresourceplacement/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
"go.goms.io/fleet/pkg/utils/controller"
"go.goms.io/fleet/pkg/utils/defaulter"
"go.goms.io/fleet/test/utils/resource"
)

Expand Down Expand Up @@ -727,7 +728,7 @@ func TestGetOrCreateClusterSchedulingPolicySnapshot(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
}
limit := fleetv1beta1.RevisionHistoryLimitDefaultValue
limit := int32(defaulter.DefaultRevisionHistoryLimitValue)
if tc.revisionHistoryLimit != nil {
limit = *tc.revisionHistoryLimit
}
Expand Down Expand Up @@ -2360,7 +2361,7 @@ func TestGetOrCreateClusterResourceSnapshot(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(10),
}
limit := fleetv1beta1.RevisionHistoryLimitDefaultValue
limit := int32(defaulter.DefaultRevisionHistoryLimitValue)
if tc.revisionHistoryLimit != nil {
limit = *tc.revisionHistoryLimit
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/controllers/rollout/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
"go.goms.io/fleet/pkg/utils/condition"
"go.goms.io/fleet/pkg/utils/controller"
"go.goms.io/fleet/pkg/utils/defaulter"
"go.goms.io/fleet/pkg/utils/informer"
"go.goms.io/fleet/pkg/utils/validator"
)
Expand Down Expand Up @@ -122,7 +123,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim
klog.V(2).InfoS("Found the latest resourceSnapshot for the clusterResourcePlacement", "clusterResourcePlacement", crpName, "latestResourceSnapshot", klog.KObj(latestResourceSnapshot))

// fill out all the default values for CRP just in case the mutation webhook is not enabled.
fleetv1beta1.SetDefaultsClusterResourcePlacement(&crp)
defaulter.SetDefaultsClusterResourcePlacement(&crp)
// validate the clusterResourcePlacement just in case the validation webhook is not enabled
if err = validator.ValidateClusterResourcePlacement(&crp); err != nil {
klog.ErrorS(err, "Encountered an invalid clusterResourcePlacement", "clusterResourcePlacement", crpName)
Expand Down
95 changes: 95 additions & 0 deletions pkg/utils/defaulter/clusterresourceplacement.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/*
Copyright (c) Microsoft Corporation.
Licensed under the MIT license.
*/

// Package defaulter is an interface for setting default values for a resource.
package defaulter

import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"

fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
)

const (
// DefaultMaxUnavailableValue is the default value of MaxUnavailable in the rolling update config.
DefaultMaxUnavailableValue = "25%"

// DefaultMaxSurgeValue is the default value of MaxSurge in the rolling update config.
DefaultMaxSurgeValue = "25%"

// DefaultUnavailablePeriodSeconds is the default period of time we consider a newly applied workload as unavailable.
DefaultUnavailablePeriodSeconds = 60

// DefaultMaxSkewValue is the default degree to which resources may be unevenly distributed.
DefaultMaxSkewValue = 1

// DefaultRevisionHistoryLimitValue is the default value of RevisionHistoryLimit.
DefaultRevisionHistoryLimitValue = 10
)

// SetDefaultsClusterResourcePlacement sets the default values for ClusterResourcePlacement.
func SetDefaultsClusterResourcePlacement(obj *fleetv1beta1.ClusterResourcePlacement) {
if obj.Spec.Policy == nil {
obj.Spec.Policy = &fleetv1beta1.PlacementPolicy{
PlacementType: fleetv1beta1.PickAllPlacementType,
}
}

if obj.Spec.Policy.TopologySpreadConstraints != nil {
for i := range obj.Spec.Policy.TopologySpreadConstraints {
if obj.Spec.Policy.TopologySpreadConstraints[i].MaxSkew == nil {
obj.Spec.Policy.TopologySpreadConstraints[i].MaxSkew = ptr.To(int32(DefaultMaxSkewValue))
}
if obj.Spec.Policy.TopologySpreadConstraints[i].WhenUnsatisfiable == "" {
obj.Spec.Policy.TopologySpreadConstraints[i].WhenUnsatisfiable = fleetv1beta1.DoNotSchedule
}
}
}

if obj.Spec.Policy.Tolerations != nil {
for i := range obj.Spec.Policy.Tolerations {
if obj.Spec.Policy.Tolerations[i].Operator == "" {
obj.Spec.Policy.Tolerations[i].Operator = corev1.TolerationOpEqual
}
}
}

strategy := &obj.Spec.Strategy
if strategy.Type == "" {
strategy.Type = fleetv1beta1.RollingUpdateRolloutStrategyType
}
if strategy.Type == fleetv1beta1.RollingUpdateRolloutStrategyType {
if strategy.RollingUpdate == nil {
strategy.RollingUpdate = &fleetv1beta1.RollingUpdateConfig{}
}
if strategy.RollingUpdate.MaxUnavailable == nil {
strategy.RollingUpdate.MaxUnavailable = ptr.To(intstr.FromString(DefaultMaxUnavailableValue))
}
if strategy.RollingUpdate.MaxSurge == nil {
strategy.RollingUpdate.MaxSurge = ptr.To(intstr.FromString(DefaultMaxSurgeValue))
}
if strategy.RollingUpdate.UnavailablePeriodSeconds == nil {
strategy.RollingUpdate.UnavailablePeriodSeconds = ptr.To(DefaultUnavailablePeriodSeconds)
}
}

if obj.Spec.Strategy.ApplyStrategy == nil {
obj.Spec.Strategy.ApplyStrategy = &fleetv1beta1.ApplyStrategy{}
}
if obj.Spec.Strategy.ApplyStrategy.Type == "" {
obj.Spec.Strategy.ApplyStrategy.Type = fleetv1beta1.ApplyStrategyTypeClientSideApply
}
if obj.Spec.Strategy.ApplyStrategy.Type == fleetv1beta1.ApplyStrategyTypeServerSideApply && obj.Spec.Strategy.ApplyStrategy.ServerSideApplyConfig == nil {
obj.Spec.Strategy.ApplyStrategy.ServerSideApplyConfig = &fleetv1beta1.ServerSideApplyConfig{
ForceConflicts: false,
}
}

if obj.Spec.RevisionHistoryLimit == nil {
obj.Spec.RevisionHistoryLimit = ptr.To(int32(DefaultRevisionHistoryLimitValue))
}
}
153 changes: 153 additions & 0 deletions pkg/utils/defaulter/clusterresourceplacement_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
/*
Copyright (c) Microsoft Corporation.
Licensed under the MIT license.
*/

package defaulter

import (
"testing"

"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/ptr"

fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
)

func TestSetDefaultsClusterResourcePlacement(t *testing.T) {
tests := map[string]struct {
obj *fleetv1beta1.ClusterResourcePlacement
wantObj *fleetv1beta1.ClusterResourcePlacement
}{
"ClusterResourcePlacement with nil Spec": {
obj: &fleetv1beta1.ClusterResourcePlacement{
Spec: fleetv1beta1.ClusterResourcePlacementSpec{},
},
wantObj: &fleetv1beta1.ClusterResourcePlacement{
Spec: fleetv1beta1.ClusterResourcePlacementSpec{
Policy: &fleetv1beta1.PlacementPolicy{
PlacementType: fleetv1beta1.PickAllPlacementType,
},
Strategy: fleetv1beta1.RolloutStrategy{
Type: fleetv1beta1.RollingUpdateRolloutStrategyType,
RollingUpdate: &fleetv1beta1.RollingUpdateConfig{
MaxUnavailable: ptr.To(intstr.FromString(DefaultMaxUnavailableValue)),
MaxSurge: ptr.To(intstr.FromString(DefaultMaxSurgeValue)),
UnavailablePeriodSeconds: ptr.To(DefaultUnavailablePeriodSeconds),
},
ApplyStrategy: &fleetv1beta1.ApplyStrategy{
Type: fleetv1beta1.ApplyStrategyTypeClientSideApply,
},
},
RevisionHistoryLimit: ptr.To(int32(DefaultRevisionHistoryLimitValue)),
},
},
},
"ClusterResourcePlacement with nil TopologySpreadConstraints & Tolerations fields": {
obj: &fleetv1beta1.ClusterResourcePlacement{
Spec: fleetv1beta1.ClusterResourcePlacementSpec{
Policy: &fleetv1beta1.PlacementPolicy{
TopologySpreadConstraints: []fleetv1beta1.TopologySpreadConstraint{
{
TopologyKey: "kubernetes.io/hostname",
},
},
Tolerations: []fleetv1beta1.Toleration{
{
Key: "key",
Value: "value",
},
},
},
Strategy: fleetv1beta1.RolloutStrategy{
Type: fleetv1beta1.RollingUpdateRolloutStrategyType,
RollingUpdate: &fleetv1beta1.RollingUpdateConfig{
MaxUnavailable: ptr.To(intstr.FromString("%15")),
MaxSurge: ptr.To(intstr.FromString("%15")),
UnavailablePeriodSeconds: ptr.To(15),
},
ApplyStrategy: &fleetv1beta1.ApplyStrategy{
Type: fleetv1beta1.ApplyStrategyTypeClientSideApply,
},
},
RevisionHistoryLimit: ptr.To(int32(10)),
},
},
wantObj: &fleetv1beta1.ClusterResourcePlacement{
Spec: fleetv1beta1.ClusterResourcePlacementSpec{
Policy: &fleetv1beta1.PlacementPolicy{
TopologySpreadConstraints: []fleetv1beta1.TopologySpreadConstraint{
{
TopologyKey: "kubernetes.io/hostname",
MaxSkew: ptr.To(int32(DefaultMaxSkewValue)),
WhenUnsatisfiable: fleetv1beta1.DoNotSchedule,
},
},
Tolerations: []fleetv1beta1.Toleration{
{
Key: "key",
Value: "value",
Operator: corev1.TolerationOpEqual,
},
},
},
Strategy: fleetv1beta1.RolloutStrategy{
Type: fleetv1beta1.RollingUpdateRolloutStrategyType,
RollingUpdate: &fleetv1beta1.RollingUpdateConfig{
MaxUnavailable: ptr.To(intstr.FromString("%15")),
MaxSurge: ptr.To(intstr.FromString("%15")),
UnavailablePeriodSeconds: ptr.To(15),
},
ApplyStrategy: &fleetv1beta1.ApplyStrategy{
Type: fleetv1beta1.ApplyStrategyTypeClientSideApply,
},
},
RevisionHistoryLimit: ptr.To(int32(10)),
},
},
},
"ClusterResourcePlacement with serverside apply config not set": {
obj: &fleetv1beta1.ClusterResourcePlacement{
Spec: fleetv1beta1.ClusterResourcePlacementSpec{
Strategy: fleetv1beta1.RolloutStrategy{
ApplyStrategy: &fleetv1beta1.ApplyStrategy{
Type: fleetv1beta1.ApplyStrategyTypeServerSideApply,
},
},
},
},
wantObj: &fleetv1beta1.ClusterResourcePlacement{
Spec: fleetv1beta1.ClusterResourcePlacementSpec{
Policy: &fleetv1beta1.PlacementPolicy{
PlacementType: fleetv1beta1.PickAllPlacementType,
},
Strategy: fleetv1beta1.RolloutStrategy{
Type: fleetv1beta1.RollingUpdateRolloutStrategyType,
RollingUpdate: &fleetv1beta1.RollingUpdateConfig{
MaxUnavailable: ptr.To(intstr.FromString(DefaultMaxUnavailableValue)),
MaxSurge: ptr.To(intstr.FromString(DefaultMaxSurgeValue)),
UnavailablePeriodSeconds: ptr.To(DefaultUnavailablePeriodSeconds),
},
ApplyStrategy: &fleetv1beta1.ApplyStrategy{
Type: fleetv1beta1.ApplyStrategyTypeServerSideApply,
ServerSideApplyConfig: &fleetv1beta1.ServerSideApplyConfig{
ForceConflicts: false,
},
},
},
RevisionHistoryLimit: ptr.To(int32(DefaultRevisionHistoryLimitValue)),
},
},
},
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
SetDefaultsClusterResourcePlacement(tt.obj)
if diff := cmp.Diff(tt.wantObj, tt.obj); diff != "" {
t.Errorf("SetDefaultsClusterResourcePlacement() mismatch (-want +got):\n%s", diff)
}
})
}
}

0 comments on commit 8ef044a

Please sign in to comment.