Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove InstancesDistribution from unmanaged nodegroup cfn #448

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Remove InstancesDistribution from unmanaged nodegroup cfn #448

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13ee4e9
Add features flag to eksapi deployer
ndbaker1 Jun 12, 2024
397dc89
Revert "Add features flag to eksapi deployer"
ndbaker1 Jun 12, 2024
afe0ab9
Remove InstancesDistribution from unmanaged nodegroup cfn
ndbaker1 Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions kubetest2/internal/deployers/eksapi/deployer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"path"
"path/filepath"
"strings"
"time"

"github.com/aws/aws-k8s-tester/kubetest2/internal"
Expand Down Expand Up @@ -39,6 +40,11 @@ var (
}
)

// default features
const (
InstancesDistribution = "InstancesDistribution"
)

// assert that deployer implements optional interfaces
var _ types.DeployerWithKubeconfig = &deployer{}
var _ types.DeployerWithInit = &deployer{}
Expand Down Expand Up @@ -82,6 +88,10 @@ type deployerOptions struct {
UnmanagedNodes bool `flag:"unmanaged-nodes" desc:"Use an AutoScalingGroup instead of an EKS-managed nodegroup. Requires --ami"`
UpClusterHeaders []string `flag:"up-cluster-header" desc:"Additional header to add to eks:CreateCluster requests. Specified in the same format as curl's -H flag."`
UserDataFormat string `flag:"user-data-format" desc:"Format of the node instance user data"`
Features []string `flag:"features" desc:"feature toggles for the nodegroup deployer"`

// private field for configurations of cloud formation templates
featureMap map[string]bool
}

// NewDeployer implements deployer.New for EKS using the EKS (and other AWS) API(s) directly (no cloudformation)
Expand Down Expand Up @@ -269,6 +279,36 @@ func (d *deployer) verifyUpFlags() error {
if d.NodeReadyTimeout == 0 {
d.NodeReadyTimeout = time.Minute * 5
}

// convert features to featureMap
d.featureMap = make(map[string]bool)
for _, feature := range d.Features {
parts := strings.Split(feature, "=")
if len(parts) != 2 {
return fmt.Errorf("feature key-value pair '%s' is incorrectly formatted", feature)
}
featureKey, featureValue := parts[0], parts[1]
var featureOn bool
if featureValue == "true" {
featureOn = true
} else if featureValue == "false" {
featureOn = false
} else {
return fmt.Errorf("feature value for '%s' must one of {true,false}", feature)
}

d.featureMap[featureKey] = featureOn
}

// set default features
for name, value := range map[string]bool{
InstancesDistribution: true,
} {
if _, ok := d.featureMap[name]; !ok {
d.featureMap[name] = value
}
}

return nil
}

Expand Down
2 changes: 1 addition & 1 deletion kubetest2/internal/deployers/eksapi/infra.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ func (m *InfrastructureManager) deleteLeakedENIs() error {
}
}
klog.Infof("deleted %d leaked ENI(s)!", len(enis))
m.metrics.Record(infraLeakedENIs, float64(len(enis)), nil)
m.metrics.Record(infraLeakedENIs, float64(len(enis)), nil)
return nil
}

