Managed Node Group launch template disk size fix #1229

JustASquid · 2024-07-03T23:34:17Z

Proposed changes

Correctly handle disk size changes when a launch template is created by ManagedNodeGroup

Related issues (optional)

Resolves #1228

… support setting disk size when a MNG uses a custom launch template

github-actions · 2024-07-03T23:34:28Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

flostadler

Thanks a lot for the PR!
Could you please add a test for this? Let me know if you need help with that; https://github.com/pulumi/pulumi-eks/blob/master/examples/examples_nodejs_test.go#L415 is a good example of how to create an additional test.

nodejs/eks/nodegroup.ts

github-actions · 2024-07-22T23:11:41Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

JustASquid · 2024-07-22T23:12:17Z

Hey @flostadler, I had a crack at making a test (I haven't run it yet), does this look like what you'd be looking for?

flostadler

Thanks a lot for your contribution! Mostly some small nits.
Once you've added the new tests to the test matrix I'll start an integration test run :)

examples/tests/managed-ng-disk-size/index.ts

flostadler · 2024-08-08T07:56:10Z

examples/examples_nodejs_test.go

+		With(integration.ProgramTestOptions{
+			Dir: path.Join(getCwd(t), "tests", "managed-ng-disk-size"),
+			ExtraRuntimeValidation: func(t *testing.T, info integration.RuntimeValidationStackInfo) {
+				utils.RunEKSSmokeTest(t,


Was the issue before that the nodes where booted with the wrong disk size or that the creation of the managed node group failed?

If it's the former we'd need to add additional verification to ensure the disk size is correct.

They booted with the wrong disk size, so yeah, would be good to check. Is there another test I can look at that does similar verification, so I can use that as a basis?

We could create a similar func like this one here that's checking if the node is ready:

pulumi-eks/examples/utils/utils.go

Lines 371 to 387 in 4e11388

func IsNodeReady(t *testing.T, clientset *kubernetes.Clientset, node *corev1.Node) (bool, *corev1.Node) {

// Attempt to retrieve Node.

o, err := clientset.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})

if err != nil {

return false, nil

}

// Check the returned Node's conditions for readiness.

for _, condition := range o.Status.Conditions {

if condition.Type == corev1.NodeReady {

t.Logf("Checking if Node %q is Ready | Condition.Status: %q | Condition: %v\n", node.Name, condition.Status, condition)

return condition.Status == corev1.ConditionTrue, o

}

}

return false, o

}

It could do something like:

var desiredSizeGB int64 = 50 nodeEphemeralStorage := o.Status.Capacity[corev1.ResourceEphemeralStorage] return nodeEphemeralStorage.CmpInt64(desiredSizeGB * 1_000_000_000) >= 0

Alternatively we could check the EC2 instances directly using AWS APIs, but I feel like the kube only approach is cleaner.

Just pushed a change to use the utils.ValidateNodes function. It's my first time using Go and I don't have a way to actually run the code so I'm sure it's missing something but hopefully it's a start?

Thanks! I'll kick off the tests :)

examples/examples_nodejs_test.go

flostadler · 2024-08-08T08:21:50Z

nodejs/eks/nodegroup.ts

@@ -1836,9 +1839,19 @@ Content-Type: text/x-shellscript; charset="us-ascii"
        ? { httpTokens: "required", httpPutResponseHopLimit: 2, httpEndpoint: "enabled" }
        : undefined;

+    const blockDeviceMappings = [
+        {
+            deviceName: "/dev/xvda",


Mental note for myself: This works only for AL2 and AL2023 based AMIs, which is fine right now because we only support AL2. Will need to be taken into account when working on #1195

github-actions · 2024-08-09T02:18:43Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-08-11T23:25:34Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

flostadler · 2024-08-13T09:08:31Z

/run-acceptance-tests

pulumi-bot · 2024-08-13T09:08:52Z

Please view the PR build - https://github.com/pulumi/pulumi-eks/actions/runs/10366894620

github-actions · 2024-08-13T10:49:46Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

flostadler · 2024-08-13T10:50:37Z

@JustASquid I had to merge in some changes from the master branch because they were required for the build to work. I'll re-run the tests now

flostadler · 2024-08-13T10:50:42Z

/run-acceptance-tests

pulumi-bot · 2024-08-13T10:51:01Z

Please view the PR build - https://github.com/pulumi/pulumi-eks/actions/runs/10368370295

flostadler · 2024-08-13T12:43:39Z

@JustASquid this seems to have broken managed node groups:

      	* creating EKS Node Group (example-managed-nodegroups-eksCluster-b3afc1b:example-managed-ng0-f56bd8b): operation error EKS: CreateNodegroup, https response error StatusCode: 400, RequestID: 2f56b595-7e06-428e-ba3c-cc309d169e6a, InvalidRequestException: The request must contain the parameter ebs

https://github.com/pulumi/pulumi-eks/actions/runs/10368370295/job/28702007770#step:21:519

I can have a look later today to see what exactly is going wrong here

flostadler · 2024-08-19T14:20:25Z

@JustASquid After debugging this, it seems that we need an additional check whether disksize is defined here: https://github.com/pulumi/pulumi-eks/pull/1229/files#diff-fa11b07a99c4eb8e85f987e265e143389230bafb7636b3b6bab10cf6158a120eR1847
If it's not defined we shouldn't pass blockDeviceMappings to the launch template.

Do you want to add this change or should I add it on top of this PR?

…-size-fix' into feature/mng-launch-template-disk-size-fix

JustASquid · 2024-08-19T22:39:00Z

@flostadler I just pushed a fix.

github-actions · 2024-08-19T22:39:02Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

flostadler · 2024-08-20T13:15:41Z

/run-acceptance-tests

pulumi-bot · 2024-08-20T13:16:09Z

Please view the PR build - https://github.com/pulumi/pulumi-eks/actions/runs/10472336069

flostadler · 2024-08-20T17:25:50Z

@JustASquid The existing tests are happy now, but I found that newly added tests aren't being executed for community PRs. I ran the MNG_DiskSize test locally and there were some minor issues in the test setup. I fixed those and created a new PR with the combined changes #1305. But don't worry, you're gonna get all the credit for this fix and excellent tests!

Thanks again for this contribution

@JustASquid

This fixes the handling of disk size changes when a custom launch template is created for the MNG. It is based on the Community PR #1229 by @JustASquid, I just added some test fixes on top. Fixes #1228 --------- Co-authored-by: Daniel West <daniel.west.279@gmail.com>

@JustASquid

This fixes the handling of disk size changes when a custom launch template is created for the MNG. It is based on the Community PR #1229 by @JustASquid, I just added some test fixes on top. Fixes #1228 --------- Co-authored-by: Daniel West <daniel.west.279@gmail.com>

JustASquid added 2 commits July 3, 2024 13:13

Add ability to specify gpu/amiId/amiType to a managed node group, and…

b749708

… support setting disk size when a MNG uses a custom launch template

Removed non-disk size changes

806b41c

JustASquid mentioned this pull request Jul 3, 2024

Improve ManagedNodeGroup when creating a LaunchTemplate #1224

Open

flostadler reviewed Jul 8, 2024

View reviewed changes

nodejs/eks/nodegroup.ts Outdated Show resolved Hide resolved

First pass at test

af8aad1

mikhailshilkov requested a review from flostadler August 5, 2024 20:24

flostadler reviewed Aug 8, 2024

View reviewed changes

PR feedback

1e658a0

Asserrt disk size in test

b5f17ba

Merge branch 'master' into feature/mng-launch-template-disk-size-fix

f44e81f

JustASquid added 2 commits August 20, 2024 08:38

Only pass blockDeviceMappings if disk size is defined

a41e27d

Merge remote-tracking branch 'origin/feature/mng-launch-template-disk…

9482be6

…-size-fix' into feature/mng-launch-template-disk-size-fix

flostadler mentioned this pull request Aug 20, 2024

Managed Node Group launch template disk size fix #1305

Merged

flostadler closed this Aug 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Managed Node Group launch template disk size fix #1229

Managed Node Group launch template disk size fix #1229

JustASquid commented Jul 3, 2024

github-actions bot commented Jul 3, 2024

flostadler left a comment

github-actions bot commented Jul 22, 2024

JustASquid commented Jul 22, 2024

flostadler left a comment

flostadler Aug 8, 2024

JustASquid Aug 9, 2024

flostadler Aug 9, 2024 •

edited

Loading

JustASquid Aug 11, 2024

flostadler Aug 13, 2024

flostadler Aug 8, 2024

github-actions bot commented Aug 9, 2024

github-actions bot commented Aug 11, 2024

flostadler commented Aug 13, 2024

pulumi-bot commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

flostadler commented Aug 13, 2024

flostadler commented Aug 13, 2024

pulumi-bot commented Aug 13, 2024

flostadler commented Aug 13, 2024

flostadler commented Aug 19, 2024

JustASquid commented Aug 19, 2024

github-actions bot commented Aug 19, 2024

flostadler commented Aug 20, 2024

pulumi-bot commented Aug 20, 2024

flostadler commented Aug 20, 2024

	func IsNodeReady(t testing.T, clientset kubernetes.Clientset, node corev1.Node) (bool, corev1.Node) {
	// Attempt to retrieve Node.
	o, err := clientset.CoreV1().Nodes().Get(context.TODO(), node.Name, metav1.GetOptions{})
	if err != nil {
	return false, nil
	}

	// Check the returned Node's conditions for readiness.
	for _, condition := range o.Status.Conditions {
	if condition.Type == corev1.NodeReady {
	t.Logf("Checking if Node %q is Ready \| Condition.Status: %q \| Condition: %v\n", node.Name, condition.Status, condition)
	return condition.Status == corev1.ConditionTrue, o
	}
	}

	return false, o
	}

Managed Node Group launch template disk size fix #1229

Managed Node Group launch template disk size fix #1229

Conversation

JustASquid commented Jul 3, 2024

Proposed changes

Related issues (optional)

github-actions bot commented Jul 3, 2024

flostadler left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2024

JustASquid commented Jul 22, 2024

flostadler left a comment

Choose a reason for hiding this comment

flostadler Aug 8, 2024

Choose a reason for hiding this comment

JustASquid Aug 9, 2024

Choose a reason for hiding this comment

flostadler Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

JustASquid Aug 11, 2024

Choose a reason for hiding this comment

flostadler Aug 13, 2024

Choose a reason for hiding this comment

flostadler Aug 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 9, 2024

github-actions bot commented Aug 11, 2024

flostadler commented Aug 13, 2024

pulumi-bot commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

flostadler commented Aug 13, 2024

flostadler commented Aug 13, 2024

pulumi-bot commented Aug 13, 2024

flostadler commented Aug 13, 2024

flostadler commented Aug 19, 2024

JustASquid commented Aug 19, 2024

github-actions bot commented Aug 19, 2024

flostadler commented Aug 20, 2024

pulumi-bot commented Aug 20, 2024

flostadler commented Aug 20, 2024

flostadler Aug 9, 2024 •

edited

Loading