created nvidia gpu operator addon #824

Howlla · 2023-08-17T06:59:18Z

Issue #, if available:

Description of changes:
Creates new addon for gpu-operator which extends helm addon. Still working on md file for documentation.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

elamaran11

@Howlla Thankyou for taking time to get this addon submitted. Couple of things are missing, please reachout for any questions:

Addon Documentation is missing. Check docs/addons/grafana-operator.md as an example.
Addon is missing in indexing list of documentation.
Addon should be added to e2e in examples/blueprint-construct/index.ts
mkdocs.yaml should be updated with new addon.
Also see if you have opportunity to add unit tests under tests

overall great work. Are you planning to do gpu-builder in separate PR?

lib/addons/gpu-operator/index.ts

Howlla · 2023-08-18T07:01:58Z

Created builder and addon with documentation. Also added it to the e2e test

@elamaran11 i was not sure whether to add the gpu operator addon in the builder itself or will we do it through the addon. It is a small change in line 124 of gpu-builder.ts

elamaran11

@Howlla Looks good to me, great work on the Builder class on Addon. Following needs to be done :

Builder class doc is missing
Also builder class should be added to mkdocs.
Plus the builder class is not aligned to functionality we discussed yesterday. So please make sure you add GPU Addon, have a method like enableGPU and parametrize GPU like AmpAddon.

elamaran11 · 2023-08-18T11:31:49Z

lib/builders/gpu-builder.ts

