From 6bb794f33a137147f429a7860783749f2d1ca68d Mon Sep 17 00:00:00 2001 From: Tatenda Zifudzi Date: Wed, 4 Sep 2024 09:38:59 -0700 Subject: [PATCH] Various code fixes for PR feedback https://github.com/aws/amazon-vpc-resource-controller-k8s/pull/443 --- pkg/config/loader.go | 11 ++---- pkg/pool/pool.go | 39 +------------------ pkg/pool/utils.go | 39 +++++++++++++++++++ .../provider_test.go => pool/utils_test.go} | 2 +- pkg/provider/provider.go | 21 ---------- 5 files changed, 45 insertions(+), 67 deletions(-) create mode 100644 pkg/pool/utils.go rename pkg/{provider/provider_test.go => pool/utils_test.go} (99%) diff --git a/pkg/config/loader.go b/pkg/config/loader.go index 5b4a116c..46041156 100644 --- a/pkg/config/loader.go +++ b/pkg/config/loader.go @@ -126,12 +126,11 @@ func ParseWinIPTargetConfigs(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (wa // If warm IP target config value is not found, or there is an error parsing it, the value will be set to zero if foundWarmIP { - warmIPTargetInt, err := strconv.Atoi(warmIPTargetStr) if err != nil { log.Info("Could not parse warm ip target, defaulting to zero", "warm ip target", warmIPTargetStr) warmIPTarget = 0 } else { - warmIPTarget = warmIPTargetInt + warmIPTarget, err = strconv.Atoi(warmIPTargetStr) // Handle secondary IP mode scenario where WarmIPTarget is explicitly configured to zero // In such a case there must always be 1 warm IP to ensure that the warmpool is never empty @@ -147,12 +146,11 @@ func ParseWinIPTargetConfigs(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (wa // If min IP target config value is not found, or there is an error parsing it, the value will be set to zero if foundMinIP { - minIPTargetInt, err := strconv.Atoi(minIPTargetStr) if err != nil { log.Info("Could not parse minimum ip target, defaulting to zero", "minimum ip target", minIPTargetStr) minIPTarget = 0 } else { - minIPTarget = minIPTargetInt + minIPTarget, err = strconv.Atoi(minIPTargetStr) } } else { log.V(1).Info("could not find minimum ip target in ConfigMap, defaulting to zero") @@ -161,11 +159,10 @@ func ParseWinIPTargetConfigs(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (wa warmPrefixTarget = 0 if isPDEnabled && foundWarmPrefix { - warmPrefixTargetInt, err := strconv.Atoi(warmPrefixTargetStr) if err != nil { log.Info("Could not parse warm prefix target, defaulting to zero", "warm prefix target", warmPrefixTargetStr) } else { - warmPrefixTarget = warmPrefixTargetInt + warmPrefixTarget, err = strconv.Atoi(warmPrefixTargetStr) } } @@ -178,7 +175,7 @@ func ParseWinIPTargetConfigs(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (wa minIPTarget = IPv4DefaultWinMinIPTarget warmIPTarget = IPv4DefaultWinWarmIPTarget } - log.V(1).Info( + log.Info( "Encountered zero values for warm-ip-target, min-ip-target and warm-prefix-target in ConfigMap data, falling back to using default values since on demand IP allocation is not supported", "minIPTarget", minIPTarget, "warmIPTarget", warmIPTarget, diff --git a/pkg/pool/pool.go b/pkg/pool/pool.go index 8f5fe26b..193312dd 100644 --- a/pkg/pool/pool.go +++ b/pkg/pool/pool.go @@ -424,21 +424,6 @@ func (p *pool) ReconcilePool() *worker.WarmPoolJob { len(p.usedResources), "pending create", p.pendingCreate, "pending delete", p.pendingDelete, "cool down queue", len(p.coolDownQueue), "total resources", totalCreatedResources, "capacity", p.capacity) - p.log.V(1).Info( - "Reconciling pool", - "isPDPool", p.isPDPool, - "reSyncRequired", p.reSyncRequired, - "minIPTarget", p.warmPoolConfig.MinIPTarget, - "warmIPTarget", p.warmPoolConfig.WarmIPTarget, - "numWarmResources", numWarmResources, - "used resouces", len(p.usedResources), - "cool down queue", len(p.coolDownQueue), - "total resources", totalCreatedResources, - "pendingCreate", p.pendingCreate, - "pendingDelete", p.pendingDelete, - "capacity", p.capacity, - ) - if p.reSyncRequired { // If Pending operations are present then we can't re-sync as the upstream // and pool could change during re-sync @@ -745,7 +730,6 @@ func (p *pool) calculateSecondaryIPDeviation() int { // warm pool is in draining state, set targets to zero if p.warmPoolConfig.DesiredSize == 0 { - p.log.V(1).Info("DesiredSize is zero, warmPool is in draining state") p.warmPoolConfig.WarmIPTarget = 0 p.warmPoolConfig.MinIPTarget = 0 p.warmPoolConfig.WarmPrefixTarget = 0 @@ -755,22 +739,14 @@ func (p *pool) calculateSecondaryIPDeviation() int { isWarmIPTargetInvalid := p.warmPoolConfig.WarmIPTarget < 0 // Handle scenario where MinIPTarget is configured to negative integer which is invalid if isMinIPTargetInvalid { - p.log.V(1).Info( - "MinIPTarget value is invalid negative integer, setting MinIPTarget to default", - "IPv4DefaultWinMinIPTarget", config.IPv4DefaultWinMinIPTarget, - ) p.warmPoolConfig.MinIPTarget = config.IPv4DefaultWinMinIPTarget } // Handle scenario where WarmIPTarget is configured to negative integer which is invalid if isWarmIPTargetInvalid { - p.log.V(1).Info( - "WarmIPTarget value is invalid negative integer, setting warmIPTarget to default", - "IPv4DefaultWinWarmIPTarget", config.IPv4DefaultWinWarmIPTarget, - ) p.warmPoolConfig.WarmIPTarget = config.IPv4DefaultWinWarmIPTarget } - availableResources := numWarmResources + p.pendingCreate - p.pendingDelete + availableResources := numWarmResources + p.pendingCreate // Calculate how many IPs we're short of the warm target resourcesShort := max(p.warmPoolConfig.WarmIPTarget-availableResources, 0) @@ -787,19 +763,6 @@ func (p *pool) calculateSecondaryIPDeviation() int { // The final deviation is the difference between short and over deviation := resourcesShort - resourcesOver - p.log.V(1).Info( - "Finished calculating IP deviation for secondary IP pool", - "minIPTarget", p.warmPoolConfig.MinIPTarget, - "warmIPTarget", p.warmPoolConfig.WarmIPTarget, - "numWarmResources", numWarmResources, - "numUsedResources", numUsedResources, - "numAssigned", numAssignedResources, - "availableResources", availableResources, - "resourcesShort", resourcesShort, - "resourcesOver", resourcesOver, - "deviationResult", deviation, - ) - return deviation } diff --git a/pkg/pool/utils.go b/pkg/pool/utils.go new file mode 100644 index 00000000..e729e081 --- /dev/null +++ b/pkg/pool/utils.go @@ -0,0 +1,39 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file 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 pool + +import ( + "github.com/go-logr/logr" + + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" +) + +// GetWinWarmPoolConfig retrieves Windows warmpool configuration from ConfigMap, falls back to using default values on failure +func GetWinWarmPoolConfig(log logr.Logger, w api.Wrapper, isPDEnabled bool) *config.WarmPoolConfig { + var resourceConfig map[string]config.ResourceConfig + vpcCniConfigMap, err := w.K8sAPI.GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace) + if err == nil { + resourceConfig = config.LoadResourceConfigFromConfigMap(log, vpcCniConfigMap) + } else { + log.Error(err, "failed to read from config map, will use default resource config") + resourceConfig = config.LoadResourceConfig() + } + + if isPDEnabled { + return resourceConfig[config.ResourceNameIPAddressFromPrefix].WarmPoolConfig + } else { + return resourceConfig[config.ResourceNameIPAddress].WarmPoolConfig + } +} diff --git a/pkg/provider/provider_test.go b/pkg/pool/utils_test.go similarity index 99% rename from pkg/provider/provider_test.go rename to pkg/pool/utils_test.go index ef3331ba..e41d12bf 100644 --- a/pkg/provider/provider_test.go +++ b/pkg/pool/utils_test.go @@ -11,7 +11,7 @@ // express or implied. See the License for the specific language governing // permissions and limitations under the License. -package provider +package pool import ( "fmt" diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 44a3ecfc..b5c80a6a 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -14,13 +14,10 @@ package provider import ( - "github.com/go-logr/logr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" - "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2" - "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/pool" ) @@ -50,21 +47,3 @@ type ResourceProvider interface { IntrospectSummary() interface{} ReconcileNode(nodeName string) bool } - -// GetWinWarmPoolConfig retrieves Windows warmpool configuration from ConfigMap, falls back to using default values on failure -func GetWinWarmPoolConfig(log logr.Logger, w api.Wrapper, isPDEnabled bool) *config.WarmPoolConfig { - var resourceConfig map[string]config.ResourceConfig - vpcCniConfigMap, err := w.K8sAPI.GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace) - if err == nil { - resourceConfig = config.LoadResourceConfigFromConfigMap(log, vpcCniConfigMap) - } else { - log.Error(err, "failed to read from config map, will use default resource config") - resourceConfig = config.LoadResourceConfig() - } - - if isPDEnabled { - return resourceConfig[config.ResourceNameIPAddressFromPrefix].WarmPoolConfig - } else { - return resourceConfig[config.ResourceNameIPAddress].WarmPoolConfig - } -}