diff --git a/pkg/apis/v1beta1/ec2nodeclass.go b/pkg/apis/v1beta1/ec2nodeclass.go index 5c264202bdde..b5e96ac1dc6b 100644 --- a/pkg/apis/v1beta1/ec2nodeclass.go +++ b/pkg/apis/v1beta1/ec2nodeclass.go @@ -293,7 +293,7 @@ type BlockDevice struct { // +kubebuilder:validation:Pattern:="^((?:[1-9][0-9]{0,3}|[1-4][0-9]{4}|[5][0-8][0-9]{3}|59000)Gi|(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|[6][0-3][0-9]{3}|64000)G|([1-9]||[1-5][0-7]|58)Ti|([1-9]||[1-5][0-9]|6[0-3]|64)T)$" // +kubebuilder:validation:XIntOrString // +optional - VolumeSize *resource.Quantity `json:"volumeSize,omitempty"` + VolumeSize *resource.Quantity `json:"volumeSize,omitempty" hash:"string"` // VolumeType of the block device. // For more information, see Amazon EBS volume types (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSVolumeTypes.html) // in the Amazon Elastic Compute Cloud User Guide. @@ -333,7 +333,7 @@ type EC2NodeClass struct { // 1. A field changes its default value for an existing field that is already hashed // 2. A field is added to the hash calculation with an already-set value // 3. A field is removed from the hash calculations -const EC2NodeClassHashVersion = "v1" +const EC2NodeClassHashVersion = "v2" func (in *EC2NodeClass) Hash() string { return fmt.Sprint(lo.Must(hashstructure.Hash(in.Spec, hashstructure.FormatV2, &hashstructure.HashOptions{ diff --git a/pkg/apis/v1beta1/ec2nodeclass_hash_test.go b/pkg/apis/v1beta1/ec2nodeclass_hash_test.go index 9581d09940e9..ccdde5a389ec 100644 --- a/pkg/apis/v1beta1/ec2nodeclass_hash_test.go +++ b/pkg/apis/v1beta1/ec2nodeclass_hash_test.go @@ -18,6 +18,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/imdario/mergo" "github.com/samber/lo" + "k8s.io/apimachinery/pkg/api/resource" "github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1" "github.com/aws/karpenter-provider-aws/pkg/test" @@ -27,76 +28,137 @@ import ( ) var _ = Describe("Hash", func() { - const staticHash = "16608948681250225098" + const staticHash = "10790156025840984195" var nodeClass *v1beta1.EC2NodeClass BeforeEach(func() { nodeClass = test.EC2NodeClass(v1beta1.EC2NodeClass{ Spec: v1beta1.EC2NodeClassSpec{ - AMIFamily: aws.String(v1beta1.AMIFamilyAL2), - Context: aws.String("context-1"), + AMIFamily: lo.ToPtr(v1beta1.AMIFamilyAL2023), Role: "role-1", Tags: map[string]string{ "keyTag-1": "valueTag-1", "keyTag-2": "valueTag-2", }, + Context: lo.ToPtr("fake-context"), + DetailedMonitoring: lo.ToPtr(false), + AssociatePublicIPAddress: lo.ToPtr(false), MetadataOptions: &v1beta1.MetadataOptions{ - HTTPEndpoint: aws.String("test-metadata-1"), + HTTPEndpoint: lo.ToPtr("disabled"), + HTTPProtocolIPv6: lo.ToPtr("disabled"), + HTTPPutResponseHopLimit: lo.ToPtr(int64(1)), + HTTPTokens: lo.ToPtr("optional"), }, BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{ { - DeviceName: aws.String("map-device-1"), + DeviceName: lo.ToPtr("map-device-1"), + RootVolume: false, + EBS: &v1beta1.BlockDevice{ + DeleteOnTermination: lo.ToPtr(false), + Encrypted: lo.ToPtr(false), + IOPS: lo.ToPtr(int64(0)), + KMSKeyID: lo.ToPtr("fakeKMSKeyID"), + SnapshotID: lo.ToPtr("fakeSnapshot"), + Throughput: lo.ToPtr(int64(0)), + VolumeSize: resource.NewScaledQuantity(2, resource.Giga), + VolumeType: lo.ToPtr("standard"), + }, }, { - DeviceName: aws.String("map-device-2"), + DeviceName: lo.ToPtr("map-device-2"), }, }, - UserData: aws.String("userdata-test-1"), - DetailedMonitoring: aws.Bool(false), + UserData: aws.String("userdata-test-1"), }, }) }) DescribeTable( - "should match static hash", - func(hash string, changes ...v1beta1.EC2NodeClass) { - modifiedNodeClass := test.EC2NodeClass(append([]v1beta1.EC2NodeClass{*nodeClass}, changes...)...) - Expect(modifiedNodeClass.Hash()).To(Equal(hash)) + "should match static hash on field value change", + func(hash string, changes v1beta1.EC2NodeClass) { + Expect(mergo.Merge(nodeClass, changes, mergo.WithOverride, mergo.WithSliceDeepCopy)).To(Succeed()) + Expect(nodeClass.Hash()).To(Equal(hash)) }, - Entry("Base EC2NodeClass", staticHash), + Entry("Base EC2NodeClass", staticHash, v1beta1.EC2NodeClass{}), // Static fields, expect changed hash from base - Entry("UserData Drift", "588756456110800812", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{UserData: aws.String("userdata-test-2")}}), - Entry("Tags Drift", "2471764681523766508", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Tags: map[string]string{"keyTag-test-3": "valueTag-test-3"}}}), - Entry("MetadataOptions Drift", "11030161632375731908", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPEndpoint: aws.String("test-metadata-2")}}}), - Entry("BlockDeviceMappings Drift", "436753305915039702", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{DeviceName: aws.String("map-device-test-3")}}}}), - Entry("Context Drift", "3729470655588343019", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Context: aws.String("context-2")}}), - Entry("DetailedMonitoring Drift", "17892305444040067573", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{DetailedMonitoring: aws.Bool(true)}}), - Entry("AMIFamily Drift", "9493798894326942407", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AMIFamily: aws.String(v1beta1.AMIFamilyBottlerocket)}}), - Entry("Reorder Tags", staticHash, v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Tags: map[string]string{"keyTag-2": "valueTag-2", "keyTag-1": "valueTag-1"}}}), - Entry("Reorder BlockDeviceMapping", staticHash, v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{DeviceName: aws.String("map-device-2")}, {DeviceName: aws.String("map-device-1")}}}}), + Entry("UserData", "18317182711135792962", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{UserData: aws.String("userdata-test-2")}}), + Entry("Tags", "7254882043893135054", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Tags: map[string]string{"keyTag-test-3": "valueTag-test-3"}}}), + Entry("Context", "17271601354348855032", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Context: aws.String("context-2")}}), + Entry("DetailedMonitoring", "3320998103335094348", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{DetailedMonitoring: aws.Bool(true)}}), + Entry("AMIFamily", "11029247967399146065", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AMIFamily: aws.String(v1beta1.AMIFamilyBottlerocket)}}), + Entry("InstanceStorePolicy", "15591048753403695860", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{InstanceStorePolicy: lo.ToPtr(v1beta1.InstanceStorePolicyRAID0)}}), + Entry("AssociatePublicIPAddress", "8788624850560996180", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AssociatePublicIPAddress: lo.ToPtr(true)}}), + Entry("MetadataOptions HTTPEndpoint", "12130088184516131939", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPEndpoint: lo.ToPtr("enabled")}}}), + Entry("MetadataOptions HTTPProtocolIPv6", "9851778617676567202", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPProtocolIPv6: lo.ToPtr("enabled")}}}), + Entry("MetadataOptions HTTPPutResponseHopLimit", "10114972825726256442", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPPutResponseHopLimit: lo.ToPtr(int64(10))}}}), + Entry("MetadataOptions HTTPTokens", "15328515228245883488", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPTokens: lo.ToPtr("required")}}}), + Entry("BlockDeviceMapping DeviceName", "14855383487702710824", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{DeviceName: lo.ToPtr("map-device-test-3")}}}}), + Entry("BlockDeviceMapping RootVolume", "9591488558660758449", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{RootVolume: true}}}}), + Entry("BlockDeviceMapping DeleteOnTermination", "2802222466202766732", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{DeleteOnTermination: lo.ToPtr(true)}}}}}), + Entry("BlockDeviceMapping Encrypted", "16743053872042184219", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{Encrypted: lo.ToPtr(true)}}}}}), + Entry("BlockDeviceMapping IOPS", "17284705682110195253", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{IOPS: lo.ToPtr(int64(10))}}}}}), + Entry("BlockDeviceMapping KMSKeyID", "9151019926310241707", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{KMSKeyID: lo.ToPtr("test")}}}}}), + Entry("BlockDeviceMapping SnapshotID", "5250341140179985875", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{SnapshotID: lo.ToPtr("test")}}}}}), + Entry("BlockDeviceMapping Throughput", "16711481758638864953", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{Throughput: lo.ToPtr(int64(10))}}}}}), + Entry("BlockDeviceMapping VolumeType", "488614640133725370", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{VolumeType: lo.ToPtr("io1")}}}}}), // Behavior / Dynamic fields, expect same hash as base Entry("Modified AMISelector", staticHash, v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AMISelectorTerms: []v1beta1.AMISelectorTerm{{Tags: map[string]string{"ami-test-key": "ami-test-value"}}}}}), Entry("Modified SubnetSelector", staticHash, v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{SubnetSelectorTerms: []v1beta1.SubnetSelectorTerm{{Tags: map[string]string{"subnet-test-key": "subnet-test-value"}}}}}), Entry("Modified SecurityGroupSelector", staticHash, v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{SecurityGroupSelectorTerms: []v1beta1.SecurityGroupSelectorTerm{{Tags: map[string]string{"security-group-test-key": "security-group-test-value"}}}}}), ) + // We create a separate test for updating blockDeviceMapping volumeSize, since resource.Quantity is a struct, and mergo.WithSliceDeepCopy + // doesn't work well with unexported fields, like the ones that are present in resource.Quantity + It("should match static hash when updating blockDeviceMapping volumeSize", func() { + nodeClass.Spec.BlockDeviceMappings[0].EBS.VolumeSize = resource.NewScaledQuantity(10, resource.Giga) + Expect(nodeClass.Hash()).To(Equal("4802236799448001710")) + }) It("should match static hash for instanceProfile", func() { nodeClass.Spec.Role = "" nodeClass.Spec.InstanceProfile = lo.ToPtr("test-instance-profile") - Expect(nodeClass.Hash()).To(Equal("15756064858220068103")) + Expect(nodeClass.Hash()).To(Equal("7914642030762404205")) + }) + It("should match static hash when reordering tags", func() { + nodeClass.Spec.Tags = map[string]string{"keyTag-2": "valueTag-2", "keyTag-1": "valueTag-1"} + Expect(nodeClass.Hash()).To(Equal(staticHash)) + }) + It("should match static hash when reordering blockDeviceMappings", func() { + nodeClass.Spec.BlockDeviceMappings[0], nodeClass.Spec.BlockDeviceMappings[1] = nodeClass.Spec.BlockDeviceMappings[1], nodeClass.Spec.BlockDeviceMappings[0] + Expect(nodeClass.Hash()).To(Equal(staticHash)) }) DescribeTable("should change hash when static fields are updated", func(changes v1beta1.EC2NodeClass) { hash := nodeClass.Hash() - Expect(mergo.Merge(nodeClass, changes, mergo.WithOverride)).To(Succeed()) + Expect(mergo.Merge(nodeClass, changes, mergo.WithOverride, mergo.WithSliceDeepCopy)).To(Succeed()) updatedHash := nodeClass.Hash() Expect(hash).ToNot(Equal(updatedHash)) }, - Entry("UserData Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{UserData: aws.String("userdata-test-2")}}), - Entry("Tags Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Tags: map[string]string{"keyTag-test-3": "valueTag-test-3"}}}), - Entry("MetadataOptions Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPEndpoint: aws.String("test-metadata-2")}}}), - Entry("BlockDeviceMappings Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{DeviceName: aws.String("map-device-test-3")}}}}), - Entry("Context Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Context: aws.String("context-2")}}), - Entry("DetailedMonitoring Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{DetailedMonitoring: aws.Bool(true)}}), - Entry("AMIFamily Drift", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AMIFamily: aws.String(v1beta1.AMIFamilyBottlerocket)}}), + Entry("UserData", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{UserData: aws.String("userdata-test-2")}}), + Entry("Tags", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Tags: map[string]string{"keyTag-test-3": "valueTag-test-3"}}}), + Entry("Context", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Context: aws.String("context-2")}}), + Entry("DetailedMonitoring", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{DetailedMonitoring: aws.Bool(true)}}), + Entry("AMIFamily", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AMIFamily: aws.String(v1beta1.AMIFamilyBottlerocket)}}), + Entry("InstanceStorePolicy", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{InstanceStorePolicy: lo.ToPtr(v1beta1.InstanceStorePolicyRAID0)}}), + Entry("AssociatePublicIPAddress", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{AssociatePublicIPAddress: lo.ToPtr(true)}}), + Entry("MetadataOptions HTTPEndpoint", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPEndpoint: lo.ToPtr("enabled")}}}), + Entry("MetadataOptions HTTPProtocolIPv6", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPProtocolIPv6: lo.ToPtr("enabled")}}}), + Entry("MetadataOptions HTTPPutResponseHopLimit", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPPutResponseHopLimit: lo.ToPtr(int64(10))}}}), + Entry("MetadataOptions HTTPTokens", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{MetadataOptions: &v1beta1.MetadataOptions{HTTPTokens: lo.ToPtr("required")}}}), + Entry("BlockDeviceMapping DeviceName", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{DeviceName: lo.ToPtr("map-device-test-3")}}}}), + Entry("BlockDeviceMapping RootVolume", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{RootVolume: true}}}}), + Entry("BlockDeviceMapping DeleteOnTermination", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{DeleteOnTermination: lo.ToPtr(true)}}}}}), + Entry("BlockDeviceMapping Encrypted", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{Encrypted: lo.ToPtr(true)}}}}}), + Entry("BlockDeviceMapping IOPS", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{IOPS: lo.ToPtr(int64(10))}}}}}), + Entry("BlockDeviceMapping KMSKeyID", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{KMSKeyID: lo.ToPtr("test")}}}}}), + Entry("BlockDeviceMapping SnapshotID", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{SnapshotID: lo.ToPtr("test")}}}}}), + Entry("BlockDeviceMapping Throughput", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{Throughput: lo.ToPtr(int64(10))}}}}}), + Entry("BlockDeviceMapping VolumeType", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{VolumeType: lo.ToPtr("io1")}}}}}), ) + // We create a separate test for updating blockDeviceMapping volumeSize, since resource.Quantity is a struct, and mergo.WithSliceDeepCopy + // doesn't work well with unexported fields, like the ones that are present in resource.Quantity + It("should change hash blockDeviceMapping volumeSize is updated", func() { + hash := nodeClass.Hash() + nodeClass.Spec.BlockDeviceMappings[0].EBS.VolumeSize = resource.NewScaledQuantity(10, resource.Giga) + updatedHash := nodeClass.Hash() + Expect(hash).ToNot(Equal(updatedHash)) + }) It("should change hash when instanceProfile is updated", func() { nodeClass.Spec.Role = "" nodeClass.Spec.InstanceProfile = lo.ToPtr("test-instance-profile") @@ -105,15 +167,18 @@ var _ = Describe("Hash", func() { updatedHash := nodeClass.Hash() Expect(hash).ToNot(Equal(updatedHash)) }) - DescribeTable("should not change hash when slices are re-ordered", func(changes v1beta1.EC2NodeClass) { + It("should not change hash when tags are re-ordered", func() { hash := nodeClass.Hash() - Expect(mergo.Merge(nodeClass, changes, mergo.WithOverride)).To(Succeed()) + nodeClass.Spec.Tags = map[string]string{"keyTag-2": "valueTag-2", "keyTag-1": "valueTag-1"} updatedHash := nodeClass.Hash() Expect(hash).To(Equal(updatedHash)) - }, - Entry("Reorder Tags", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{Tags: map[string]string{"keyTag-2": "valueTag-2", "keyTag-1": "valueTag-1"}}}), - Entry("Reorder BlockDeviceMapping", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{DeviceName: aws.String("map-device-2")}, {DeviceName: aws.String("map-device-1")}}}}), - ) + }) + It("should not change hash when blockDeviceMappings are re-ordered", func() { + hash := nodeClass.Hash() + nodeClass.Spec.BlockDeviceMappings[0], nodeClass.Spec.BlockDeviceMappings[1] = nodeClass.Spec.BlockDeviceMappings[1], nodeClass.Spec.BlockDeviceMappings[0] + updatedHash := nodeClass.Hash() + Expect(hash).To(Equal(updatedHash)) + }) It("should not change hash when behavior/dynamic fields are updated", func() { hash := nodeClass.Hash() diff --git a/pkg/cloudprovider/suite_test.go b/pkg/cloudprovider/suite_test.go index ccb4e25b1828..4e980174a4ba 100644 --- a/pkg/cloudprovider/suite_test.go +++ b/pkg/cloudprovider/suite_test.go @@ -771,7 +771,7 @@ var _ = Describe("CloudProvider", func() { Expect(err).NotTo(HaveOccurred()) Expect(isDrifted).To(BeEmpty()) - Expect(mergo.Merge(nodeClass, changes, mergo.WithOverride, mergo.WithSliceDeepCopy)) + Expect(mergo.Merge(nodeClass, changes, mergo.WithOverride, mergo.WithSliceDeepCopy)).To(Succeed()) nodeClass.Annotations = lo.Assign(nodeClass.Annotations, map[string]string{v1beta1.AnnotationEC2NodeClassHash: nodeClass.Hash()}) ExpectApplied(ctx, env.Client, nodeClass) @@ -798,12 +798,19 @@ var _ = Describe("CloudProvider", func() { Entry("BlockDeviceMapping KMSKeyID", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{KMSKeyID: lo.ToPtr("test")}}}}}), Entry("BlockDeviceMapping SnapshotID", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{SnapshotID: lo.ToPtr("test")}}}}}), Entry("BlockDeviceMapping Throughput", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{Throughput: lo.ToPtr(int64(10))}}}}}), - // Karpenter currently have a bug with volumeSize will cause NodeClaims to not be drifted, when the field is updated - // TODO: Enable volumeSize once they can be used for static drift - // For more information: https://github.com/aws/karpenter-provider-aws/issues/5447 - // Entry("BlockDeviceMapping VolumeSize", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{VolumeSize: resource.NewScaledQuantity(10, resource.Giga)}}}}}), Entry("BlockDeviceMapping VolumeType", v1beta1.EC2NodeClass{Spec: v1beta1.EC2NodeClassSpec{BlockDeviceMappings: []*v1beta1.BlockDeviceMapping{{EBS: &v1beta1.BlockDevice{VolumeType: lo.ToPtr("io1")}}}}}), ) + // We create a separate test for updating blockDeviceMapping volumeSize, since resource.Quantity is a struct, and mergo.WithSliceDeepCopy + // doesn't work well with unexported fields, like the ones that are present in resource.Quantity + It("should return drifted when updating blockDeviceMapping volumeSize", func() { + nodeClass.Spec.BlockDeviceMappings[0].EBS.VolumeSize = resource.NewScaledQuantity(10, resource.Giga) + nodeClass.Annotations = lo.Assign(nodeClass.Annotations, map[string]string{v1beta1.AnnotationEC2NodeClassHash: nodeClass.Hash()}) + + ExpectApplied(ctx, env.Client, nodeClass) + isDrifted, err := cloudProvider.IsDrifted(ctx, nodeClaim) + Expect(err).NotTo(HaveOccurred()) + Expect(isDrifted).To(Equal(cloudprovider.NodeClassDrift)) + }) DescribeTable("should not return drifted if dynamic fields are updated", func(changes v1beta1.EC2NodeClass) { ExpectApplied(ctx, env.Client, nodePool, nodeClass) diff --git a/pkg/providers/instancetype/instancetype.go b/pkg/providers/instancetype/instancetype.go index 92daf188bc42..049d7be873ad 100644 --- a/pkg/providers/instancetype/instancetype.go +++ b/pkg/providers/instancetype/instancetype.go @@ -48,7 +48,6 @@ import ( const ( InstanceTypesCacheKey = "types" InstanceTypeOfferingsCacheKey = "offerings" - ZonesCacheKey = "zones" ) type Provider struct { @@ -117,12 +116,7 @@ func (p *Provider) List(ctx context.Context, kc *corev1beta1.KubeletConfiguratio systemReservedHash, _ = hashstructure.Hash(resources.StringMap(kc.SystemReserved), hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true}) } blockDeviceMappingsHash, _ := hashstructure.Hash(nodeClass.Spec.BlockDeviceMappings, hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true}) - // TODO: remove volumeSizeHash once resource.Quantity objects get hashed as a string in BlockDeviceMappings - // For more information on the resource.Quantity hash issue: https://github.com/aws/karpenter-provider-aws/issues/5447 - volumeSizeHash, _ := hashstructure.Hash(lo.Reduce(nodeClass.Spec.BlockDeviceMappings, func(agg string, block *v1beta1.BlockDeviceMapping, _ int) string { - return fmt.Sprintf("%s/%s", agg, block.EBS.VolumeSize) - }, ""), hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true}) - key := fmt.Sprintf("%d-%d-%d-%016x-%016x-%016x-%s-%s-%016x-%016x-%016x", + key := fmt.Sprintf("%d-%d-%d-%016x-%016x-%016x-%s-%s-%016x-%016x", p.instanceTypesSeqNum, p.instanceTypeOfferingsSeqNum, p.unavailableOfferings.SeqNum, @@ -131,7 +125,6 @@ func (p *Provider) List(ctx context.Context, kc *corev1beta1.KubeletConfiguratio blockDeviceMappingsHash, aws.StringValue((*string)(nodeClass.Spec.InstanceStorePolicy)), aws.StringValue(nodeClass.Spec.AMIFamily), - volumeSizeHash, kubeReservedHash, systemReservedHash, ) diff --git a/pkg/providers/launchtemplate/launchtemplate.go b/pkg/providers/launchtemplate/launchtemplate.go index eee9f1cf3e2a..2ddcf81d61cd 100644 --- a/pkg/providers/launchtemplate/launchtemplate.go +++ b/pkg/providers/launchtemplate/launchtemplate.go @@ -147,17 +147,12 @@ func (p *Provider) Invalidate(ctx context.Context, ltName string, ltID string) { } func launchTemplateName(options *amifamily.LaunchTemplate) string { - // TODO: jmdeal@ remove custom hash struct once BlockDeviceMapping and KubeletConfiguration hashing is fixed, only hash Options - volumeSizeHash, _ := hashstructure.Hash(lo.Reduce(options.BlockDeviceMappings, func(agg string, block *v1beta1.BlockDeviceMapping, _ int) string { - return fmt.Sprintf("%s/%s", agg, block.EBS.VolumeSize) - }, ""), hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true}) + // TODO: jmdeal@ remove custom hash struct once KubeletConfiguration hashing is fixed, only hash Options hashStruct := struct { Options *amifamily.LaunchTemplate - VolumeSizeHash string ReservedResourcesHash string }{ Options: options, - VolumeSizeHash: fmt.Sprint(volumeSizeHash), ReservedResourcesHash: options.UserData.HashReservedResources(), } hash, err := hashstructure.Hash(hashStruct, hashstructure.FormatV2, &hashstructure.HashOptions{SlicesAsSets: true}) diff --git a/test/suites/drift/suite_test.go b/test/suites/drift/suite_test.go index 46c77c543320..2011d29fcf9e 100644 --- a/test/suites/drift/suite_test.go +++ b/test/suites/drift/suite_test.go @@ -32,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/karpenter/pkg/utils/resources" awssdk "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -680,7 +681,7 @@ var _ = Describe("Drift", func() { { DeviceName: awssdk.String("/dev/xvda"), EBS: &v1beta1.BlockDevice{ - VolumeSize: resource.NewScaledQuantity(20, resource.Giga), + VolumeSize: resources.Quantity("20Gi"), VolumeType: awssdk.String("gp3"), Encrypted: awssdk.Bool(true), }, @@ -719,6 +720,37 @@ var _ = Describe("Drift", func() { env.EventuallyExpectNotFound(pod, node) env.EventuallyExpectHealthyPodCount(selector, numPods) }) + It("should drift the EC2NodeClass on BlockDeviceMappings volume size update", func() { + nodeClass.Spec.BlockDeviceMappings = []*v1beta1.BlockDeviceMapping{ + { + DeviceName: awssdk.String("/dev/xvda"), + EBS: &v1beta1.BlockDevice{ + VolumeSize: resources.Quantity("20Gi"), + VolumeType: awssdk.String("gp3"), + Encrypted: awssdk.Bool(true), + }, + }, + } + env.ExpectCreated(dep, nodeClass, nodePool) + pod := env.EventuallyExpectHealthyPodCount(selector, numPods)[0] + nodeClaim := env.EventuallyExpectCreatedNodeClaimCount("==", 1)[0] + node := env.ExpectCreatedNodeCount("==", 1)[0] + + nodeClass.Spec.BlockDeviceMappings[0].EBS.VolumeSize = resources.Quantity("100Gi") + env.ExpectCreatedOrUpdated() + + By("validating the drifted status condition has propagated") + Eventually(func(g Gomega) { + g.Expect(env.Client.Get(env, client.ObjectKeyFromObject(nodeClaim), nodeClaim)).To(Succeed()) + g.Expect(nodeClaim.StatusConditions().GetCondition(corev1beta1.Drifted)).ToNot(BeNil()) + g.Expect(nodeClaim.StatusConditions().GetCondition(corev1beta1.Drifted).IsTrue()).To(BeTrue()) + }).Should(Succeed()) + + delete(pod.Annotations, corev1beta1.DoNotDisruptAnnotationKey) + env.ExpectUpdated(pod) + env.EventuallyExpectNotFound(pod, node) + env.EventuallyExpectHealthyPodCount(selector, numPods) + }) It("should update the nodepool-hash annotation on the nodepool and nodeclaim when the nodepool's nodepool-hash-version annotation does not match the controller hash version", func() { env.ExpectCreated(dep, nodeClass, nodePool) env.EventuallyExpectHealthyPodCount(selector, numPods)