Skip to content

Commit

Permalink
Various code fixes for PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
tzifudzi committed Sep 4, 2024
1 parent 1dd0d2e commit 6bb794f
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 67 deletions.
11 changes: 4 additions & 7 deletions pkg/config/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
Expand All @@ -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)
}
}

Expand All @@ -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,
Expand Down
39 changes: 1 addition & 38 deletions pkg/pool/pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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
}

Expand Down
39 changes: 39 additions & 0 deletions pkg/pool/utils.go
Original file line number Diff line number Diff line change
@@ -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
}
}
2 changes: 1 addition & 1 deletion pkg/provider/provider_test.go → pkg/pool/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
21 changes: 0 additions & 21 deletions pkg/provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}
}

0 comments on commit 6bb794f

Please sign in to comment.