-
Notifications
You must be signed in to change notification settings - Fork 205
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
KubeRay Operator add-on #849
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freschri Nice work but i have few questions:
- Where is the ask from Partner side to build this addon though this will be an AWSome addition to Blueprints?
- Are you planning to build a complete pattern to show usage of this KubeRay with a acomplete workload. We can also build a pattern with observability for this? If so please create a issue in patterns and observability repo
- Update to doc index and mkdocs is missing too
the idea is to build the cdk equivalent of DoEKS's JARK stack, also presented here: https://aws.amazon.com/blogs/containers/deploy-generative-ai-models-on-amazon-eks/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freschri Doc index is still not updated so this wont list in list of addons page. Also can you make sure you test the addon with blueprint-construction running it whole and let us know if it works. We can then run e2e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @shapirov103 Please check from your end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freschri looks great, a couple of comments.
docs/addons/kuberay-operator.md
Outdated
import { Construct } from 'constructs'; | ||
import * as blueprints from '@aws-quickstart/eks-blueprints'; | ||
|
||
export class DatadogConstruct extends Construct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using DatadogConstruct here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry copy paste error. done.
/** | ||
* User provided options for the Helm Chart | ||
*/ | ||
export interface KubeRayAddOnProps extends HelmAddOnUserProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any common values that customers should configure when deploying this addon, that would make sense to promote to this struct and make explicit for the customers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are no config values, please see here: https://ray-project.github.io/kuberay/deploy/helm/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freschri please see my comment. we are targetting 1.13 release this week, there is a chance this addon can make it in.
lib/addons/kuberay/index.ts
Outdated
name: "kuberay-operator", | ||
chart: "kuberay-operator", | ||
namespace: "default", | ||
version: "1.0.0-rc.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use an RC version for the release? I see the have 1.0.0 - let's use that. We generally avoid using non-GA versions as default for the addon (with rare exceptions).
@freschri please igore the markdown broken links in the above check. We are addressing in a separate PR. |
@freschri Are you planning to close this PR comments? |
@freschri let's address minor feedback and please push something to retrigger the GH actions. I suppose the MD check that is failing should succeed now. |
@shapirov103 done |
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests failed. A maintainer can provide more details.
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests failed. A maintainer can provide more details.
@freschri I am getting failure on stack destroy consistently, hence it cannot pass e2e. Have you tried dropping (deleting) the blueprint stack with this addon in place? |
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests failed. A maintainer can provide more details.
failing after conflict resolution
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests failed. A maintainer can provide more details.
consistently leaves one ENI behind in a secondary subnet. this will have to be resolved before we include it in the release. @freschri |
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests passed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.