Various code fixes for PR feedback

#443

Author: tzifudzi

pkg/config/loader.go

@@ -97,7 +97,7 @@ func ParseWinIPTargetConfigs(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (wa
 	if vpcCniConfigMap.Data == nil {
 		warmIPTarget = IPv4DefaultWinWarmIPTarget
 		minIPTarget = IPv4DefaultWinMinIPTarget
-		log.V(1).Info(
+		log.Info(
 			"No ConfigMap data found, falling back to using default values",
 			"minIPTarget", minIPTarget,
 			"warmIPTarget", warmIPTarget,
@@ -126,46 +126,36 @@ 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)
+		warmIPTarget, 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
-
+		} else if !isPDEnabled && warmIPTarget == 0 {
 			// 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
-			if !isPDEnabled && warmIPTarget == 0 {
-				log.V(1).Info("Explicitly setting WarmIPTarget zero value not supported in secondary IP mode, will override with 1")
-				warmIPTarget = 1
-			}
+			log.Info("Explicitly setting WarmIPTarget zero value not supported in secondary IP mode, will override with 1")
+			warmIPTarget = 1
 		}
 	} else {
-		log.V(1).Info("could not find warm ip target in ConfigMap, defaulting to zero")
+		log.Info("could not find warm ip target in ConfigMap, defaulting to zero")
 		warmIPTarget = 0
 	}
 
 	// 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)
+		minIPTarget, 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
 		}
 	} else {
-		log.V(1).Info("could not find minimum ip target in ConfigMap, defaulting to zero")
+		log.Info("could not find minimum ip target in ConfigMap, defaulting to zero")
 		minIPTarget = 0
 	}
 
 	warmPrefixTarget = 0
 	if isPDEnabled && foundWarmPrefix {
-		warmPrefixTargetInt, err := strconv.Atoi(warmPrefixTargetStr)
+		warmPrefixTarget, 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
 		}
 	}
 
@@ -178,7 +168,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,

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
 }
 

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
+	}
+}

pkg/provider/provider_test.go renamed to 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"

pkg/provider/ip/provider.go

@@ -154,7 +154,7 @@ func (p *ipv4Provider) InitResource(instance ec2.EC2Instance) error {
 	nodeCapacity := getCapacity(instance.Type(), instance.Os())
 
 	isPDEnabled := p.conditions.IsWindowsPrefixDelegationEnabled()
-	p.config = provider.GetWinWarmPoolConfig(p.log, p.apiWrapper, isPDEnabled)
+	p.config = pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isPDEnabled)
 
 	// Set warm pool config to empty config if PD is enabled
 	secondaryIPWPConfig := p.config
@@ -241,7 +241,7 @@ func (p *ipv4Provider) UpdateResourceCapacity(instance ec2.EC2Instance) error {
 
 	resourceProviderAndPool.isPrevPDEnabled = false
 
-	p.config = provider.GetWinWarmPoolConfig(p.log, p.apiWrapper, isCurrPDEnabled && isNitroInstance)
+	p.config = pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isCurrPDEnabled && isNitroInstance)
 
 	// Set the secondary IP provider pool state to active
 	job := resourceProviderAndPool.resourcePool.SetToActive(p.config)

pkg/provider/prefix/provider.go

@@ -156,7 +156,7 @@ func (p *ipv4PrefixProvider) InitResource(instance ec2.EC2Instance) error {
 	nodeCapacity := getCapacity(instance.Type(), instance.Os()) * pool.NumIPv4AddrPerPrefix
 
 	isPDEnabled := p.conditions.IsWindowsPrefixDelegationEnabled()
-	p.config = provider.GetWinWarmPoolConfig(p.log, p.apiWrapper, isPDEnabled)
+	p.config = pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isPDEnabled)
 
 	// Set warm pool config to empty if PD is not enabled
 	prefixIPWPConfig := p.config
@@ -234,7 +234,7 @@ func (p *ipv4PrefixProvider) UpdateResourceCapacity(instance ec2.EC2Instance) er
 
 	resourceProviderAndPool.isPrevPDEnabled = true
 
-	warmPoolConfig := provider.GetWinWarmPoolConfig(p.log, p.apiWrapper, isCurrPDEnabled)
+	warmPoolConfig := pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isCurrPDEnabled)
 
 	// Set the secondary IP provider pool state to active
 	job := resourceProviderAndPool.resourcePool.SetToActive(warmPoolConfig)

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
-	}
-}