-
Notifications
You must be signed in to change notification settings - Fork 54
feat: setup tilt for easier local controller development #220
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: notebooks-v2
Are you sure you want to change the base?
Changes from all commits
5b74df0
443f5a9
f237715
3bd03c8
bc76a4e
01e2995
ce0479f
3063ce3
d582a12
efb3377
2c096e2
7879d29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,2 @@ | ||
# More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file | ||
# Ignore build and test binaries. | ||
bin/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
# Development Guide | ||
|
||
## Table of Contents | ||
|
||
- [Development Guide](#development-guide) | ||
- [Table of Contents](#table-of-contents) | ||
- [Introduction](#introduction) | ||
- [Prerequisites](#prerequisites) | ||
- [Getting Started](#getting-started) | ||
- [In an existing cluster](#in-an-existing-cluster) | ||
- [Using kind](#using-kind) | ||
- [Teardown](#teardown) | ||
- [Pull Request Checklist](#pull-request-checklist) | ||
- [Best Practices](#best-practices) | ||
- [Troubleshooting](#troubleshooting) | ||
- ["Build Failed: failed to dial gRPC: unable to upgrade to h2c, received 404"](#build-failed-failed-to-dial-grpc-unable-to-upgrade-to-h2c-received-404) | ||
|
||
## Introduction | ||
|
||
This guide will help you set up a development environment for the Kubeflow Notebooks project. | ||
|
||
## Prerequisites | ||
|
||
- Go >= 1.22 | ||
- Kubectl >= 1.22 | ||
- A Kubernetes cluster (e.g. [kind](https://kind.sigs.k8s.io/#installation-and-usage)) | ||
- Cert-manager installed in the cluster, see [cert-manager installation guide](https://cert-manager.io/docs/installation/#default-static-install) | ||
|
||
## Getting Started | ||
|
||
This project uses [Tilt](https://tilt.dev/) to manage the development environment. Tilt will watch for changes in the project and automatically rebuild the Docker image and redeploy the application in the **current Kubernetes context**. | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
|
||
### Cluster Selection | ||
|
||
Make sure you have a Kubernetes cluster running and `kubectl` is configured to use it. | ||
* `kubectl config current-context` will report which context Tilt will interact with | ||
|
||
💡 For development purposes, you may find using `kind` to be beneficial. You can create your own local cluster with the following command: | ||
- `kind create cluster` | ||
- This command will also change the `current-context` of `kubectl` to the `kind` cluster that is created | ||
|
||
### Running Tilt | ||
|
||
1. Run the following command to start Tilt: | ||
|
||
```bash | ||
make -C devenv tilt-up | ||
``` | ||
|
||
ℹ️ Please make sure you are in the `workspaces/controller` directory when executing the command. | ||
|
||
2. Hit `space` to open the Tilt dashboard in your browser and here you can see the logs and status of every resource deployed. | ||
|
||
## Teardown | ||
|
||
To stop Tilt and remove all resources created by it, run: | ||
|
||
```bash | ||
make -C devenv tilt-down | ||
``` | ||
|
||
ℹ️ Please make sure you are in the `workspaces/controller` directory when executing the command. | ||
|
||
## Pull Request Checklist | ||
|
||
Before raising a PR, ensure you run the following checks to maintain code quality and reliability: | ||
|
||
1. **Linting** | ||
```bash | ||
make lint | ||
``` | ||
- This runs static code analysis to ensure code style consistency | ||
- Fix any linting errors before proceeding | ||
|
||
2. **Unit Tests** | ||
```bash | ||
make test | ||
``` | ||
- Runs all unit tests in the project | ||
- Ensure all tests pass before submitting changes | ||
- Consider adding new tests for any new functionality | ||
|
||
3. **End-to-End Tests** | ||
```bash | ||
make test-e2e | ||
``` | ||
- Validates the complete workflow of the application | ||
- Requires a running Kubernetes cluster | ||
|
||
### Best Practices | ||
|
||
- Run tests locally before pushing changes | ||
- Write meaningful commit messages | ||
- Keep PRs focused and small | ||
- Update documentation if you change functionality | ||
- Consider adding new tests for new features | ||
- Run all checks in sequence before final submission | ||
|
||
## Troubleshooting | ||
|
||
### "Build Failed: failed to dial gRPC: unable to upgrade to h2c, received 404" | ||
|
||
If you see the following error message when tilt builds the image, try setting `DOCKER_BUILDKIT=0`: | ||
|
||
```bash | ||
DOCKER_BUILDKIT=0 make -C devenv tilt-up | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
bin/* | ||
tilt_config.json |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
detected_OS := $(shell uname -s) | ||
real_OS := $(detected_OS) | ||
arch := $(shell uname -m) | ||
goarch := $(shell go env GOARCH) | ||
ifeq ($(detected_OS),Darwin) | ||
detected_OS := mac | ||
real_OS := darwin | ||
endif | ||
ifeq ($(detected_OS),Linux) | ||
detected_OS := linux | ||
real_OS := linux | ||
endif | ||
|
||
## Cleanup targets | ||
|
||
.PHONY: cleanup-crds | ||
cleanup-crds: | ||
@echo "Cleaning up CRDs..." | ||
@kubectl delete -f ../config/crd/bases/ || true | ||
|
||
## Requirements | ||
|
||
.PHONY: check-cert-manager | ||
check-cert-manager: cmctl | ||
@echo "Verifying cert-manager installation..." | ||
@$(LOCALBIN)/cmctl check api > /dev/null 2>&1 || (printf "cert-manager is either not installed or not ready. Please install cert-manager or wait until it becomes ready.\nInstallation instructions can be found here: https://cert-manager.io/docs/installation/\n\n" && exit 1) | ||
@echo "cert-manager is installed and ready." | ||
|
||
## Location to install dependencies to | ||
LOCALBIN ?= $(shell pwd)/bin | ||
$(LOCALBIN): | ||
mkdir -p $(LOCALBIN) | ||
|
||
## Tool Binaries | ||
TILT ?= $(LOCALBIN)/tilt | ||
CMCTL ?= $(LOCALBIN)/cmctl | ||
|
||
## Tool Versions | ||
TILT_VERSION := 0.33.22 | ||
CMCTL_VERSION := 2.1.1 | ||
|
||
.PHONY: cmctl | ||
.PHONY: $(CMCTL) | ||
cmctl: $(CMCTL) | ||
$(CMCTL): $(LOCALBIN) | ||
test -s $(LOCALBIN)/cmctl || curl -fsSL https://github.com/cert-manager/cmctl/releases/download/v$(CMCTL_VERSION)/cmctl_$(real_OS)_$(goarch).tar.gz | tar -xz -C $(LOCALBIN) cmctl | ||
|
||
.PHONY: tilt | ||
.PHONY: $(TILT) | ||
tilt: $(TILT) | ||
$(TILT): $(LOCALBIN) | ||
test -s $(LOCALBIN)/tilt || curl -fsSL https://github.com/tilt-dev/tilt/releases/download/v$(TILT_VERSION)/tilt.$(TILT_VERSION).$(detected_OS).$(arch).tar.gz | tar -xz -C $(LOCALBIN) tilt | ||
|
||
tilt-up: tilt check-cert-manager | ||
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for checking this PR, yes the Makefile had a bug in line 25, cmctl was meant to be in line 26, I fixed it in the latest commit eda5804 |
||
$(LOCALBIN)/tilt up | ||
|
||
tilt-down: tilt cleanup-crds | ||
$(LOCALBIN)/tilt down |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,122 @@ | ||||||||||
load("ext://restart_process", "docker_build_with_restart") | ||||||||||
|
||||||||||
load_dynamic("./configs/tiltfiles/Tiltfile.setup") | ||||||||||
|
||||||||||
config.define_string_list("services") | ||||||||||
|
||||||||||
parsed_config = config.parse() | ||||||||||
|
||||||||||
for service in parsed_config.get("services", []): | ||||||||||
load_dynamic("./configs/tiltfiles/Tiltfile.%s" % (service)) | ||||||||||
|
||||||||||
manifests = kustomize("../config/default") | ||||||||||
|
||||||||||
objects = decode_yaml_stream(manifests) | ||||||||||
|
||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should clarify why we are doing this wacky manipulation of securityContext:
Suggested change
|
||||||||||
for o in objects: | ||||||||||
if o["kind"] == "Deployment" and o.get("metadata").get("name") in ["workspace-controller-controller-manager"]: | ||||||||||
o["spec"]["template"]["spec"]["securityContext"] = {"runAsNonRoot": False, "readOnlyRootFilesystem": False} | ||||||||||
o["spec"]["template"]["spec"]["containers"][0]["imagePullPolicy"] = "Always" | ||||||||||
|
||||||||||
overridden_manifests = encode_yaml_stream(objects) | ||||||||||
Comment on lines
+14
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious why we are programmatically modifying manifests here... instead of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't want to create an overlay for local development, but I am open to do it if the community agrees There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thesuperzapper what are your thoughts on a kustomize local development overlay vs. this programmatic approach? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the correct approach because these patches are about making tilt work (allowing hot swap of go binaries in the manager container) rather than about enabling development in general. |
||||||||||
|
||||||||||
k8s_yaml(overridden_manifests, allow_duplicates=True) | ||||||||||
|
||||||||||
local_resource( | ||||||||||
"cert-manager-req-check", | ||||||||||
serve_cmd="while true; do sleep 86400; done", | ||||||||||
labels="requirements", | ||||||||||
readiness_probe=probe( | ||||||||||
exec=exec_action( | ||||||||||
command=["/bin/sh", "-c", "./bin/cmctl check api"] | ||||||||||
), initial_delay_secs=5, timeout_secs=60 | ||||||||||
) | ||||||||||
) | ||||||||||
|
||||||||||
k8s_resource( | ||||||||||
new_name="certs", | ||||||||||
objects=[ | ||||||||||
"workspace-controller-serving-cert:certificate", | ||||||||||
"workspace-controller-selfsigned-issuer:issuer" | ||||||||||
], | ||||||||||
labels="controller", | ||||||||||
resource_deps=[ | ||||||||||
"controller-namespace", | ||||||||||
"cert-manager-req-check" | ||||||||||
] | ||||||||||
) | ||||||||||
|
||||||||||
k8s_resource( | ||||||||||
new_name="reqs", | ||||||||||
objects=[ | ||||||||||
"workspace-controller-controller-manager:serviceaccount", | ||||||||||
"workspace-controller-leader-election-role:role", | ||||||||||
"workspace-controller-manager-role:clusterrole", | ||||||||||
"workspace-controller-workspace-editor-role:clusterrole", | ||||||||||
"workspace-controller-workspace-viewer-role:clusterrole", | ||||||||||
"workspace-controller-workspacekind-editor-role:clusterrole", | ||||||||||
"workspace-controller-workspacekind-viewer-role:clusterrole", | ||||||||||
"workspace-controller-leader-election-rolebinding:rolebinding", | ||||||||||
"workspace-controller-manager-rolebinding:clusterrolebinding", | ||||||||||
"workspace-controller-validating-webhook-configuration:validatingwebhookconfiguration" | ||||||||||
], | ||||||||||
labels="controller", | ||||||||||
resource_deps=[ | ||||||||||
"controller-namespace" | ||||||||||
] | ||||||||||
) | ||||||||||
|
||||||||||
k8s_resource( | ||||||||||
new_name="crds", | ||||||||||
objects=[ | ||||||||||
"workspacekinds.kubeflow.org:customresourcedefinition", | ||||||||||
"workspaces.kubeflow.org:customresourcedefinition" | ||||||||||
], | ||||||||||
labels="controller", | ||||||||||
resource_deps=[ | ||||||||||
"controller-namespace", | ||||||||||
] | ||||||||||
) | ||||||||||
|
||||||||||
k8s_resource( | ||||||||||
new_name="controller-namespace", | ||||||||||
objects=["workspace-controller-system:Namespace:default"], | ||||||||||
labels="requirements" | ||||||||||
) | ||||||||||
|
||||||||||
k8s_resource( | ||||||||||
workload="workspace-controller-controller-manager", | ||||||||||
new_name="controller", | ||||||||||
labels="controller", | ||||||||||
resource_deps=[ | ||||||||||
"controller-namespace", | ||||||||||
"cert-manager-req-check", | ||||||||||
"certs" | ||||||||||
] | ||||||||||
) | ||||||||||
Comment on lines
+36
to
+96
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we confirm if these extra label/group are actually required to run this, that is, if we can successfully tilt-up and down a few times without these configs being here? I get the need to confirm that cert-manager is up and running, and possibly that it has provisioned the cert for the webhook, but that will happen naturally (similar to when the user applies the manifests themselves). The reason I ask is that I think people won't keep these lists up to date, and I am not sure that the grouping are really meaningful to developers of the controller either way. Separately, we might want to enable |
||||||||||
|
||||||||||
local_resource( | ||||||||||
"manager-bin", | ||||||||||
"CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o bin/manager cmd/main.go", | ||||||||||
dir = "../", | ||||||||||
deps = [ | ||||||||||
"../cmd", | ||||||||||
"../internal", | ||||||||||
"../go.mod", | ||||||||||
"../go.sum", | ||||||||||
], | ||||||||||
labels="controller", | ||||||||||
) | ||||||||||
|
||||||||||
docker_build_with_restart( | ||||||||||
"ghcr.io/kubeflow/notebooks/workspace-controller", | ||||||||||
context = "../", | ||||||||||
dockerfile = "../tilt.dockerfile", | ||||||||||
entrypoint = ["/manager"], | ||||||||||
only=[ | ||||||||||
"bin/", | ||||||||||
], | ||||||||||
live_update = [ | ||||||||||
sync("../bin/manager", "/manager"), | ||||||||||
], | ||||||||||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Disable analytics | ||
analytics_settings(False) | ||
|
||
update_settings(k8s_upsert_timeout_secs = 120) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
FROM alpine:3.12 | ||
|
||
WORKDIR / | ||
|
||
COPY bin/manager /manager | ||
|
||
ENTRYPOINT ["/manager"] |
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.
@Al-Pragliola can we please add something about running the the tests locally before raising a PR:
make lint
make test
(for unit tests without a local kube/kind)make test-e2e
(run intergration tests on a local kube)As people wont know they need to do this otherwise
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.
wrote a small section in 7879d29