-
Notifications
You must be signed in to change notification settings - Fork 78
WIP: Add instructions for running the example driver on GKE #93
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mortent The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
nojnhuh
left a comment
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.
I don't have a GCP account where I could test this myself, but the docs look good overall, thanks! I left a few small comments inline.
| enabled by default in GKE since 1.32.1-gke.1489001, so we will create | ||
| a cluster in the rapid channel to make sure we get a recent version. | ||
|
|
||
| Since DRA is still a beta feature, we need to explicitely enable it |
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.
| Since DRA is still a beta feature, we need to explicitely enable it | |
| Since DRA is still a beta feature, we need to explicitly enable it |
| deployments/helm/dra-example-driver | ||
| ``` | ||
|
|
||
| The examples in `demo/gpu-test{1,2,3,4,5}.yaml` works just like with Kind. |
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.
| The examples in `demo/gpu-test{1,2,3,4,5}.yaml` works just like with Kind. | |
| The examples in `demo/gpu-test{1,2,3,4,5}.yaml` work just like with Kind. |
| ./demo/delete-cluster.sh | ||
| ``` | ||
|
|
||
| ## Installing the example driver on a GKE cluster |
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.
It would be great to also run the e2e test on GKE to make sure we don't accidentally break this. I'm not sure if there's an existing pattern to do that in Prow or if GitHub Actions makes that easy. We can definitely address that later.
| --namespace dra-example-driver \ | ||
| --set=resourcequota.enabled=true \ | ||
| dra-example-driver \ | ||
| deployments/helm/dra-example-driver |
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.
Using the chart from the main branch might not always be compatible with the latest released version of the container image. Ideally we'd stick to either building the image from source for the checked out version of the chart (like the e2e CI tests do), or use compatible published release versions of each. I see published releases won't work here immediately without these unreleased changes to the chart, but we could merge the chart changes, cut a chart release, then merge the docs.
Seamless upgrade support for 1.33 is one thing I'm anticipating which will involve changes to both the image and the chart. Installing a chart that is set up for seamless upgrades and an image that isn't will likely cause issues, though that might only be in marginal cases users walking through the demo wouldn't normally hit. I haven't thought that particular scenario all the way through. For sensitive changes like that I'm confident we can work things out such that a little bit of skew is probably fine, but zero skew is of course preferred.
| name: "" | ||
|
|
||
| resourcequota: | ||
| enabled: false |
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.
I'd vote to enable this for e2e tests for that extra bit of coverage like we do for the webhook which is disabled by default:
dra-example-driver/test/e2e/setup-e2e.sh
Line 36 in e82f291
| --set webhook.enabled=true \ |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@mortent: please address the comments and rebase. |
|
/retitle WIP: Add instructions for running the example driver on GKE |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
We currently only provide instructions for running the example driver on Kind. We should provide instructions for how to run the driver on other Kubernetes distributions, as they will include some different steps. This PR adds instructions for how to get the example driver running on GKE.
For now this just adds a new section with the new content. Ideally we should restructure the README file, so that we can provide separate instructions for getting the driver running on a cluster, and then a shared section for running the examples. I decided not to do that in this PR, as I want to make sure we have agreement on a structure first. Created an issue for this: #94