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.

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

fix: Hash volume size quantity value as a string #5454

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

jonathan-innis
Copy link
Contributor

Fixes #5447

Description

This change applies the change that was added in #3330 and ensures that we won't regress on this issue again by adding the relevant testing into functional and E2E testing. Hashing the resource quantity value as a string was missed with the changes were ported over and duplicated from the AWS provider struct in v1alpha1.

How was this change tested?

make presubmit
/karpenter snapshot

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jonathan-innis jonathan-innis requested a review from a team as a code owner January 11, 2024 07:46
Copy link

netlify bot commented Jan 11, 2024

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 0b0569c
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/660476335f32d800083ae2ab

@coveralls
Copy link

coveralls commented Jan 11, 2024

Pull Request Test Coverage Report for Build 8457681220

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 82.468%

Totals Coverage Status
Change from base Build 8456676269: -0.01%
Covered Lines: 5353
Relevant Lines: 6491

💛 - Coveralls

@jonathan-innis jonathan-innis added the blocked Unable to make progress due to some dependency label Jan 11, 2024
@jonathan-innis jonathan-innis changed the title fix: Fix hash volume size quantity value fix: Hash volume size quantity value as a string Jan 11, 2024
@jonathan-innis jonathan-innis removed the blocked Unable to make progress due to some dependency label Jan 11, 2024
@jonathan-innis jonathan-innis added the blocked Unable to make progress due to some dependency label Jan 11, 2024
@jonathan-innis
Copy link
Contributor Author

This change is blocked until we have the ability to version the hashes that we are using to evaluate drift since this would be a breaking change to the existing hash versioning mechanism since a previously unconsidered field would now get hashed with a value.

Copy link
Contributor Author

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

Copy link
Contributor

Snapshot successfully published to oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:v0-84f3589222580a56e359f098387ba5c5df442bfb.
To install you must login to the ECR repo with an AWS account:

aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 021119463062.dkr.ecr.us-east-1.amazonaws.com

Copy link
Contributor Author

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

@jonathan-innis jonathan-innis force-pushed the fix-volume-size-hash branch 2 times, most recently from 75b797a to 0a3f092 Compare March 27, 2024 15:23
Copy link
Contributor

Snapshot successfully published to oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:0-f507ab814467f45b99e2091bbded95edbf22635b.
To install you must login to the ECR repo with an AWS account:

aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 021119463062.dkr.ecr.us-east-1.amazonaws.com

helm upgrade --install karpenter oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter --version "0-f507ab814467f45b99e2091bbded95edbf22635b" --namespace "kube-system" --create-namespace \
  --set "settings.clusterName=${CLUSTER_NAME}" \
  --set "settings.interruptionQueue=${CLUSTER_NAME}" \
  --set controller.resources.requests.cpu=1 \
  --set controller.resources.requests.memory=1Gi \
  --set controller.resources.limits.cpu=1 \
  --set controller.resources.limits.memory=1Gi \
  --wait

@jonathan-innis jonathan-innis removed the blocked Unable to make progress due to some dependency label Mar 27, 2024
Copy link
Contributor Author

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

Copy link
Contributor

Snapshot successfully published to oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter:0-984c99cf65997c66e8733e04da30a2234a1db9d3.
To install you must login to the ECR repo with an AWS account:

aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 021119463062.dkr.ecr.us-east-1.amazonaws.com

helm upgrade --install karpenter oci://021119463062.dkr.ecr.us-east-1.amazonaws.com/karpenter/snapshot/karpenter --version "0-984c99cf65997c66e8733e04da30a2234a1db9d3" --namespace "kube-system" --create-namespace \
  --set "settings.clusterName=${CLUSTER_NAME}" \
  --set "settings.interruptionQueue=${CLUSTER_NAME}" \
  --set controller.resources.requests.cpu=1 \
  --set controller.resources.requests.memory=1Gi \
  --set controller.resources.limits.cpu=1 \
  --set controller.resources.limits.memory=1Gi \
  --wait

@engedaam
Copy link
Contributor

Can we also unblock this test as part of this PR?

// Karpenter currently have a bug with volumeSize will cause NodeClaims to not be drifted, when the field is updated

@jonathan-innis jonathan-innis force-pushed the fix-volume-size-hash branch 4 times, most recently from 5199f6e to c6322c7 Compare March 27, 2024 17:59
@jonathan-innis jonathan-innis enabled auto-merge (squash) March 27, 2024 18:01
Copy link
Contributor

@engedaam engedaam left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jonathan-innis jonathan-innis merged commit 9410598 into aws:main Mar 27, 2024
16 checks passed
engedaam pushed a commit to engedaam/karpenter that referenced this pull request Mar 27, 2024
engedaam pushed a commit to engedaam/karpenter that referenced this pull request Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A blockDeviceMapping change in EC2NodeClass does not trigger drift replace
3 participants