Add Kubernetes deployment option mirroring compose stack (#9)#11
Add Kubernetes deployment option mirroring compose stack (#9)#11
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces Kubernetes deployment manifests that mirror the existing Docker Compose observability stack, providing a complete path for deploying the LGTM (Loki, Grafana, Tempo, Mimir/Prometheus) stack in Kubernetes environments.
- Adds comprehensive Kubernetes manifests with base configurations and environment-specific overlays
- Provides local development support via kind/k3d/minikube with appropriate image policies
- Includes production-ready templates with resource limits, storage classes, and ingress configurations
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| k8s/base/*.yaml | Core Kubernetes manifests for all stack components (Grafana, Loki, Tempo, Prometheus, OTel Collector, FastAPI app, loadgen) |
| k8s/base/files/* | Configuration files mirroring the compose setup for each component |
| k8s/overlays/local/* | Local development overlay with Never pull policy and local image tags |
| k8s/overlays/production/* | Production overlay with resource limits, storage classes, and ingress definitions |
| .github/workflows/integration-tests.yml | CI integration to test Kubernetes deployment using kind |
| Makefile | Make targets for applying/deleting Kubernetes manifests |
| README.md | Comprehensive documentation for Kubernetes deployment workflows |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| registry: | ||
| external_labels: # Labels attached to generated metrics (use meaningful cluster/service labels in prod) | ||
| source: tempo | ||
| cluster: docker-compose |
There was a problem hiding this comment.
The cluster label value 'docker-compose' is misleading in a Kubernetes context. Consider using a more generic value like 'demo' or 'local' that applies to both deployment methods.
| cluster: docker-compose | |
| cluster: demo |
k8s/overlays/production/ingress.yaml
Outdated
| ingressClassName: nginx | ||
| tls: | ||
| - hosts: | ||
| - grafana.example.com |
There was a problem hiding this comment.
The example.com hostnames in production ingress should be documented as placeholders that need to be replaced with actual hostnames before deployment.
k8s/overlays/production/ingress.yaml
Outdated
| ingressClassName: nginx | ||
| tls: | ||
| - hosts: | ||
| - app.example.com |
There was a problem hiding this comment.
The example.com hostnames in production ingress should be documented as placeholders that need to be replaced with actual hostnames before deployment.
hyzhak
left a comment
There was a problem hiding this comment.
Addressed the review feedback in the latest commit—details inline.
| registry: | ||
| external_labels: # Labels attached to generated metrics (use meaningful cluster/service labels in prod) | ||
| source: tempo | ||
| cluster: demo |
There was a problem hiding this comment.
Thanks for spotting this! I've updated the metrics generator label to use cluster: demo so the value makes sense in both compose and Kubernetes environments.
| spec: | ||
| ingressClassName: nginx | ||
| tls: | ||
| - hosts: |
There was a problem hiding this comment.
Good call. I added a YAML comment above the spec and inline on the host entries to make it clear that the example.com domains must be replaced before deploying.
| spec: | ||
| ingressClassName: nginx | ||
| tls: | ||
| - hosts: |
There was a problem hiding this comment.
Added inline placeholder notes for the app.example.com entry as well so both ingress hosts now explicitly call out the need for customization.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ingressClassName: nginx | ||
| tls: | ||
| - hosts: | ||
| - grafana.example.com # placeholder hostname |
There was a problem hiding this comment.
The placeholder hostname comment is inline but should be properly documented. Consider adding a block comment above the TLS section explaining that all example.com hostnames must be replaced with actual domains before deployment.
There was a problem hiding this comment.
could it be even better to add details into Readme md?
| ingressClassName: nginx | ||
| tls: | ||
| - hosts: | ||
| - app.example.com # placeholder hostname |
There was a problem hiding this comment.
This placeholder hostname comment is inline but should be properly documented. Consider adding a block comment above this TLS section explaining that all example.com hostnames must be replaced with actual domains before deployment.
There was a problem hiding this comment.
could it be even better to add details into Readme md?
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| replication_factor: 1 | ||
| chunk_idle_period: 1h | ||
| chunk_target_size: 1536000 | ||
| # NOTE: removed: max_transfer_retries (deprecated/invalid) |
There was a problem hiding this comment.
[nitpick] This comment references a removed configuration option but doesn't provide context about when it was deprecated or what replaced it, which could be helpful for users migrating from older Loki versions.
| # NOTE: removed: max_transfer_retries (deprecated/invalid) | |
| # NOTE: removed: max_transfer_retries (deprecated/invalid as of Loki 2.8.0; no direct replacement—retries are now managed internally) |
| tsdb_shipper: | ||
| active_index_directory: /loki/tsdb-shipper-active | ||
| cache_location: /loki/tsdb-shipper-cache | ||
| # NOTE: do not set shared_store here; not needed for filesystem |
There was a problem hiding this comment.
[nitpick] These comments about shared_store appear twice with slightly different explanations. Consider consolidating them or making the distinction clearer.
|
|
||
| compactor: | ||
| working_directory: /loki/compactor | ||
| # NOTE: removed: shared_store (deprecated/invalid) |
There was a problem hiding this comment.
[nitpick] These comments about shared_store appear twice with slightly different explanations. Consider consolidating them or making the distinction clearer.
Summary
k8s/(namespace, PVCs, Deployments, Services, ConfigMaps, Secrets) plus local and production kustomize overlaysdocs/k8s-manifests.mdTesting