Skip to content

Commit

Permalink
review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
jmdeal committed Feb 22, 2024
1 parent 8cb7c57 commit d719ae1
Show file tree
Hide file tree
Showing 12 changed files with 94 additions and 18 deletions.
8 changes: 4 additions & 4 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ require (
go.uber.org/zap v1.26.0
golang.org/x/sync v0.6.0
golang.org/x/time v0.5.0
k8s.io/api v0.29.1
k8s.io/apiextensions-apiserver v0.29.1
k8s.io/apimachinery v0.29.1
k8s.io/client-go v0.29.1
k8s.io/api v0.29.2
k8s.io/apiextensions-apiserver v0.29.2
k8s.io/apimachinery v0.29.2
k8s.io/client-go v0.29.2
k8s.io/utils v0.0.0-20240102154912-e7106e64919e
knative.dev/pkg v0.0.0-20231010144348-ca8c009405dd
sigs.k8s.io/controller-runtime v0.17.2
Expand Down
5 changes: 1 addition & 4 deletions pkg/operator/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont
amiResolver := amifamily.New(amiProvider)
launchTemplateProvider := launchtemplate.NewProvider(
ctx,
cache.New(time.Hour, awscache.DefaultCleanupInterval), // Extending TTL for testing purposes, DO NOT MERGE!!!
cache.New(awscache.DefaultTTL, awscache.DefaultCleanupInterval),
ec2api,
amiResolver,
securityGroupProvider,
Expand Down Expand Up @@ -242,9 +242,6 @@ func ResolveClusterEndpoint(ctx context.Context, eksAPI eksiface.EKSAPI) (string
}

func ResolveClusterCIDR(ctx context.Context, eksAPI eksiface.EKSAPI) (string, error) {
if cidr := options.FromContext(ctx).ClusterCIDR; cidr != "" {
return cidr, nil
}
out, err := eksAPI.DescribeClusterWithContext(ctx, &eks.DescribeClusterInput{
Name: aws.String(options.FromContext(ctx).ClusterName),
})
Expand Down
2 changes: 0 additions & 2 deletions pkg/operator/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ type Options struct {
ClusterCABundle string
ClusterName string
ClusterEndpoint string
ClusterCIDR string
IsolatedVPC bool
VMMemoryOverheadPercent float64
InterruptionQueue string
Expand All @@ -51,7 +50,6 @@ func (o *Options) AddFlags(fs *coreoptions.FlagSet) {
fs.StringVar(&o.ClusterCABundle, "cluster-ca-bundle", env.WithDefaultString("CLUSTER_CA_BUNDLE", ""), "Cluster CA bundle for nodes to use for TLS connections with the API server. If not set, this is taken from the controller's TLS configuration.")
fs.StringVar(&o.ClusterName, "cluster-name", env.WithDefaultString("CLUSTER_NAME", ""), "[REQUIRED] The kubernetes cluster name for resource discovery.")
fs.StringVar(&o.ClusterEndpoint, "cluster-endpoint", env.WithDefaultString("CLUSTER_ENDPOINT", ""), "The external kubernetes cluster endpoint for new nodes to connect with. If not specified, will discover the cluster endpoint using DescribeCluster API.")
fs.StringVar(&o.ClusterCIDR, "cluster-cidr", env.WithDefaultString("CLUSTER_CIDR", ""), "The IP address range from which cluster services will receive IP addresses. Corresponds to cluster service IPv4 / IPv6 range.")
fs.BoolVarWithEnv(&o.IsolatedVPC, "isolated-vpc", "ISOLATED_VPC", false, "If true, then assume we can't reach AWS services which don't have a VPC endpoint. This also has the effect of disabling look-ups to the AWS pricing endpoint.")
fs.Float64Var(&o.VMMemoryOverheadPercent, "vm-memory-overhead-percent", env.WithDefaultFloat64("VM_MEMORY_OVERHEAD_PERCENT", 0.075), "The VM memory overhead as a percent that will be subtracted from the total memory for all instance types.")
fs.StringVar(&o.InterruptionQueue, "interruption-queue", env.WithDefaultString("INTERRUPTION_QUEUE", ""), "Interruption queue is disabled if not specified. Enabling interruption handling may require additional permissions on the controller service account. Additional permissions are outlined in the docs.")
Expand Down
5 changes: 0 additions & 5 deletions pkg/operator/options/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ var _ = Describe("Options", func() {
"--cluster-ca-bundle", "env-bundle",
"--cluster-name", "env-cluster",
"--cluster-endpoint", "https://env-cluster",
"--cluster-cidr", "10.100.0.0/16",
"--isolated-vpc",
"--vm-memory-overhead-percent", "0.1",
"--interruption-queue", "env-cluster",
Expand All @@ -74,7 +73,6 @@ var _ = Describe("Options", func() {
ClusterCABundle: lo.ToPtr("env-bundle"),
ClusterName: lo.ToPtr("env-cluster"),
ClusterEndpoint: lo.ToPtr("https://env-cluster"),
ClusterCIDR: lo.ToPtr("10.100.0.0/16"),
IsolatedVPC: lo.ToPtr(true),
VMMemoryOverheadPercent: lo.ToPtr[float64](0.1),
InterruptionQueue: lo.ToPtr("env-cluster"),
Expand All @@ -87,7 +85,6 @@ var _ = Describe("Options", func() {
os.Setenv("CLUSTER_CA_BUNDLE", "env-bundle")
os.Setenv("CLUSTER_NAME", "env-cluster")
os.Setenv("CLUSTER_ENDPOINT", "https://env-cluster")
os.Setenv("CLUSTER_CIDR", "10.100.0.0/16")
os.Setenv("ISOLATED_VPC", "true")
os.Setenv("VM_MEMORY_OVERHEAD_PERCENT", "0.1")
os.Setenv("INTERRUPTION_QUEUE", "env-cluster")
Expand All @@ -104,7 +101,6 @@ var _ = Describe("Options", func() {
ClusterCABundle: lo.ToPtr("env-bundle"),
ClusterName: lo.ToPtr("env-cluster"),
ClusterEndpoint: lo.ToPtr("https://env-cluster"),
ClusterCIDR: lo.ToPtr("10.100.0.0/16"),
IsolatedVPC: lo.ToPtr(true),
VMMemoryOverheadPercent: lo.ToPtr[float64](0.1),
InterruptionQueue: lo.ToPtr("env-cluster"),
Expand Down Expand Up @@ -146,7 +142,6 @@ func expectOptionsEqual(optsA *options.Options, optsB *options.Options) {
Expect(optsA.ClusterCABundle).To(Equal(optsB.ClusterCABundle))
Expect(optsA.ClusterName).To(Equal(optsB.ClusterName))
Expect(optsA.ClusterEndpoint).To(Equal(optsB.ClusterEndpoint))
Expect(optsA.ClusterCIDR).To(Equal(optsB.ClusterCIDR))
Expect(optsA.IsolatedVPC).To(Equal(optsB.IsolatedVPC))
Expect(optsA.VMMemoryOverheadPercent).To(Equal(optsB.VMMemoryOverheadPercent))
Expect(optsA.InterruptionQueue).To(Equal(optsB.InterruptionQueue))
Expand Down
30 changes: 30 additions & 0 deletions pkg/operator/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,34 @@ var _ = Describe("Operator", func() {
_, err := awscontext.ResolveClusterEndpoint(ctx, fakeEKSAPI)
Expect(err).To(HaveOccurred())
})
It("should resolve IPv4 cluster CIDR via call to API", func() {
ctx = options.ToContext(ctx, test.Options(test.OptionsFields{
ClusterName: lo.ToPtr("test-cluster"),
}))
fakeEKSAPI.DescribeClusterBehavior.Output.Set(&eks.DescribeClusterOutput{
Cluster: &eks.Cluster{
KubernetesNetworkConfig: &eks.KubernetesNetworkConfigResponse{
ServiceIpv4Cidr: lo.ToPtr("10.100.0.0/16"),
},
},
})
clusterCIDR, err := awscontext.ResolveClusterCIDR(ctx, fakeEKSAPI)
Expect(err).To(BeNil())
Expect(clusterCIDR).To(Equal("10.100.0.0/16"))
})
It("should resolve IPv6 cluster CIDR via call to API", func() {
ctx = options.ToContext(ctx, test.Options(test.OptionsFields{
ClusterName: lo.ToPtr("test-cluster"),
}))
fakeEKSAPI.DescribeClusterBehavior.Output.Set(&eks.DescribeClusterOutput{
Cluster: &eks.Cluster{
KubernetesNetworkConfig: &eks.KubernetesNetworkConfigResponse{
ServiceIpv6Cidr: lo.ToPtr("2001:db8::/64"),
},
},
})
clusterCIDR, err := awscontext.ResolveClusterCIDR(ctx, fakeEKSAPI)
Expect(err).To(BeNil())
Expect(clusterCIDR).To(Equal("2001:db8::/64"))
})
})
19 changes: 19 additions & 0 deletions pkg/providers/amifamily/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,16 @@ var _ = Describe("AMIProvider", func() {
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(4))
})
It("should succeed to resolve AMIs (AL2023)", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyAL2023
awsEnv.SSMAPI.Parameters = map[string]string{
fmt.Sprintf("/aws/service/eks/optimized-ami/%s/amazon-linux-2023/x86_64/standard/recommended/image_id", version): amd64AMI,
fmt.Sprintf("/aws/service/eks/optimized-ami/%s/amazon-linux-2023/arm64/standard/recommended/image_id", version): arm64AMI,
}
amis, err := awsEnv.AMIProvider.Get(ctx, nodeClass, &amifamily.Options{})
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(2))
})
It("should succeed to resolve AMIs (Bottlerocket)", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyBottlerocket
awsEnv.SSMAPI.Parameters = map[string]string{
Expand Down Expand Up @@ -199,6 +209,15 @@ var _ = Describe("AMIProvider", func() {
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(2))
})
It("should succeed to partially resolve AMIs if all SSM aliases don't exist (AL2023)", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyAL2023
awsEnv.SSMAPI.Parameters = map[string]string{
fmt.Sprintf("/aws/service/eks/optimized-ami/%s/amazon-linux-2023/x86_64/standard/recommended/image_id", version): amd64AMI,
}
amis, err := awsEnv.AMIProvider.Get(ctx, nodeClass, &amifamily.Options{})
Expect(err).ToNot(HaveOccurred())
Expect(amis).To(HaveLen(1))
})
It("should succeed to partially resolve AMIs if all SSM aliases don't exist (Bottlerocket)", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyBottlerocket
// No GPU AMI exists for AM64 here
Expand Down
15 changes: 15 additions & 0 deletions pkg/providers/instancetype/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1594,6 +1594,21 @@ var _ = Describe("InstanceTypes", func() {
Expect(*ltInput.LaunchTemplateData.BlockDeviceMappings[0].Ebs.SnapshotId).To(Equal("snap-xxxxxxxx"))
})
})
It("should default to EBS defaults when volumeSize is not defined in blockDeviceMappings for AL2023 Root volume", func() {
nodeClass.Spec.AMIFamily = aws.String(v1beta1.AMIFamilyAL2023)
awsEnv.LaunchTemplateProvider.CABundle = lo.ToPtr("Y2EtYnVuZGxlCg==")
ExpectApplied(ctx, env.Client, nodePool, nodeClass)
pod := coretest.UnschedulablePod()
ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod)
node := ExpectScheduled(ctx, env.Client, pod)
Expect(*node.Status.Capacity.StorageEphemeral()).To(Equal(resource.MustParse("20Gi")))
Expect(awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Len()).To(BeNumerically(">=", 1))
awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.ForEach(func(ltInput *ec2.CreateLaunchTemplateInput) {
Expect(ltInput.LaunchTemplateData.BlockDeviceMappings).To(HaveLen(1))
Expect(*ltInput.LaunchTemplateData.BlockDeviceMappings[0].DeviceName).To(Equal("/dev/xvda"))
Expect(*ltInput.LaunchTemplateData.BlockDeviceMappings[0].Ebs.SnapshotId).To(Equal("snap-xxxxxxxx"))
})
})
It("should default to EBS defaults when volumeSize is not defined in blockDeviceMappings for Bottlerocket Root volume", func() {
nodeClass.Spec.AMIFamily = aws.String(v1beta1.AMIFamilyBottlerocket)
nodeClass.Spec.BlockDeviceMappings[0].DeviceName = aws.String("/dev/xvdb")
Expand Down
2 changes: 2 additions & 0 deletions pkg/providers/instancetype/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,10 @@ func memory(ctx context.Context, info *ec2.InstanceTypeInfo) *resource.Quantity
}

// Setting ephemeral-storage to be either the default value, what is defined in blockDeviceMappings, or the combined size of local store volumes.
// nolint:gocyclo
func ephemeralStorage(info *ec2.InstanceTypeInfo, amiFamily amifamily.AMIFamily, nodeClass *v1beta1.EC2NodeClass) *resource.Quantity {
// If local store disks have been configured for node ephemeral-storage, use the total size of the disks.
// if *nodeClass.Spec.AMIFamily == v1beta1.AMIFamilyAL2023 || lo.FromPtr(nodeClass.Spec.InstanceStorePolicy) == v1beta1.InstanceStorePolicyRAID0 {
if lo.FromPtr(nodeClass.Spec.InstanceStorePolicy) == v1beta1.InstanceStorePolicyRAID0 {
if info.InstanceStorageInfo != nil && info.InstanceStorageInfo.TotalSizeInGB != nil {
return resources.Quantity(fmt.Sprintf("%dG", *info.InstanceStorageInfo.TotalSizeInGB))
Expand Down
16 changes: 15 additions & 1 deletion pkg/providers/launchtemplate/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,21 @@ var _ = Describe("LaunchTemplates", func() {
Expect(ltInput.LaunchTemplateData.BlockDeviceMappings[0].Ebs.Iops).To(BeNil())
})
})
It("should default AL2023 block device mappings", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyAL2023
awsEnv.LaunchTemplateProvider.CABundle = lo.ToPtr("Y2EtYnVuZGxlCg==")
ExpectApplied(ctx, env.Client, nodePool, nodeClass)
pod := coretest.UnschedulablePod()
ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, pod)
ExpectScheduled(ctx, env.Client, pod)
Expect(awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.Len()).To(BeNumerically(">=", 1))
awsEnv.EC2API.CalledWithCreateLaunchTemplateInput.ForEach(func(ltInput *ec2.CreateLaunchTemplateInput) {
Expect(len(ltInput.LaunchTemplateData.BlockDeviceMappings)).To(Equal(1))
Expect(*ltInput.LaunchTemplateData.BlockDeviceMappings[0].Ebs.VolumeSize).To(Equal(int64(20)))
Expect(*ltInput.LaunchTemplateData.BlockDeviceMappings[0].Ebs.VolumeType).To(Equal("gp3"))
Expect(ltInput.LaunchTemplateData.BlockDeviceMappings[0].Ebs.Iops).To(BeNil())
})
})
It("should use custom block device mapping", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyAL2
nodeClass.Spec.BlockDeviceMappings = []*v1beta1.BlockDeviceMapping{
Expand Down Expand Up @@ -1398,7 +1413,6 @@ var _ = Describe("LaunchTemplates", func() {

// base64 encoded version of "ca-bundle" to ensure the nodeadm bootstrap provider can decode successfully
awsEnv.LaunchTemplateProvider.CABundle = lo.ToPtr("Y2EtYnVuZGxlCg==")
options.FromContext(ctx).ClusterCIDR = "10.100.0.0/16"
})
Context("Kubelet", func() {
It("should specify taints in the KubeletConfiguration when specified in NodePool", func() {
Expand Down
2 changes: 0 additions & 2 deletions pkg/test/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ type OptionsFields struct {
ClusterCABundle *string
ClusterName *string
ClusterEndpoint *string
ClusterCIDR *string
IsolatedVPC *bool
VMMemoryOverheadPercent *float64
InterruptionQueue *string
Expand All @@ -50,7 +49,6 @@ func Options(overrides ...OptionsFields) *options.Options {
ClusterCABundle: lo.FromPtrOr(opts.ClusterCABundle, ""),
ClusterName: lo.FromPtrOr(opts.ClusterName, "test-cluster"),
ClusterEndpoint: lo.FromPtrOr(opts.ClusterEndpoint, "https://test-cluster"),
ClusterCIDR: lo.FromPtrOr(opts.ClusterCIDR, ""),
IsolatedVPC: lo.FromPtrOr(opts.IsolatedVPC, false),
VMMemoryOverheadPercent: lo.FromPtrOr(opts.VMMemoryOverheadPercent, 0.075),
InterruptionQueue: lo.FromPtrOr(opts.InterruptionQueue, ""),
Expand Down
7 changes: 7 additions & 0 deletions test/suites/integration/ami_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,13 @@ var _ = Describe("AMI", func() {
env.EventuallyExpectHealthy(pod)
env.ExpectCreatedNodeCount("==", 1)
})
It("should provision a node using the AL2023 family", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyAL2023
pod := coretest.Pod()
env.ExpectCreated(nodeClass, nodePool, pod)
env.EventuallyExpectHealthy(pod)
env.ExpectCreatedNodeCount("==", 1)
})
It("should provision a node using the Bottlerocket family", func() {
nodeClass.Spec.AMIFamily = &v1beta1.AMIFamilyBottlerocket
pod := coretest.Pod()
Expand Down
1 change: 1 addition & 0 deletions test/suites/integration/kubelet_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ var _ = Describe("KubeletConfiguration Overrides", func() {
env.ExpectCreatedNodeCount("==", 1)
},
Entry("when the AMIFamily is AL2", &v1beta1.AMIFamilyAL2),
Entry("when the AMIFamily is AL2023", &v1beta1.AMIFamilyAL2023),
Entry("when the AMIFamily is Ubuntu", &v1beta1.AMIFamilyUbuntu),
Entry("when the AMIFamily is Bottlerocket", &v1beta1.AMIFamilyBottlerocket),
)
Expand Down

0 comments on commit d719ae1

Please sign in to comment.