Skip to content

Commit

Permalink
[release-v1.19] Automated cherry pick of #336: Slightly clarify shoot…
Browse files Browse the repository at this point in the history 
….worker validation error #352: Make admission check of min worker pool and number of zones backward compatible (#354)

* Clarify autoscaling validation error

* Make admission check of min worker pool and number of zones backward compatible

* Use github.com/gardener/gardener/pkg/apis/core/helper.FindWorkerByName to get the old worker

Co-authored-by: Plamen Kokanov <plamen.kokanov@sap.com>
  • Loading branch information
@vpnachev @plkokanov
vpnachev and plkokanov authored Nov 24, 2021
1 parent ff3c06f commit 3ad2bc2
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 15 deletions.
4 changes: 2 additions & 2 deletions pkg/admission/validator/shoot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,8 @@ func (s *shoot) validateCreate(ctx context.Context, shoot *core.Shoot) error {
// TODO: This check won't be needed after generic support to scale from zero is introduced in CA
// Ongoing issue - https://github.com/gardener/autoscaler/issues/27
for i, worker := range shoot.Spec.Provider.Workers {
if worker.Minimum < int32(len(worker.Zones)) {
return fmt.Errorf("%s minimum value must be >= %d if maximum value > 0 (auto scaling to 0 & from 0 is not supported", workersPath.Index(i).Child("minimum").String(), len(worker.Zones))
if err = gcpvalidation.ValidateWorkerAutoScaling(worker, workersPath.Index(i).Child("minimum").String()); err != nil {
return err
}
}

Expand Down
31 changes: 21 additions & 10 deletions pkg/apis/gcp/validation/shoot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ import (
"fmt"

"github.com/gardener/gardener/pkg/apis/core"
"github.com/gardener/gardener/pkg/apis/core/helper"
"github.com/gardener/gardener/pkg/apis/core/validation"

"k8s.io/apimachinery/pkg/api/equality"
apivalidation "k8s.io/apimachinery/pkg/api/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
)
Expand Down Expand Up @@ -60,6 +62,15 @@ func ValidateWorkers(workers []core.Worker, fldPath *field.Path) field.ErrorList
return allErrs
}

// ValidateWorkerAutoScaling checks if the worker.minimum value is greater or equal to the number of worker.zones[]
// when the worker.maximum value is greater than zero. This check is necessary because autoscaling from 0 is not supported on gcp.
func ValidateWorkerAutoScaling(worker core.Worker, path string) error {
if worker.Maximum > 0 && worker.Minimum < int32(len(worker.Zones)) {
return fmt.Errorf("%s value must be >= %d (number of zones) if maximum value > 0 (auto scaling to 0 & from 0 is not supported)", path, len(worker.Zones))
}
return nil
}

func validateVolume(vol *core.Volume, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
if vol.Type == nil {
Expand All @@ -75,19 +86,19 @@ func validateVolume(vol *core.Volume, fldPath *field.Path) field.ErrorList {
func ValidateWorkersUpdate(oldWorkers, newWorkers []core.Worker, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
for i, newWorker := range newWorkers {

workerFldPath := fldPath.Index(i)
for _, oldWorker := range oldWorkers {
if newWorker.Name == oldWorker.Name {
if validation.ShouldEnforceImmutability(newWorker.Zones, oldWorker.Zones) {
allErrs = append(allErrs, apivalidation.ValidateImmutableField(newWorker.Zones, oldWorker.Zones, workerFldPath.Child("zones"))...)
}
break
}
oldWorker := helper.FindWorkerByName(oldWorkers, newWorker.Name)

if oldWorker != nil && validation.ShouldEnforceImmutability(newWorker.Zones, oldWorker.Zones) {
allErrs = append(allErrs, apivalidation.ValidateImmutableField(newWorker.Zones, oldWorker.Zones, workerFldPath.Child("zones"))...)
}

if newWorker.Minimum < int32(len(newWorker.Zones)) {
allErrs = append(allErrs, field.Forbidden(workerFldPath.Child("minimum"), fmt.Sprintf("minimum value must be >= %d if maximum value > 0 (auto scaling to 0 & from 0 is not supported", len(newWorker.Zones))))
// TODO: This check won't be needed after generic support to scale from zero is introduced in CA
// Ongoing issue - https://github.com/gardener/autoscaler/issues/27
if !equality.Semantic.DeepEqual(&newWorker, oldWorker) {
if err := ValidateWorkerAutoScaling(newWorker, workerFldPath.Child("minimum").String()); err != nil {
allErrs = append(allErrs, field.Forbidden(workerFldPath.Child("minimum"), err.Error()))
}
}
}
return allErrs
Expand Down
124 changes: 121 additions & 3 deletions pkg/apis/gcp/validation/shoot_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,38 @@ var _ = Describe("Shoot validation", func() {
})
})

Describe("#ValidateWorkerAutoscaling", func() {
var worker core.Worker

BeforeEach(func() {
worker = core.Worker{
Minimum: 2,
Maximum: 4,
Zones: []string{"zone1", "zone2"},
}
})

It("should not return an error if worker.minimum is equal to number of zones", func() {
Expect(ValidateWorkerAutoScaling(worker, "")).To(Succeed())
})

It("should not return an error if worker.minimum is greater than number of zones", func() {
worker.Minimum = 3
Expect(ValidateWorkerAutoScaling(worker, "")).To(Succeed())
})

It("should not return an error if worker.maximum is 0 and worker.minimum is less than number of zones", func() {
worker.Minimum = 0
worker.Maximum = 0
Expect(ValidateWorkerAutoScaling(worker, "")).To(Succeed())
})

It("should return an error if worker.minimum is less than number of zones", func() {
worker.Minimum = 1
Expect(ValidateWorkerAutoScaling(worker, "")).To(HaveOccurred())
})
})

Describe("#ValidateWorkersUpdate", func() {
var workerPath = field.NewPath("spec", "workers")

Expand Down Expand Up @@ -99,7 +131,7 @@ var _ = Describe("Shoot validation", func() {

oldWorkers := []core.Worker{
{
Name: "worker-test",
Name: "worker-test2",
Machine: core.Machine{
Type: "n1-standard-2",
Image: &core.ShootMachineImage{
Expand All @@ -124,7 +156,7 @@ var _ = Describe("Shoot validation", func() {
Expect(errorList).To(BeEmpty())
})

It("should return an error because new worker pool added does not honor having min workers >= number of zones", func() {
It("should return an error because updated worker pool does not honor having min workers >= number of zones", func() {
newWorkers := []core.Worker{
{
Name: "worker-test",
Expand Down Expand Up @@ -173,7 +205,93 @@ var _ = Describe("Shoot validation", func() {

errorList := ValidateWorkersUpdate(oldWorkers, newWorkers, workerPath)

Expect(errorList[0].Error()).To(Equal("spec.workers[0].minimum: Forbidden: minimum value must be >= " + fmt.Sprint(len(newWorkers[0].Zones)) + " if maximum value > 0 (auto scaling to 0 & from 0 is not supported"))
Expect(errorList).ToNot(HaveLen(0))
Expect(errorList[0].Error()).To(Equal("spec.workers[0].minimum: Forbidden: spec.workers[0].minimum value must be >= " + fmt.Sprint(len(newWorkers[0].Zones)) + " (number of zones) if maximum value > 0 (auto scaling to 0 & from 0 is not supported)"))
})

It("should return an error because newly added worker pool does not honor having min workers >= number of zones", func() {
newWorkers := []core.Worker{
{
Name: "worker-test",
Machine: core.Machine{
Type: "n1-standard-2",
Image: &core.ShootMachineImage{
Name: "gardenlinux",
Version: "318.8.0",
},
},
Maximum: 6,
Minimum: 1,
Zones: []string{
"europe-west1-b",
"europe-west1-c",
},
Volume: &core.Volume{
Type: pointer.StringPtr("pd-standard"),
VolumeSize: "50Gi",
},
},
}

oldWorkers := []core.Worker{}

errorList := ValidateWorkersUpdate(oldWorkers, newWorkers, workerPath)

Expect(errorList).ToNot(HaveLen(0))
Expect(errorList[0].Error()).To(Equal("spec.workers[0].minimum: Forbidden: spec.workers[0].minimum value must be >= " + fmt.Sprint(len(newWorkers[0].Zones)) + " (number of zones) if maximum value > 0 (auto scaling to 0 & from 0 is not supported)"))
})

It("should not return any error because existing worker pool does not honor having min workers >= number of zones", func() {
newWorkers := []core.Worker{
{
Name: "worker-test",
Machine: core.Machine{
Type: "n1-standard-2",
Image: &core.ShootMachineImage{
Name: "gardenlinux",
Version: "318.8.0",
},
},
Maximum: 6,
Minimum: 2,
Zones: []string{
"europe-west1-b",
"europe-west1-c",
"europe-west1-d",
},
Volume: &core.Volume{
Type: pointer.StringPtr("pd-standard"),
VolumeSize: "50Gi",
},
},
}

oldWorkers := []core.Worker{
{
Name: "worker-test",
Machine: core.Machine{
Type: "n1-standard-2",
Image: &core.ShootMachineImage{
Name: "gardenlinux",
Version: "318.8.0",
},
},
Maximum: 6,
Minimum: 2,
Zones: []string{
"europe-west1-b",
"europe-west1-c",
"europe-west1-d",
},
Volume: &core.Volume{
Type: pointer.StringPtr("pd-standard"),
VolumeSize: "50Gi",
},
},
}

errorList := ValidateWorkersUpdate(oldWorkers, newWorkers, workerPath)
Expect(errorList).To(BeEmpty())
})
})
})
Expand Down

0 comments on commit 3ad2bc2

Please sign in to comment.