+                    ]
+                })
+            )
+            .addOns(


I think you should have GPU Addon as part of the builder. You should also be adding other required addons for the cluster which you used in your sample runs like coredns, kube-proxy etc etc to minimally run the solution. You can follow observability builder as an example to have a method like enableGPU and have all addons. Also as i mentioned before, you should think about grabbing all values as parameters from pattern you are building to builder class to pass to GPU Addon like AmpAddon.

mkdocs.yml

elamaran11

@Howlla We are on track. Just couple of comments.

elamaran11 · 2023-08-18T18:24:05Z

docs/builders/gpu-builder.md

+        const region = process.env.CDK_DEFAULT_REGION!;
+        const stackID = `${id}-eks-blueprint`;
+
+        const nodeRole = new blueprints.CreateRoleProvider("blueprint-node-role", new iam.ServicePrincipal("ec2.amazonaws.com"),


This is not required. We needed that for windows for different reason

Fixed, should i remove the nodeRole arguments from gpu-builder.ts?

nodeRole itself is not required here because its taken care by blueprints. This was specific to make windows work. I think you should follow the Observability Builder convention.

lib/builders/gpu-builder.ts

elamaran11

@shapirov103 This PR looks near to complete with couple of comments open. Can you take a look in the PR, provide feedback so we can merge this with Release?

shapirov103

@Howlla great work, I added a few comments, there is a bit of work left, happy to discuss.
Please also make sure you run standard commands for PR submission such as make lint. This process is described here: https://github.com/aws-quickstart/cdk-eks-blueprints/blob/main/docs/internal/readme-internal.md#submitting-pull-requests

.github/workflows/linkcheck.json

lib/builders/gpu-builder.ts

shapirov103 · 2023-08-21T17:31:26Z

lib/builders/gpu-builder.ts

+     * This method helps you prepare a blueprint for setting up observability 
+     * returning an array of blueprint addons for AWS managed open source services
+     */
+    public enableGpu(values?: blueprints.Values): GpuBuilder {


what are these values? it is going confusing to the customers which values are allowed to be passed. the way it stands, it is just key value pairs and no indication of the schema. Did you mean to allow passing some particular type here?

Added valueSchema similar to container insights values

shapirov103 · 2023-08-21T17:31:55Z

lib/builders/gpu-builder.ts

+     * This method helps you prepare a blueprint for setting up windows nodes with 
+     * usage tracking addon
+     */
+     public static builder(): GpuBuilder {


Any constructor args to allow customers to pass here?

Passing gpuOptions now

docs/addons/gpu-operator.md

Howlla · 2023-08-22T19:04:05Z

Fixed the issues @shapirov103

elamaran11

@Howlla The gpu-builder.ts should be reworked based on my current PR for observability, Also you should add the builder to index.ts in builder folder in lib. Please let us know once done, we can review.

lib/builders/gpu-builder.ts

Howlla · 2023-08-22T22:49:20Z

made changes @elamaran11

elamaran11

@Howlla Did you merge from the branch in my PR. I think i have most of these changes for builders plus few other changes in my PR needs to be merged, before we can move in. @shapirov103 Please check

Howlla · 2023-08-23T03:14:57Z

I havent merged from your yours but i made sure no conflicts occur. Let me know if you want me to create a branch with both ghaction and gpu builder code

elamaran11 · 2023-08-23T10:59:05Z

I havent merged from your yours but i made sure no conflicts occur. Let me know if you want me to create a branch with both ghaction and gpu builder code

You can do couple of things. One merge from my branch or merge from main after my PR is merged.

Howlla · 2023-08-23T19:48:21Z

Merged with main branch and resolved conflicts

elamaran11

LGTM. @shapirov103 Please check and merge this.,

elamaran11

Some more minor feedback.

lib/builders/gpu-builder.ts

elamaran11

LGTM. @shapirov103 Please review this, we can move to merge.

shapirov103

@Howlla Great work!
I added a question that should not affect the PR status, i will merge after e2e.

shapirov103 · 2023-08-25T18:51:35Z

examples/blueprint-construct/index.ts

+                      WITH_REBOOT: true
+                    },
+                    toolkit: {
+                      version: 'v1.13.1-centos7'


is this a mandatory parameter? I am curious how customers (and us) will update this moving forward.

This toolkit version was verified as working with AL2 by Re Alvarez

shapirov103 · 2023-08-25T18:55:05Z

/do-e2e-tests

shapirov103 · 2023-08-25T19:54:21Z

/do-e2e-tests

aws-ia-ci

end to end tests passed

added nvidia gpu operator addon helm chart

028127d

Howlla requested review from aws-ia-ci, parkand1, shapirov103, youngjeong46, elamaran11 and a team as code owners August 17, 2023 06:59

elamaran11 requested changes Aug 17, 2023

View reviewed changes

lib/addons/gpu-operator/index.ts Outdated Show resolved Hide resolved

lib/addons/gpu-operator/index.ts Outdated Show resolved Hide resolved

Howlla added 6 commits August 17, 2023 11:58

added documentation for nvidia gpu operator and update to latest version

882d1c0

add to doc to index integrate e2e test

ca2e731

add to doc to index integrate e2e test

f19a3f8

ignore link check for paralus addon

bac7545

added GPU builder

51d3444

Add jsdoc

8d0bdbb

elamaran11 requested changes Aug 18, 2023

View reviewed changes

Howlla added 2 commits August 18, 2023 11:20

added gpu builder, docs and enablegpu method

be6056d

lint fix

d041663

elamaran11 requested changes Aug 18, 2023

View reviewed changes

remove vpc and passing parameters to builder

54c627e

elamaran11 reviewed Aug 18, 2023

View reviewed changes

remove noderole boilerplate

2cf20bc

shapirov103 reviewed Aug 21, 2023

View reviewed changes

Howlla added 5 commits August 21, 2023 13:13

Input gpuOptions from builder parameters

887a4da

add unknown params to gpu operator values

3c08536

add usage id qs-1ug68anj6

22162c5

Merge branch 'aws-quickstart:main' into main

a147b29

fix lintcheck

29bcbae

elamaran11 requested changes Aug 22, 2023

View reviewed changes

lib/builders/gpu-builder.ts Outdated Show resolved Hide resolved

lib/builders/gpu-builder.ts Outdated Show resolved Hide resolved

Refactor code for builders folder and reduce imports

c8f2fcc

elamaran11 previously approved these changes Aug 23, 2023

View reviewed changes

resolve conflict

683a2dc

Howlla dismissed elamaran11’s stale review via 683a2dc August 23, 2023 19:47

elamaran11 approved these changes Aug 23, 2023

View reviewed changes

elamaran11 requested changes Aug 24, 2023

View reviewed changes

lib/builders/gpu-builder.ts Outdated Show resolved Hide resolved

fix import statements

4559e1b

elamaran11 previously approved these changes Aug 25, 2023

View reviewed changes

Merge branch 'aws-quickstart:main' into main

f3bedcb

shapirov103 previously approved these changes Aug 25, 2023

View reviewed changes

Modify e2e test add gpu nodegroup

8ace408

Howlla dismissed stale reviews from shapirov103 and elamaran11 via 8ace408 August 25, 2023 19:52

aws-ia-ci approved these changes Aug 25, 2023

View reviewed changes

shapirov103 merged commit 65f76b4 into aws-quickstart:main Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

created nvidia gpu operator addon #824

created nvidia gpu operator addon #824

Howlla commented Aug 17, 2023

elamaran11 left a comment •

edited

Loading

Howlla commented Aug 18, 2023

elamaran11 left a comment

elamaran11 Aug 18, 2023

elamaran11 left a comment

elamaran11 Aug 18, 2023

Howlla Aug 18, 2023

elamaran11 Aug 18, 2023

elamaran11 left a comment

shapirov103 left a comment

shapirov103 Aug 21, 2023

Howlla Aug 21, 2023

shapirov103 Aug 21, 2023

Howlla Aug 21, 2023

Howlla commented Aug 22, 2023

elamaran11 left a comment

Howlla commented Aug 22, 2023

elamaran11 left a comment

Howlla commented Aug 23, 2023

elamaran11 commented Aug 23, 2023

Howlla commented Aug 23, 2023

elamaran11 left a comment

elamaran11 left a comment

elamaran11 left a comment

shapirov103 left a comment

shapirov103 Aug 25, 2023

Howlla Aug 25, 2023

shapirov103 commented Aug 25, 2023

shapirov103 commented Aug 25, 2023

aws-ia-ci left a comment

created nvidia gpu operator addon #824

created nvidia gpu operator addon #824

Conversation

Howlla commented Aug 17, 2023

elamaran11 left a comment • edited Loading

Choose a reason for hiding this comment

Howlla commented Aug 18, 2023

elamaran11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Howlla commented Aug 22, 2023

elamaran11 left a comment

Choose a reason for hiding this comment

Howlla commented Aug 22, 2023

elamaran11 left a comment

Choose a reason for hiding this comment

Howlla commented Aug 23, 2023

elamaran11 commented Aug 23, 2023

Howlla commented Aug 23, 2023

elamaran11 left a comment

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shapirov103 commented Aug 25, 2023

shapirov103 commented Aug 25, 2023

aws-ia-ci left a comment

Choose a reason for hiding this comment

elamaran11 left a comment •

edited

Loading