From 0633fea03a2b99de8d1a6ddf080cf7a12c167546 Mon Sep 17 00:00:00 2001 From: Yaroslava Serdiuk Date: Tue, 2 Jan 2024 11:04:23 +0000 Subject: [PATCH] Review --- .../podlistprocessor/pod_list_processor.go | 18 +++++------- cluster-autoscaler/main.go | 4 +-- .../processors/pods/pod_list_processor.go | 29 +++++++++++-------- .../provisioning_request_processors.go | 2 +- .../provisioning_request_processors_test.go | 2 +- .../provreqclient/client.go | 9 +----- 6 files changed, 30 insertions(+), 34 deletions(-) diff --git a/cluster-autoscaler/core/podlistprocessor/pod_list_processor.go b/cluster-autoscaler/core/podlistprocessor/pod_list_processor.go index f14860983475..26d199e0cc24 100644 --- a/cluster-autoscaler/core/podlistprocessor/pod_list_processor.go +++ b/cluster-autoscaler/core/podlistprocessor/pod_list_processor.go @@ -23,14 +23,12 @@ import ( // NewDefaultPodListProcessor returns a default implementation of the pod list // processor, which wraps and sequentially runs other sub-processors. -func NewDefaultPodListProcessor(predicateChecker predicatechecker.PredicateChecker) *pods.ListedPodListProcessor { - return &pods.ListedPodListProcessor{ - Processors: []pods.PodListProcessor{ - NewClearTPURequestsPodListProcessor(), - NewFilterOutExpendablePodListProcessor(), - NewCurrentlyDrainedNodesPodListProcessor(), - NewFilterOutSchedulablePodListProcessor(predicateChecker), - NewFilterOutDaemonSetPodListProcessor(), - }, - } +func NewDefaultPodListProcessor(predicateChecker predicatechecker.PredicateChecker) *pods.CombinedPodListProcessor { + return pods.NewCombinedPodListProcessor([]pods.PodListProcessor{ + NewClearTPURequestsPodListProcessor(), + NewFilterOutExpendablePodListProcessor(), + NewCurrentlyDrainedNodesPodListProcessor(), + NewFilterOutSchedulablePodListProcessor(predicateChecker), + NewFilterOutDaemonSetPodListProcessor(), + }) } diff --git a/cluster-autoscaler/main.go b/cluster-autoscaler/main.go index 5c1a07e6327e..5d7cf2acf648 100644 --- a/cluster-autoscaler/main.go +++ b/cluster-autoscaler/main.go @@ -251,7 +251,7 @@ var ( "--max-graceful-termination-sec flag should not be set when this flag is set. Not setting this flag will use unordered evictor by default."+ "Priority evictor reuses the concepts of drain logic in kubelet(https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2712-pod-priority-based-graceful-node-shutdown#migration-from-the-node-graceful-shutdown-feature)."+ "Eg. flag usage: '10000:20,1000:100,0:60'") - provisioningRequestsEnabled = flag.Bool("enable-provisioning-requests", false, "Whether the clusterautoscaler will be handling the ProvisioningRequest CRs.") + provisioningRequestsEnabled = flag.Bool("enable-provisioning-requests", false, "Whether the clusterautoscaler will be handling the ProvisioningRequest CRs.") ) func isFlagPassed(name string) bool { @@ -480,7 +480,7 @@ func buildAutoscaler(debuggingSnapshotter debuggingsnapshot.DebuggingSnapshotter opts.Processors.TemplateNodeInfoProvider = nodeinfosprovider.NewDefaultTemplateNodeInfoProvider(nodeInfoCacheExpireTime, *forceDaemonSets) podListProcessor := podlistprocessor.NewDefaultPodListProcessor(opts.PredicateChecker) if autoscalingOptions.ProvisioningRequestEnabled { - podListProcessor.AppendProcessor(provreq.NewProvisioningRequestPodsFilter(provreq.NewDefautlEventManager())) + podListProcessor.AddProcessor(provreq.NewProvisioningRequestPodsFilter(provreq.NewDefautlEventManager())) } opts.Processors.PodListProcessor = podListProcessor scaleDownCandidatesComparers := []scaledowncandidates.CandidatesComparer{} diff --git a/cluster-autoscaler/processors/pods/pod_list_processor.go b/cluster-autoscaler/processors/pods/pod_list_processor.go index cd3a5a3fd45e..65490dac51c6 100644 --- a/cluster-autoscaler/processors/pods/pod_list_processor.go +++ b/cluster-autoscaler/processors/pods/pod_list_processor.go @@ -49,15 +49,25 @@ func (p *NoOpPodListProcessor) Process( func (p *NoOpPodListProcessor) CleanUp() { } -// ListedPodListProcessor is a list of PodListProcessors -type ListedPodListProcessor struct { - Processors []PodListProcessor +// CombinedPodListProcessor is a list of PodListProcessors +type CombinedPodListProcessor struct { + processors []PodListProcessor +} + +// NewCombinedPodListProcessor construct CombinedPodListProcessor. +func NewCombinedPodListProcessor(processors []PodListProcessor) *CombinedPodListProcessor { + return &CombinedPodListProcessor{processors} +} + +// AddProcessor append processor to the list. +func (p *CombinedPodListProcessor) AddProcessor(processor PodListProcessor) { + p.processors = append(p.processors, processor) } // Process runs sub-processors sequentially -func (p *ListedPodListProcessor) Process(ctx *context.AutoscalingContext, unschedulablePods []*apiv1.Pod) ([]*apiv1.Pod, error) { +func (p *CombinedPodListProcessor) Process(ctx *context.AutoscalingContext, unschedulablePods []*apiv1.Pod) ([]*apiv1.Pod, error) { var err error - for _, processor := range p.Processors { + for _, processor := range p.processors { unschedulablePods, err = processor.Process(ctx, unschedulablePods) if err != nil { return nil, err @@ -67,13 +77,8 @@ func (p *ListedPodListProcessor) Process(ctx *context.AutoscalingContext, unsche } // CleanUp cleans up the processor's internal structures. -func (p *ListedPodListProcessor) CleanUp() { - for _, processor := range p.Processors { +func (p *CombinedPodListProcessor) CleanUp() { + for _, processor := range p.processors { processor.CleanUp() } } - -// AppendProcessor append processor to the list. -func (p *ListedPodListProcessor) AppendProcessor(processor PodListProcessor) { - p.Processors = append(p.Processors, processor) -} diff --git a/cluster-autoscaler/processors/provreq/provisioning_request_processors.go b/cluster-autoscaler/processors/provreq/provisioning_request_processors.go index d7bee0fa63e1..c48d489c0746 100644 --- a/cluster-autoscaler/processors/provreq/provisioning_request_processors.go +++ b/cluster-autoscaler/processors/provreq/provisioning_request_processors.go @@ -50,7 +50,7 @@ func NewDefautlEventManager() *defaultEventManager { // LogIgnoredInScaleUpEvent adds event about ignored scale up for unscheduled pod, that consumes Provisioning Request. func (e *defaultEventManager) LogIgnoredInScaleUpEvent(context *context.AutoscalingContext, now time.Time, pod *apiv1.Pod, prName string) { - message := fmt.Sprintf("Unschedulable pod ignored in scale-up loop, because it's consuming ProvisioningRequest %s/%s", pod.Namespace, prName) + message := fmt.Sprintf("Unschedulable pod didn't trigger scale-up, because it's consuming ProvisioningRequest %s/%s", pod.Namespace, prName) if e.loggedEvents < e.limit { context.Recorder.Event(pod, apiv1.EventTypeNormal, "", message) e.loggedEvents++ diff --git a/cluster-autoscaler/processors/provreq/provisioning_request_processors_test.go b/cluster-autoscaler/processors/provreq/provisioning_request_processors_test.go index d6c99e7b8385..487ecb91c473 100644 --- a/cluster-autoscaler/processors/provreq/provisioning_request_processors_test.go +++ b/cluster-autoscaler/processors/provreq/provisioning_request_processors_test.go @@ -73,7 +73,7 @@ func TestProvisioningRequestPodsFilter(t *testing.T) { if len(test.expectedUnscheduledPods) < len(test.expectedUnscheduledPods) { select { case event := <-eventRecorder.Events: - assert.Contains(t, event, "Unschedulable pod ignored in scale-up loop, because it's consuming ProvisioningRequest default/pr-class") + assert.Contains(t, event, "Unschedulable pod didn't trigger scale-up, because it's consuming ProvisioningRequest default/pr-class") case <-time.After(1 * time.Second): t.Errorf("Timeout waiting for event") } diff --git a/cluster-autoscaler/provisioningrequest/provreqclient/client.go b/cluster-autoscaler/provisioningrequest/provreqclient/client.go index db1852b35c51..f11c5cd42555 100644 --- a/cluster-autoscaler/provisioningrequest/provreqclient/client.go +++ b/cluster-autoscaler/provisioningrequest/provreqclient/client.go @@ -41,13 +41,6 @@ const ( provisioningRequestClientCallTimeout = 4 * time.Second ) -// ProvisioningRequestClient represents the service that is able to list -// and access different Provisioning Requests. -type ProvisioningRequestClient interface { - ProvisioningRequest(namespace, name string) (*provreqwrapper.ProvisioningRequest, error) - ProvisioningRequests() ([]*provreqwrapper.ProvisioningRequest, error) -} - // ProvisioningRequestClientV1beta1 represents client for v1beta1 ProvReq CRD. type ProvisioningRequestClientV1beta1 struct { client versioned.Interface @@ -56,7 +49,7 @@ type ProvisioningRequestClientV1beta1 struct { } // NewProvisioningRequestClient configures and returns a provisioningRequestClient. -func NewProvisioningRequestClient(kubeConfig *rest.Config) (ProvisioningRequestClient, error) { +func NewProvisioningRequestClient(kubeConfig *rest.Config) (*ProvisioningRequestClientV1beta1, error) { prClient, err := newPRClient(kubeConfig) if err != nil { return nil, fmt.Errorf("Failed to create Provisioning Request client: %v", err)