-
Notifications
You must be signed in to change notification settings - Fork 635
tilt: improve Tilt development workflow (#12956) #13255
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
Remove test values file reference and add KGW_DISABLE_LEADER_ELECTION=true via --set Signed-off-by: Wolf-06 <[email protected]>
tilt-settings.yaml
Outdated
| helm_values_files: | ||
| - ./test/e2e/tests/manifests/common-recommendations.yaml | ||
| helm_values_files: | ||
| - /tmp/strict-mode.yaml |
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.
where is this value coming from?
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.
Apparently, I have committed a local file which was meant to test KGW_VALIDATION_MODE=STRICT .
I will clean it.
Edit: fixed it
thank you
Signed-off-by: Wolf-06 <[email protected]>
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.
Pull request overview
This PR improves the Tilt development workflow by removing CI-specific configuration and enabling debugger-friendly settings for local development.
Changes:
- Removed default reference to
test/kubernetes/e2e/tests/manifests/common-recommendations.yamlwhich was intended for CI/E2E testing - Added
--set controller.extraEnv.KGW_DISABLE_LEADER_ELECTION='true'to disable leader election during local development, which is required for debugging - Updated configuration files to use empty helm values by default with helpful comments for customization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Tiltfile | Removed test values file reference from default settings and added leader election disabling via helm --set flag in both template and install commands |
| tilt-settings.yaml | Changed helm_values_files from referencing the test file to an empty array with a comment showing how to add custom values files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I don't think validation mode will work until envoyinit Dockerfile is incorporated. To test if validation mode is working set KGW_VALIDATION_MODE to STRICT. you should NOT see this error in the kgateway controller logs |
Signed-off-by: Wolf-06 <[email protected]>
Head branch was pushed to by a user without write access
|
Hi @puertomontt @yuval-k Changes:
How to Test Strict Validation ModePrerequisites# Create Kind cluster with local registry
ctlptl create cluster kind --name kind-kgateway --registry=ctlptl-registry
# Build and load images
VERSION=1.0.0-ci1 CLUSTER_NAME=kgateway make kind-build-and-loadStep 1: Configure Validation ModeEdit helm_sets:
controller.extraEnv.KGW_VALIDATION_MODE: "strict" # or "standard"Step 2: Start Tilttilt upStep 3: Apply Test Resources# Apply Gateway
kubectl apply -f examples/example-gw.yaml
# Apply invalid route with malformed regex
kubectl apply -f test/e2e/features/routereplacement/testdata/invalid-matcher.yamlStep 4: Verify Results# Check HTTPRoute status
kubectl get httproute invalid-matcher-route -n default -o yaml | grep -A 10 "conditions:"Expected Results:
Step 5: Check Controller Logskubectl logs -n kgateway-system deployment/kgateway --tail=20 | grep -i "invalid"In STRICT mode, you'll see: {"msg":"invalid matcher","error":"invalid matcher configuration: validation failed: invalid xds configuration: error initializing configuration '/dev/fd/0': missing ]: [a-z"}Switching Between Modes
Notes
Thank you |
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1a3ea70 to
b111c5f
Compare
… fixes Signed-off-by: Wolf-06 <[email protected]>
b111c5f to
ead187c
Compare
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| helm_installation_name: kgateway | ||
| helm_values_files: | ||
| - ./test/e2e/tests/manifests/common-recommendations.yaml |
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'm personally not in favour of this as the common-recommendations.yaml contain recommended values, especially in prod. They are used in tests and tilt is used for validating behaviour when writing tests, not just local dev
Instead, devs can just comment out this line when bringing up tilt
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 file is kind of a holdover from v1, doesn't really have much in it. Tilt needs some specific values, esp. KGW_DISABLE_LEADER_ELECTION.
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.
Looking at the file again, I see now that it doesn't have a lot and is leftover from v1, but afaict most tests use this as the ProfileValuesManifestFile. The other specific values can be added to a tilt-profile.yaml or passed as helm flags in tilt-setting.yaml
| label: kgateway | ||
| # Command to build the binary | ||
| #build_binary: GCFLAGS='all="-N -l"' make -B kgateway | ||
| #build_binary: make -B kgateway GCFLAGS="all=-N -l" LDFLAGS="" |
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.
Could this change be reverted? It only updates 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.
Hi @davidjumani
It is not just a comment. It is an option of which type of binary to build.
make -B kgateway GCFLAGS="all=-N -l" LDFLAGS="" is used to build the kgateway binary specifically for debugging.
I will add a comment to make it more clear, does that work ?
Thank you
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.
What I mean is that the line is commented out, so any changes made don't have an effect. The command to build the binary is the subsequent line build_binary: make -B kgateway
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.
make -B kgateway and make -B kgateway GCFLAGS="all=-N -l" LDFLAGS="" are basically two options for building the binary with different motives.
- With
make -B kgateway GCFLAGS="all=-N -l" LDFLAGS=""we build binary with motive of debugging - With
make -B kgatewaywe build the normal binary with optimisation and Inlining.
Use case:
- use the standard one for normal coding and testing.
- switch to the debugger one ONLY if we are actively trying to attach a debugger
We toggle between the options by commenting the other and uncommenting the one we wan to use.
i will add comments for the instruction on using them to make it clear.
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.
Has debugging been verified with this change ?
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.
Yes
I have verified the debugging with the help of breakpoints in reconcile func (in pkg/kgateway/controller and it hit the breakpoints.
I tried the same with normal build (keeping the debug_port as it is), it didn't hit the breakpoint.
Tiltfile
Outdated
| "helm_installation_name": "kgateway", | ||
| "helm_installation_namespace": "kgateway-system", | ||
| "helm_values_files": ["./test/kubernetes/e2e/tests/manifests/common-recommendations.yaml"], | ||
| "helm_values_files": [], |
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.
Not in favour of this change
Tiltfile
Outdated
| """ | ||
| return str(value).replace("'", "'\"'\"'") | ||
|
|
||
| get_resources_cmd = "{0} -n {1} template {2} --include-crds install/helm/kgateway/ --set image.pullPolicy='Never' --set image.registry=ghcr.io/kgateway-dev --set image.tag='{3}' --set controller.extraEnv.KGW_DISABLE_LEADER_ELECTION='true'".format(helm_cmd, settings.get("helm_installation_namespace"), settings.get("helm_installation_name"), image_tag) |
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.
Lets move this over helm_sets. This should be as clean as possible as it only gets the resources and not installs kgateway
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.
Working on it.
Tiltfile
Outdated
| kubectl get crd gateways.gateway.networking.k8s.io &> /dev/null || {{ kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.1.0/standard-install.yaml; }} ; | ||
| {0} upgrade --install -n {1} --create-namespace kgateway-crds install/helm/kgateway-crds ; | ||
| {0} upgrade --install -n {1} --create-namespace {2} install/helm/kgateway/ --set controller.image.pullPolicy='Never' --set image.registry=ghcr.io/kgateway-dev --set image.tag='{3}'""".format(helm_cmd, settings.get("helm_installation_namespace"), settings.get("helm_installation_name"), image_tag) | ||
| {0} upgrade --install -n {1} --create-namespace {2} install/helm/kgateway/ --set controller.image.pullPolicy='Never' --set image.registry=ghcr.io/kgateway-dev --set image.tag='{3}' --set controller.extraEnv.KGW_DISABLE_LEADER_ELECTION='true'""".format(helm_cmd, settings.get("helm_installation_namespace"), settings.get("helm_installation_name"), image_tag) |
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.
as above, lets move this over to helm_sets
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.
Working on it.
tilt-settings.yaml
Outdated
| helm_installation_namespace: kgateway-system | ||
|
|
||
| # Inline Helm --set flags (key: value format) | ||
| helm_sets: {} |
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.
can this be renamed to helm_flags
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.
Sure.
Signed-off-by: Wolf-06 <[email protected]>
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
This PR improves the Tilt development workflow by addressing issue #12956.
Changes
Removed default test values file reference: The Tiltfile no longer defaults to using
test/kubernetes/e2e/tests/manifests/common-recommendations.yaml, which was meant for CI/E2E testing, not local development.Added KGW_DISABLE_LEADER_ELECTION via --set: Leader election is now disabled by default when using Tilt, which is required for debugging. This is passed via Helm
--set controller.extraEnv.KGW_DISABLE_LEADER_ELECTION='true'instead of a separate values file.Verified KGW_VALIDATION_MODE=STRICT works: Tested that
ValidationMode=strictworks correctly with the current setup. Users can enable it by adding a custom values file tohelm_values_filesin [tilt-settings.yaml]How to Test
Run
tilt up.Inspect the deployment manifest or logs to confirm KGW_DISABLE_LEADER_ELECTION is set to 'true'.
Closes #12956
Files Modified
Change Type
Changelog
Additional Notes