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 e466622
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 81 deletions.
24 changes: 7 additions & 17 deletions pkg/config/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,14 @@ 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.V(1).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")
Expand All @@ -147,12 +142,9 @@ 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)
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")
Expand All @@ -161,11 +153,9 @@ func ParseWinIPTargetConfigs(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (wa

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

Expand All @@ -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,
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
4 changes: 2 additions & 2 deletions pkg/provider/ip/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions pkg/provider/prefix/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
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 e466622

Please sign in to comment.