diff --git a/README.md b/README.md index 2ab0c24..c7b223d 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,7 @@ Note: this is not an officially supported Google product. * [Features](#features) * [[Boost target] POD label selector](#boost-target-pod-label-selector) * [[Boost resources] percentage increase](#boost-resources-percentage-increase) + * [[Boost resources] fixed target](#boost-resources-fixed-target) * [[Boost duration] fixed time](#boost-duration-fixed-time) * [[Boost duration] POD condition](#boost-duration-pod-condition) * [License](#license) @@ -144,6 +145,21 @@ spec: value: 50 ``` +### [Boost resources] fixed target + +Define the fixed resources for a target container(s). The CPU requests and limits of selected +container(s) will be set to the given values. Note that specified requests and limits have to be +higher than the ones in the container. + +```yaml +spec: + containerPolicies: + - containerName: spring-rest-jpa + fixedResources: + requests: "1" + limits: "2" +``` + ### [Boost duration] fixed time Define the fixed amount of time, the resource boost effect will last for it since the POD creation. diff --git a/api/v1alpha1/startupcpuboost_types.go b/api/v1alpha1/startupcpuboost_types.go index a7d1281..6626b07 100644 --- a/api/v1alpha1/startupcpuboost_types.go +++ b/api/v1alpha1/startupcpuboost_types.go @@ -16,6 +16,7 @@ package v1alpha1 import ( corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -60,8 +61,19 @@ type DurationPolicy struct { PodCondition *PodConditionDurationPolicy `json:"podCondition,omitempty"` } -// PercentageIncrease defines the policy used to determine the target -// resources for a container +// FixedResources defines the CPU resource policy that sets CPU resources +// to the given values +type FixedResources struct { + // Requests specifies the CPU requests + // +kubebuilder:validation:Required + Requests resource.Quantity `json:"requests,omitempty"` + // Limits specifies the CPU requests + // +kubebuilder:validation:Optional + Limits resource.Quantity `json:"limits,omitempty"` +} + +// PercentageIncrease defines the CPU resource policy that increases +// CPU resources by the given percentage value type PercentageIncrease struct { // Value specifies the percentage value // +kubebuilder:validation:Required @@ -75,9 +87,14 @@ type ContainerPolicy struct { // ContainerName specifies the name of container for a given policy // +kubebuilder:validation:Required ContainerName string `json:"containerName,omitempty"` - // PercentageIncrease specifies the percentage increase policy for a container - // +kubebuilder:validation:Required - PercentageIncrease PercentageIncrease `json:"percentageIncrease,omitempty"` + // PercentageIncrease specifies the CPU resource policy that increases + // CPU resources by the given percentage value + // +kubebuilder:validation:Optional + PercentageIncrease *PercentageIncrease `json:"percentageIncrease,omitempty"` + // FixedResources specifies the CPU resource policy that sets the CPU + // resources to the given values + // +kubebuilder:validation:Optional + FixedResources *FixedResources `json:"fixedResources,omitempty"` } // ResourcePolicy defines the policy used to determine the target diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 200bef1..d9c8a18 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -25,7 +25,16 @@ import ( // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ContainerPolicy) DeepCopyInto(out *ContainerPolicy) { *out = *in - out.PercentageIncrease = in.PercentageIncrease + if in.PercentageIncrease != nil { + in, out := &in.PercentageIncrease, &out.PercentageIncrease + *out = new(PercentageIncrease) + **out = **in + } + if in.FixedResources != nil { + in, out := &in.FixedResources, &out.FixedResources + *out = new(FixedResources) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ContainerPolicy. @@ -78,6 +87,23 @@ func (in *FixedDurationPolicy) DeepCopy() *FixedDurationPolicy { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FixedResources) DeepCopyInto(out *FixedResources) { + *out = *in + out.Requests = in.Requests.DeepCopy() + out.Limits = in.Limits.DeepCopy() +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FixedResources. +func (in *FixedResources) DeepCopy() *FixedResources { + if in == nil { + return nil + } + out := new(FixedResources) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PercentageIncrease) DeepCopyInto(out *PercentageIncrease) { *out = *in @@ -114,7 +140,9 @@ func (in *ResourcePolicy) DeepCopyInto(out *ResourcePolicy) { if in.ContainerPolicies != nil { in, out := &in.ContainerPolicies, &out.ContainerPolicies *out = make([]ContainerPolicy, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } } diff --git a/config/crd/bases/autoscaling.x-k8s.io_startupcpuboosts.yaml b/config/crd/bases/autoscaling.x-k8s.io_startupcpuboosts.yaml index 430e2e6..885acaa 100644 --- a/config/crd/bases/autoscaling.x-k8s.io_startupcpuboosts.yaml +++ b/config/crd/bases/autoscaling.x-k8s.io_startupcpuboosts.yaml @@ -123,9 +123,29 @@ spec: description: ContainerName specifies the name of container for a given policy type: string + fixedResources: + description: FixedResources specifies the CPU resource policy + that sets the CPU resources to the given values + properties: + limits: + anyOf: + - type: integer + - type: string + description: Limits specifies the CPU requests + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + requests: + anyOf: + - type: integer + - type: string + description: Requests specifies the CPU requests + pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ + x-kubernetes-int-or-string: true + type: object percentageIncrease: - description: PercentageIncrease specifies the percentage - increase policy for a container + description: PercentageIncrease specifies the CPU resource + policy that increases CPU resources by the given percentage + value properties: value: description: Value specifies the percentage value diff --git a/internal/boost/boost_suite_test.go b/internal/boost/boost_suite_test.go index f7b2683..d20ce2d 100644 --- a/internal/boost/boost_suite_test.go +++ b/internal/boost/boost_suite_test.go @@ -52,25 +52,8 @@ var _ = BeforeSuite(func() { Name: "boost-001", Namespace: "demo", }, - Spec: autoscaling.StartupCPUBoostSpec{ - ResourcePolicy: autoscaling.ResourcePolicy{ - ContainerPolicies: []autoscaling.ContainerPolicy{ - { - ContainerName: containerOneName, - PercentageIncrease: autoscaling.PercentageIncrease{ - Value: containerOnePercValue, - }, - }, - { - ContainerName: containerTwoName, - PercentageIncrease: autoscaling.PercentageIncrease{ - Value: containerTwoPercValue, - }, - }, - }, - }, - }, } + annotTemplate = &bpod.BoostPodAnnotation{ BoostTimestamp: time.Now(), InitCPURequests: map[string]string{ diff --git a/internal/boost/resource/fixed.go b/internal/boost/resource/fixed.go new file mode 100644 index 0000000..c780b92 --- /dev/null +++ b/internal/boost/resource/fixed.go @@ -0,0 +1,69 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package resource + +import ( + "context" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + apiResource "k8s.io/apimachinery/pkg/api/resource" + ctrl "sigs.k8s.io/controller-runtime" +) + +type FixedPolicy struct { + cpuRequests apiResource.Quantity + cpuLimits apiResource.Quantity +} + +func NewFixedPolicy(requests apiResource.Quantity, limits apiResource.Quantity) ContainerPolicy { + return &FixedPolicy{ + cpuRequests: requests, + cpuLimits: limits, + } +} + +func (p *FixedPolicy) Requests() apiResource.Quantity { + return p.cpuRequests +} + +func (p *FixedPolicy) Limits() apiResource.Quantity { + return p.cpuLimits +} + +func (p *FixedPolicy) NewResources(ctx context.Context, container *corev1.Container) *corev1.ResourceRequirements { + log := ctrl.LoggerFrom(ctx).WithName("fixed-cpu-policy"). + WithValues("newCPURequsts", p.cpuRequests.String()). + WithValues("newCPULimits", p.cpuLimits.String()) + result := container.Resources.DeepCopy() + p.setResource(corev1.ResourceCPU, result.Requests, p.cpuRequests, log) + p.setResource(corev1.ResourceCPU, result.Limits, p.cpuLimits, log) + return result +} + +func (p *FixedPolicy) setResource(resource corev1.ResourceName, resources corev1.ResourceList, target apiResource.Quantity, log logr.Logger) { + if target.IsZero() { + return + } + current, ok := resources[resource] + if !ok { + return + } + if target.Cmp(current) < 0 { + log.V(2).Info("container has higher CPU requests than policy") + return + } + resources[resource] = target +} diff --git a/internal/boost/resource/fixed_test.go b/internal/boost/resource/fixed_test.go new file mode 100644 index 0000000..493e528 --- /dev/null +++ b/internal/boost/resource/fixed_test.go @@ -0,0 +1,148 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package resource_test + +import ( + "context" + + "github.com/google/kube-startup-cpu-boost/internal/boost/resource" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + corev1 "k8s.io/api/core/v1" + apiResource "k8s.io/apimachinery/pkg/api/resource" +) + +var _ = Describe("Fixed Resource Policy", func() { + var ( + policy resource.ContainerPolicy + newResources *corev1.ResourceRequirements + container *corev1.Container + cpuRequests, cpuLimits apiResource.Quantity + ) + BeforeEach(func() { + container = containerTemplate.DeepCopy() + }) + JustBeforeEach(func() { + policy = resource.NewFixedPolicy(cpuRequests, cpuLimits) + newResources = policy.NewResources(context.TODO(), container) + }) + Describe("has both requests and limits defined", func() { + BeforeEach(func() { + cpuRequests = apiResource.MustParse("1") + cpuLimits = apiResource.MustParse("2") + }) + When("container has requests and limits defined", func() { + BeforeEach(func() { + container.Resources.Requests[corev1.ResourceCPU] = apiResource.MustParse("500m") + container.Resources.Limits[corev1.ResourceCPU] = apiResource.MustParse("1") + }) + It("returns resources with a valid CPU requests", func() { + Expect(newResources.Requests).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Requests[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(cpuRequests.String())) + }) + It("returns resources with a valid CPU limits", func() { + Expect(newResources.Limits).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Limits[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(cpuLimits.String())) + }) + }) + When("container has no requests and limits defined", func() { + BeforeEach(func() { + container.Resources.Requests = nil + container.Resources.Limits = nil + }) + It("returns empty new resources", func() { + Expect(newResources.Requests).To(HaveLen(0)) + Expect(newResources.Limits).To(HaveLen(0)) + }) + }) + When("container has requests defined", func() { + BeforeEach(func() { + container.Resources.Requests[corev1.ResourceCPU] = apiResource.MustParse("500m") + container.Resources.Limits = nil + }) + It("returns resources with a valid CPU requests", func() { + Expect(newResources.Requests).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Requests[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(cpuRequests.String())) + }) + It("returns resources without CPU limits", func() { + Expect(newResources.Limits).NotTo(HaveKey(corev1.ResourceCPU)) + }) + }) + When("container has lower requests and limits defined", func() { + var ( + containerCPUReq, containerCPULim apiResource.Quantity + ) + BeforeEach(func() { + containerCPUReq = cpuRequests.DeepCopy() + containerCPUReq.Add(apiResource.MustParse("1")) + containerCPULim = cpuLimits.DeepCopy() + containerCPULim.Add(apiResource.MustParse("1")) + container.Resources.Requests[corev1.ResourceCPU] = containerCPUReq + container.Resources.Limits[corev1.ResourceCPU] = containerCPULim + }) + It("returns resources with a valid CPU requests", func() { + Expect(newResources.Requests).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Requests[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(containerCPUReq.String())) + }) + It("returns resources with a valid CPU limits", func() { + Expect(newResources.Limits).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Limits[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(containerCPULim.String())) + }) + }) + }) + Describe("has only requests defined", func() { + BeforeEach(func() { + cpuRequests = apiResource.MustParse("1") + cpuLimits = apiResource.Quantity{} + }) + When("container has resources and limits defined", func() { + BeforeEach(func() { + container.Resources.Requests[corev1.ResourceCPU] = apiResource.MustParse("500m") + container.Resources.Limits[corev1.ResourceCPU] = apiResource.MustParse("1") + }) + It("returns resources with a valid CPU requests", func() { + Expect(newResources.Requests).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Requests[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(cpuRequests.String())) + }) + It("returns resources with a valid CPU limits", func() { + Expect(newResources.Limits).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Limits[corev1.ResourceCPU] + containerLimits := container.Resources.Limits[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(containerLimits.String())) + }) + }) + When("container has resources defined", func() { + BeforeEach(func() { + container.Resources.Requests[corev1.ResourceCPU] = apiResource.MustParse("500m") + container.Resources.Limits = nil + }) + It("returns resources with a valid CPU requests", func() { + Expect(newResources.Requests).To(HaveKey(corev1.ResourceCPU)) + qty := newResources.Requests[corev1.ResourceCPU] + Expect(qty.String()).To(Equal(cpuRequests.String())) + }) + It("returns resources without CPU limits", func() { + Expect(newResources.Limits).NotTo(HaveKey(corev1.ResourceCPU)) + }) + }) + }) +}) diff --git a/internal/boost/resource/percentage.go b/internal/boost/resource/percentage.go index a81f1d0..4597d02 100644 --- a/internal/boost/resource/percentage.go +++ b/internal/boost/resource/percentage.go @@ -12,10 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package resource contains implementation of resource boost duration policies package resource import ( + "context" + "gopkg.in/inf.v0" corev1 "k8s.io/api/core/v1" apiResource "k8s.io/apimachinery/pkg/api/resource" @@ -35,7 +36,7 @@ func (p *PercentageContainerPolicy) Percentage() int64 { return p.percentage } -func (p *PercentageContainerPolicy) NewResources(container *corev1.Container) *corev1.ResourceRequirements { +func (p *PercentageContainerPolicy) NewResources(ctx context.Context, container *corev1.Container) *corev1.ResourceRequirements { result := container.Resources.DeepCopy() p.increaseResource(corev1.ResourceCPU, result.Requests) p.increaseResource(corev1.ResourceCPU, result.Limits) diff --git a/internal/boost/resource/percentage_test.go b/internal/boost/resource/percentage_test.go index 4deadac..5fc6c97 100644 --- a/internal/boost/resource/percentage_test.go +++ b/internal/boost/resource/percentage_test.go @@ -15,6 +15,8 @@ package resource_test import ( + "context" + "github.com/google/kube-startup-cpu-boost/internal/boost/resource" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -36,15 +38,13 @@ var _ = Describe("PercentageResourcePolicy", func() { }) JustBeforeEach(func() { policy = resource.NewPercentageContainerPolicy(percentage) - newResources = policy.NewResources(container) + newResources = policy.NewResources(context.TODO(), container) }) When("There are resources and limits defined", func() { BeforeEach(func() { container = containerTemplate.DeepCopy() - cpuReq, err := apiResource.ParseQuantity("1") - Expect(err).NotTo(HaveOccurred()) - cpuLim, err := apiResource.ParseQuantity("2") - Expect(err).NotTo(HaveOccurred()) + cpuReq := apiResource.MustParse("1") + cpuLim := apiResource.MustParse("2") container.Resources.Requests[corev1.ResourceCPU] = cpuReq container.Resources.Limits[corev1.ResourceCPU] = cpuLim }) @@ -59,7 +59,7 @@ var _ = Describe("PercentageResourcePolicy", func() { Expect(qty.String()).To(Equal("2400m")) }) }) - When("There are no resources and limits defined", func() { + When("There are no requests and limits defined", func() { BeforeEach(func() { container = containerTemplate.DeepCopy() container.Resources.Requests = nil diff --git a/internal/boost/resource/resource.go b/internal/boost/resource/resource.go index 86ba42f..e371b0f 100644 --- a/internal/boost/resource/resource.go +++ b/internal/boost/resource/resource.go @@ -15,8 +15,12 @@ // Package resource contains implementation of resource boost duration policies package resource -import corev1 "k8s.io/api/core/v1" +import ( + "context" + + corev1 "k8s.io/api/core/v1" +) type ContainerPolicy interface { - NewResources(container *corev1.Container) *corev1.ResourceRequirements + NewResources(ctx context.Context, container *corev1.Container) *corev1.ResourceRequirements } diff --git a/internal/boost/startupcpuboost.go b/internal/boost/startupcpuboost.go index 0726d3d..b35a508 100644 --- a/internal/boost/startupcpuboost.go +++ b/internal/boost/startupcpuboost.go @@ -16,6 +16,7 @@ package boost import ( "context" + "errors" "fmt" "sync" "time" @@ -65,12 +66,16 @@ func NewStartupCPUBoost(client client.Client, boost *autoscaling.StartupCPUBoost if err != nil { return nil, err } + resourcePolicies, err := mapResourcePolicy(boost.Spec.ResourcePolicy) + if err != nil { + return nil, err + } return &StartupCPUBoostImpl{ name: boost.Name, namespace: boost.Namespace, selector: selector, durationPolicies: mapDurationPolicy(boost.Spec.DurationPolicy), - resourcePolicies: mapResourcePolicy(boost.Spec.ResourcePolicy), + resourcePolicies: resourcePolicies, client: client, }, nil } @@ -120,7 +125,7 @@ func (b *StartupCPUBoostImpl) UpsertPod(ctx context.Context, pod *corev1.Pod) er return nil } if valid := b.validatePolicyOnPod(ctx, condPolicy, pod); !valid { - log.V(5).Info("updating pod with initial resources") + log.V(2).Info("updating pod with initial resources") if err := b.RevertResources(ctx, pod); err != nil { return fmt.Errorf("failed to update pod: %s", err) } @@ -211,12 +216,30 @@ func mapDurationPolicy(policiesSpec autoscaling.DurationPolicy) map[string]durat // mapResourcePolicy maps the Resource Policy from the API spec to the map of policy // implementations with container name keys -func mapResourcePolicy(spec autoscaling.ResourcePolicy) map[string]resource.ContainerPolicy { +func mapResourcePolicy(spec autoscaling.ResourcePolicy) (map[string]resource.ContainerPolicy, error) { + var errs []error policies := make(map[string]resource.ContainerPolicy) for _, policySpec := range spec.ContainerPolicies { - policies[policySpec.ContainerName] = resource.NewPercentageContainerPolicy(policySpec.PercentageIncrease.Value) + var policy resource.ContainerPolicy + var cnt int + if fixedResources := policySpec.FixedResources; fixedResources != nil { + policy = resource.NewFixedPolicy(fixedResources.Requests, fixedResources.Limits) + cnt++ + } + if percIncrease := policySpec.PercentageIncrease; percIncrease != nil { + policy = resource.NewPercentageContainerPolicy(percIncrease.Value) + cnt++ + } + if cnt != 1 { + errs = append(errs, fmt.Errorf("invalid number of resource policies fo container %s; must be one", policySpec.ContainerName)) + continue + } + policies[policySpec.ContainerName] = policy } - return policies + if len(errs) > 0 { + return nil, errors.Join(errs...) + } + return policies, nil } // fixedPolicyToDuration maps the attributes from FixedDurationPolicy API spec to the diff --git a/internal/boost/startupcpuboost_test.go b/internal/boost/startupcpuboost_test.go index f98b3e0..717d858 100644 --- a/internal/boost/startupcpuboost_test.go +++ b/internal/boost/startupcpuboost_test.go @@ -27,6 +27,7 @@ import ( . "github.com/onsi/gomega" "go.uber.org/mock/gomock" corev1 "k8s.io/api/core/v1" + apiResource "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -44,7 +45,9 @@ var _ = Describe("StartupCPUBoost", func() { Describe("Instantiates from the API specification", func() { JustBeforeEach(func() { boost, err = cpuboost.NewStartupCPUBoost(nil, spec) - Expect(err).ShouldNot(HaveOccurred()) + }) + It("does not error", func() { + Expect(err).NotTo(HaveOccurred()) }) It("returns valid name", func() { Expect(boost.Name()).To(Equal(spec.Name)) @@ -52,19 +55,79 @@ var _ = Describe("StartupCPUBoost", func() { It("returns valid namespace", func() { Expect(boost.Namespace()).To(Equal(spec.Namespace)) }) - It("returns valid resource policy for container one", func() { - p, ok := boost.ResourcePolicy(containerOneName) - Expect(ok).To(BeTrue()) - Expect(p).To(BeAssignableToTypeOf(&resource.PercentageContainerPolicy{})) - percPolicy, _ := p.(*resource.PercentageContainerPolicy) - Expect(percPolicy.Percentage()).To(Equal(containerOnePercValue)) + When("the spec has resource policy for containers", func() { + var ( + containerOneName = "container-one" + containerTwoName = "container-two" + containerOnePercValue int64 = 120 + containerTwoFixedReq = apiResource.MustParse("1") + containerTwoFixedLim = apiResource.MustParse("2") + ) + BeforeEach(func() { + spec.Spec.ResourcePolicy = autoscaling.ResourcePolicy{ + ContainerPolicies: []autoscaling.ContainerPolicy{ + { + ContainerName: containerOneName, + PercentageIncrease: &autoscaling.PercentageIncrease{ + Value: containerOnePercValue, + }, + }, + { + ContainerName: containerTwoName, + FixedResources: &autoscaling.FixedResources{ + Requests: containerTwoFixedReq, + Limits: containerTwoFixedLim, + }, + }, + }, + } + }) + It("does not error", func() { + Expect(err).NotTo(HaveOccurred()) + }) + It("returns valid resource policy for container one", func() { + p, ok := boost.ResourcePolicy(containerOneName) + Expect(ok).To(BeTrue()) + Expect(p).To(BeAssignableToTypeOf(&resource.PercentageContainerPolicy{})) + percPolicy, _ := p.(*resource.PercentageContainerPolicy) + Expect(percPolicy.Percentage()).To(Equal(containerOnePercValue)) + }) + It("returns valid resource policy for container two", func() { + p, ok := boost.ResourcePolicy(containerTwoName) + Expect(ok).To(BeTrue()) + Expect(p).To(BeAssignableToTypeOf(&resource.FixedPolicy{})) + fixedPolicy, _ := p.(*resource.FixedPolicy) + Expect(fixedPolicy.Requests()).To(Equal(containerTwoFixedReq)) + Expect(fixedPolicy.Limits()).To(Equal(containerTwoFixedLim)) + }) }) - It("returns valid resource policy for container two", func() { - p, ok := boost.ResourcePolicy(containerTwoName) - Expect(ok).To(BeTrue()) - Expect(p).To(BeAssignableToTypeOf(&resource.PercentageContainerPolicy{})) - percPolicy, _ := p.(*resource.PercentageContainerPolicy) - Expect(percPolicy.Percentage()).To(Equal(containerTwoPercValue)) + When("the spec has container policy without resource policy", func() { + BeforeEach(func() { + spec.Spec.ResourcePolicy = autoscaling.ResourcePolicy{ + ContainerPolicies: []autoscaling.ContainerPolicy{ + { + ContainerName: "container-one", + }, + }, + } + }) + It("errors", func() { + Expect(err).To(HaveOccurred()) + }) + }) + When("the spec has container policy with two resource policies", func() { + BeforeEach(func() { + spec.Spec.ResourcePolicy = autoscaling.ResourcePolicy{ + ContainerPolicies: []autoscaling.ContainerPolicy{ + { + ContainerName: "container-one", + }, + }, + } + }) + It("errors", func() { + Expect(err).To(HaveOccurred()) + }) }) When("the spec has fixed duration policy", func() { BeforeEach(func() { diff --git a/internal/controller/controller_suite_test.go b/internal/controller/controller_suite_test.go index efe2850..39f9f95 100644 --- a/internal/controller/controller_suite_test.go +++ b/internal/controller/controller_suite_test.go @@ -49,7 +49,7 @@ var _ = BeforeSuite(func() { ContainerPolicies: []autoscaling.ContainerPolicy{ { ContainerName: "demo", - PercentageIncrease: autoscaling.PercentageIncrease{ + PercentageIncrease: &autoscaling.PercentageIncrease{ Value: 120, }, }, diff --git a/internal/webhook/podcpuboost_webhook.go b/internal/webhook/podcpuboost_webhook.go index 8e20ab9..d822848 100644 --- a/internal/webhook/podcpuboost_webhook.go +++ b/internal/webhook/podcpuboost_webhook.go @@ -60,7 +60,7 @@ func (h *podCPUBoostHandler) Handle(ctx context.Context, req admission.Request) return admission.Allowed("no StartupCPUBoost matched") } log = log.WithValues("boost", boostImpl.Name()) - h.boostContainerResources(boostImpl, pod, log) + h.boostContainerResources(ctx, boostImpl, pod, log) marshaledPod, err := json.Marshal(pod) if err != nil { return admission.Errored(http.StatusInternalServerError, err) @@ -68,7 +68,7 @@ func (h *podCPUBoostHandler) Handle(ctx context.Context, req admission.Request) return admission.PatchResponseFromRaw(req.Object.Raw, marshaledPod) } -func (h *podCPUBoostHandler) boostContainerResources(b boost.StartupCPUBoost, pod *corev1.Pod, log logr.Logger) { +func (h *podCPUBoostHandler) boostContainerResources(ctx context.Context, b boost.StartupCPUBoost, pod *corev1.Pod, log logr.Logger) { annotation := bpod.NewBoostAnnotation() for i, container := range pod.Spec.Containers { policy, found := b.ResourcePolicy(container.Name) @@ -84,12 +84,12 @@ func (h *podCPUBoostHandler) boostContainerResources(b boost.StartupCPUBoost, po continue } updateBoostAnnotation(annotation, container.Name, container.Resources) - resources := policy.NewResources(&container) + resources := policy.NewResources(ctx, &container) log = log.WithValues( "newCPURequests", resources.Requests.Cpu().String(), "newCPULimits", resources.Limits.Cpu().String(), ) - log.Info("increasing resources") + log.V(2).Info("increasing resources") pod.Spec.Containers[i].Resources = *resources } if len(annotation.InitCPULimits) > 0 || len(annotation.InitCPURequests) > 0 { diff --git a/internal/webhook/podcpuboost_webhook_test.go b/internal/webhook/podcpuboost_webhook_test.go index ca1b6e5..93828b0 100644 --- a/internal/webhook/podcpuboost_webhook_test.go +++ b/internal/webhook/podcpuboost_webhook_test.go @@ -278,9 +278,13 @@ func containerResourcePatch(pod *corev1.Pod, policy resource.ContainerPolicy, re var newQuantity apiResource.Quantity switch requirement { case "requests": - newQuantity = policy.NewResources(&pod.Spec.Containers[containerIdx]).Requests[corev1.ResourceCPU] + newQuantity = policy.NewResources( + context.TODO(), + &pod.Spec.Containers[containerIdx]).Requests[corev1.ResourceCPU] case "limits": - newQuantity = policy.NewResources(&pod.Spec.Containers[containerIdx]).Limits[corev1.ResourceCPU] + newQuantity = policy.NewResources( + context.TODO(), + &pod.Spec.Containers[containerIdx]).Limits[corev1.ResourceCPU] default: panic("unsupported resource requirement") } diff --git a/internal/webhook/startupcpuboost_webhook.go b/internal/webhook/startupcpuboost_webhook.go index d421073..aaa4966 100644 --- a/internal/webhook/startupcpuboost_webhook.go +++ b/internal/webhook/startupcpuboost_webhook.go @@ -67,6 +67,9 @@ func (w *StartupCPUBoostWebhook) ValidateDelete(ctx context.Context, obj runtime // validation on a top of declarative API validation func validate(boost *v1alpha1.StartupCPUBoost) error { var allErrs field.ErrorList + if errs := validateContainerPolicies(boost.Spec.ResourcePolicy.ContainerPolicies); len(errs) > 0 { + allErrs = append(allErrs, errs...) + } if err := validateDurationPolicy(boost.Spec.DurationPolicy); err != nil { allErrs = append(allErrs, err) } @@ -93,3 +96,27 @@ func validateDurationPolicy(policy v1alpha1.DurationPolicy) *field.Error { } return nil } + +func validateContainerPolicies(policies []v1alpha1.ContainerPolicy) field.ErrorList { + var allErrs field.ErrorList + baseFldPath := field.NewPath("spec"). + Child("resourcePolicy"). + Child("containerPolicies") + for i := range policies { + fldPath := baseFldPath.Index(i) + var cnt int + if policies[i].FixedResources != nil { + cnt++ + } + if policies[i].PercentageIncrease != nil { + cnt++ + } + if cnt != 1 { + allErrs = append(allErrs, field.Invalid(fldPath, + policies[i], + "one type of resource policy should be defined", + )) + } + } + return allErrs +} diff --git a/internal/webhook/startupcpuboost_webhook_test.go b/internal/webhook/startupcpuboost_webhook_test.go index 462ab9e..4008922 100644 --- a/internal/webhook/startupcpuboost_webhook_test.go +++ b/internal/webhook/startupcpuboost_webhook_test.go @@ -88,6 +88,90 @@ var _ = Describe("StartupCPUBoost webhook", func() { _, err = w.ValidateCreate(context.TODO(), &boost) Expect(err).NotTo(HaveOccurred()) + By("validating update event") + _, err = w.ValidateUpdate(context.TODO(), nil, &boost) + Expect(err).NotTo(HaveOccurred()) + }) + }) + When("Startup CPU Boost has container without resource policies", func() { + BeforeEach(func() { + boost = v1alpha1.StartupCPUBoost{ + Spec: v1alpha1.StartupCPUBoostSpec{ + ResourcePolicy: v1alpha1.ResourcePolicy{ + ContainerPolicies: []v1alpha1.ContainerPolicy{ + { + ContainerName: "container-one", + }, + }, + }, + DurationPolicy: v1alpha1.DurationPolicy{ + PodCondition: &v1alpha1.PodConditionDurationPolicy{}, + }, + }, + } + }) + It("errors", func() { + By("validating create event") + _, err = w.ValidateCreate(context.TODO(), &boost) + Expect(err).To(HaveOccurred()) + + By("validating update event") + _, err = w.ValidateUpdate(context.TODO(), nil, &boost) + Expect(err).To(HaveOccurred()) + }) + }) + When("Startup CPU Boost has container with two resource policies", func() { + BeforeEach(func() { + boost = v1alpha1.StartupCPUBoost{ + Spec: v1alpha1.StartupCPUBoostSpec{ + ResourcePolicy: v1alpha1.ResourcePolicy{ + ContainerPolicies: []v1alpha1.ContainerPolicy{ + { + ContainerName: "container-one", + FixedResources: &v1alpha1.FixedResources{}, + PercentageIncrease: &v1alpha1.PercentageIncrease{}, + }, + }, + }, + DurationPolicy: v1alpha1.DurationPolicy{ + PodCondition: &v1alpha1.PodConditionDurationPolicy{}, + }, + }, + } + }) + It("errors", func() { + By("validating create event") + _, err = w.ValidateCreate(context.TODO(), &boost) + Expect(err).To(HaveOccurred()) + + By("validating update event") + _, err = w.ValidateUpdate(context.TODO(), nil, &boost) + Expect(err).To(HaveOccurred()) + }) + }) + When("Startup CPU Boost has container with one resource policies", func() { + BeforeEach(func() { + boost = v1alpha1.StartupCPUBoost{ + Spec: v1alpha1.StartupCPUBoostSpec{ + ResourcePolicy: v1alpha1.ResourcePolicy{ + ContainerPolicies: []v1alpha1.ContainerPolicy{ + { + ContainerName: "container-one", + FixedResources: &v1alpha1.FixedResources{}, + }, + }, + }, + DurationPolicy: v1alpha1.DurationPolicy{ + PodCondition: &v1alpha1.PodConditionDurationPolicy{}, + }, + }, + } + }) + It("does not error", func() { + By("validating create event") + _, err = w.ValidateCreate(context.TODO(), &boost) + Expect(err).NotTo(HaveOccurred()) + By("validating update event") _, err = w.ValidateUpdate(context.TODO(), nil, &boost) Expect(err).NotTo(HaveOccurred())