Expand Down
20 changes: 11 additions & 9 deletions kubetest2/internal/deployers/eksapi/nodegroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,10 @@ func (m *NodegroupManager) createUnmanagedNodegroup(infra *Infrastructure, clust
return err
}
templateBuf := bytes.Buffer{}
err = templates.UnmanagedNodegroup.Execute(&templateBuf, struct {
InstanceTypes []string
KubernetesVersion string
}{
InstanceTypes: opts.InstanceTypes,
KubernetesVersion: opts.KubernetesVersion,
})
if err != nil {
if err = templates.UnmanagedNodegroup.Execute(&templateBuf, templates.UnmanagedNodegroupTemplateData{
InstanceTypes: opts.InstanceTypes,
Features: opts.featureMap,
}); err != nil {
return err
}
// pull the role name out of the ARN
Expand Down Expand Up @@ -211,12 +207,18 @@ func (m *NodegroupManager) createUnmanagedNodegroupWithEFA(infra *Infrastructure
if err != nil {
return err
}
templateBuf := bytes.Buffer{}
if err = templates.UnmanagedNodegroupEFA.Execute(&templateBuf, templates.UnmanagedNodegroupEFATemplateData{
Features: opts.featureMap,
}); err != nil {
return err
}
// pull the role name out of the ARN
nodeRoleArnParts := strings.Split(infra.nodeRole, "/")
nodeRoleName := nodeRoleArnParts[len(nodeRoleArnParts)-1]
input := cloudformation.CreateStackInput{
StackName: aws.String(stackName),
TemplateBody: aws.String(templates.UnmanagedNodegroupEFA),
TemplateBody: aws.String(templateBuf.String()),
Capabilities: []cloudformationtypes.Capability{cloudformationtypes.CapabilityCapabilityIam},
Parameters: []cloudformationtypes.Parameter{
{
Expand Down
15 changes: 10 additions & 5 deletions kubetest2/internal/deployers/eksapi/templates/templates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,23 @@ import (
//go:embed infra.yaml
var Infrastructure string

//go:embed unmanaged-nodegroup-efa.yaml
var UnmanagedNodegroupEFA string

var (
//go:embed unmanaged-nodegroup-efa.yaml.template
UnmanagedNodegroupEFATemplate string
UnmanagedNodegroupEFA = template.Must(template.New("unmanagedNodegroupEFA").Parse(UnmanagedNodegroupEFATemplate))

//go:embed unmanaged-nodegroup.yaml.template
unmanagedNodegroupTemplate string
UnmanagedNodegroup = template.Must(template.New("unmanagedNodegroup").Parse(unmanagedNodegroupTemplate))
)

type UnmanagedNodegroupTemplateData struct {
KubernetesVersion string
InstanceTypes []string
InstanceTypes []string
Features map[string]bool
}

type UnmanagedNodegroupEFATemplateData struct {
Features map[string]bool
}

var (
Expand Down
1 change: 0 additions & 1 deletion kubetest2/internal/deployers/eksapi/templates/templates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
func Test_UnmanagedNodegroup(t *testing.T) {
buf := bytes.Buffer{}
err := UnmanagedNodegroup.Execute(&buf, UnmanagedNodegroupTemplateData{
KubernetesVersion: "1.28",
InstanceTypes: []string{
"t2.medium",
"t2.large",
Expand Down
2 changes: 2 additions & 0 deletions ...pi/templates/unmanaged-nodegroup-efa.yaml → ...tes/unmanaged-nodegroup-efa.yaml.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,10 @@ Resources:
Properties:
AutoScalingGroupName: !Ref ResourceId
MixedInstancesPolicy:
{{- if .Features.InstancesDistribution }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we even need the InstancedDistribution? I just copied this from an unmanaged nodegroup example to begin with, wr can probably just remove it entirely if it's not available everywhere.

or we can just use a CFN intrinsic func to switch this off in certain partitions, if that's what we're getting at. Would be easier than pushing that logic up to the user of the deployer IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case i think i'd rather pull it out because it doesn't seem required 🤔

not sure if the parameters will affect runtimes based on capacity of Spot vs OnDemand, but the instance type prioritization seems like the only real different behavior

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, can you test and make sure the nodes still come up without this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ran a test successfully and updated the overview with the steps

InstancesDistribution:
OnDemandAllocationStrategy: "prioritized"
{{- end }}
LaunchTemplate:
LaunchTemplateSpecification:
LaunchTemplateId: !Ref NodeLaunchTemplate
Expand Down
2 changes: 2 additions & 0 deletions kubetest2/internal/deployers/eksapi/templates/unmanaged-nodegroup.yaml.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,12 @@ Resources:
MinSize: !Ref NodeCount
MaxSize: !Ref NodeCount
MixedInstancesPolicy:
{{- if .Features.InstancesDistribution }}
InstancesDistribution:
OnDemandAllocationStrategy: prioritized
OnDemandBaseCapacity: !Ref NodeCount
OnDemandPercentageAboveBaseCapacity: 0
{{- end }}
LaunchTemplate:
LaunchTemplateSpecification:
LaunchTemplateId: !Ref NodeLaunchTemplate
Expand Down
Loading