Introduce builder #559
Introduce builder #559
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
dosubot
bot
added
the
size:L
WalkthroughThis pull request introduces a comprehensive setup for a Kubernetes-based builder environment within the Cozy stack. The changes include creating a Helm chart for the builder, defining a Makefile for managing the builder deployment, configuring OCI worker storage and garbage collection, and setting up a Kubernetes sandbox for a Talos imager. The modifications aim to streamline the image building process by leveraging Kubernetes infrastructure and providing flexible configuration options for resource management. Changes
Sequence DiagramsequenceDiagram
participant Makefile as Installer Makefile
participant Builder as Kubernetes Builder
participant Imager as Talos Imager Pod
Makefile->>Builder: run-builder
Builder-->>Makefile: Builder ready
Makefile->>Imager: Execute image build commands
Imager-->>Makefile: Image build complete
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/core/builder/config.toml (1)
9-11: Consider adding keepDuration to the second GC policyThe second policy lacks a
keepDurationparameter while keeping a larger storage quota (~50GB). This could lead to unbounded storage growth over time. Consider adding a duration limit to ensure proper cleanup of older artifacts.[[worker.oci.gcpolicy]] all = true keepBytes = 53687091200 + keepDuration = 2592000 # 30 days retentionpackages/core/builder/Makefile (2)
4-4: Add error handling for TALOS_VERSION extractionThe version extraction could fail silently if the YAML file is missing or malformed. Consider adding validation:
-TALOS_VERSION=$(shell awk '/^version:/ {print $$2}' ../installer/images/talos/profiles/installer.yaml) +TALOS_VERSION=$(shell awk '/^version:/ {print $$2}' ../installer/images/talos/profiles/installer.yaml) +$(if $(TALOS_VERSION),,$(error Failed to extract TALOS_VERSION))
33-35: Add timeout and feedback to wait conditionsThe wait commands could hang indefinitely. Add timeout and better error handling:
wait-for-builder: - kubectl wait deploy --for=condition=Progressing -n $(NAMESPACE) $(NAME)-talos-imager - kubectl wait pod --for=condition=Ready -n $(NAMESPACE) -l app=$(NAME)-talos-imager + kubectl wait deploy --timeout=5m --for=condition=Progressing -n $(NAMESPACE) $(NAME)-talos-imager || (echo "Deployment failed to progress" && exit 1) + kubectl wait pod --timeout=5m --for=condition=Ready -n $(NAMESPACE) -l app=$(NAME)-talos-imager || (echo "Pods failed to become ready" && exit 1)packages/core/installer/Makefile (1)
22-23: Add error handling for configuration updatesThe configuration update logic could be more robust:
- Add checks for file existence
- Validate YAML structure before updating
- Consider using yq for both reading and writing to ensure consistent YAML handling
- IMAGE=$$(awk '/^version:/ {print $$2}' images/talos/profiles/installer.yaml) \ - yq -i '.talos.imager.image = "ghcr.io/siderolabs/imager:" + strenv(IMAGE)' ../builder/values.yaml + @if [ ! -f images/talos/profiles/installer.yaml ]; then \ + echo "Error: installer.yaml not found"; \ + exit 1; \ + fi + @if [ ! -f ../builder/values.yaml ]; then \ + echo "Error: builder values.yaml not found"; \ + exit 1; \ + fi + IMAGE=$$(yq e '.version' images/talos/profiles/installer.yaml) \ + yq -i '.talos.imager.image = "ghcr.io/siderolabs/imager:" + env(IMAGE)' ../builder/values.yaml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/builder/Chart.yaml(1 hunks)packages/core/builder/Makefile(1 hunks)packages/core/builder/config.toml(1 hunks)packages/core/builder/templates/sandbox.yaml(1 hunks)packages/core/builder/values.yaml(1 hunks)packages/core/installer/Makefile(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/core/builder/values.yaml
- packages/core/builder/Chart.yaml
🧰 Additional context used
🪛 yamllint (1.35.1)
packages/core/builder/templates/sandbox.yaml
[error] 12-12: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (2)
packages/core/builder/config.toml (1)
2-3: Specify units for gckeepstorage valueThe
gckeepstorageparameter value (50000) lacks unit specification. Please clarify if this represents bytes, megabytes, or another unit to avoid ambiguity.packages/core/installer/Makefile (1)
27-27: Verify builder readiness in image targetsWhile adding
run-builderas a prerequisite is good, consider adding explicit verification that the builder is operational before proceeding with image operations.Consider adding a check like this to the image targets:
image-talos: run-builder + @kubectl wait --for=condition=ready pod -l app=builder-talos-imager -n cozy-builder --timeout=60s test -f ../../../_out/assets/installer-amd64.tar || make talos-installer skopeo copy docker-archive:../../../_out/assets/installer-amd64.tar docker://$(REGISTRY)/talos:$(call settag,$(TALOS_VERSION))Also applies to: 42-42, 44-44, 46-46
| securityContext: | ||
| privileged: true | ||
| command: |
There was a problem hiding this comment.
Review security implications of privileged container
Running containers in privileged mode with host device access poses significant security risks. Consider:
- Implementing more granular capabilities instead of full privileged mode
- Adding SecurityContext restrictions at the pod level
- Restricting the deployment to specific nodes using nodeSelector
| automountServiceAccountToken: false | ||
| terminationGracePeriodSeconds: 1 | ||
| containers: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve container health management
Current configuration has several operational concerns:
- Using
sleep infinitymakes it difficult to implement proper health checks - No resource limits specified
- No readiness/liveness probes defined
Consider adding:
spec:
automountServiceAccountToken: false
terminationGracePeriodSeconds: 1
+ resources:
+ limits:
+ cpu: "1"
+ memory: "1Gi"
+ requests:
+ cpu: "100m"
+ memory: "128Mi"
containers:
- name: imagere
image: "{{ .Values.talos.imager.image }}"
+ livenessProbe:
+ exec:
+ command:
+ - sh
+ - -c
+ - "ps aux | grep -v grep | grep sleep"
+ initialDelaySeconds: 5
+ periodSeconds: 10Also applies to: 33-35
| delete: ## Remove builder sandbox from existing Kubernetes cluster. | ||
| kubectl delete deploy -n $(NAMESPACE) $(NAME)-talos-imager | ||
| docker buildx rm buildkit-$(NAME) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cleanup process in delete target
The delete target could leave orphaned resources. Consider:
- Adding namespace cleanup if empty
- Adding error handling for non-existent resources
- Cleaning up any persistent volumes/claims
delete: ## Remove builder sandbox from existing Kubernetes cluster.
- kubectl delete deploy -n $(NAMESPACE) $(NAME)-talos-imager
- docker buildx rm buildkit-$(NAME)
+ -kubectl delete deploy -n $(NAMESPACE) $(NAME)-talos-imager || true
+ -docker buildx rm buildkit-$(NAME) || true
+ @echo "Checking for orphaned resources in namespace..."
+ @if [ -z "$$(kubectl get all -n $(NAMESPACE) -o name)" ]; then \
+ kubectl delete namespace $(NAMESPACE) || true; \
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete: ## Remove builder sandbox from existing Kubernetes cluster. | |
| kubectl delete deploy -n $(NAMESPACE) $(NAME)-talos-imager | |
| docker buildx rm buildkit-$(NAME) | |
| delete: ## Remove builder sandbox from existing Kubernetes cluster. | |
| -kubectl delete deploy -n $(NAMESPACE) $(NAME)-talos-imager || true | |
| -docker buildx rm buildkit-$(NAME) || true | |
| @echo "Checking for orphaned resources in namespace..." | |
| @if [ -z "$$(kubectl get all -n $(NAMESPACE) -o name)" ]; then \ | |
| kubectl delete namespace $(NAMESPACE) || true; \ | |
| fi |
| kubectl exec -i -n cozy-builder deploy/builder-talos-imager -- imager --tar-to-stdout - | \ | ||
| tar -C ../../../_out/assets -xzf- |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance reliability of kubectl exec operations
The kubectl exec pipeline could be more robust with proper error handling and timeouts.
- kubectl exec -i -n cozy-builder deploy/builder-talos-imager -- imager --tar-to-stdout - | \
- tar -C ../../../_out/assets -xzf-
+ if ! kubectl exec -i -n cozy-builder deploy/builder-talos-imager --timeout=5m -- \
+ imager --tar-to-stdout - > /tmp/imager.tar; then
+ echo "Error: imager command failed"
+ exit 1
+ fi
+ if ! tar -C ../../../_out/assets -xzf /tmp/imager.tar; then
+ echo "Error: failed to extract imager output"
+ rm -f /tmp/imager.tar
+ exit 1
+ fi
+ rm -f /tmp/imager.tar📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl exec -i -n cozy-builder deploy/builder-talos-imager -- imager --tar-to-stdout - | \ | |
| tar -C ../../../_out/assets -xzf- | |
| if ! kubectl exec -i -n cozy-builder deploy/builder-talos-imager --timeout=5m -- \ | |
| imager --tar-to-stdout - > /tmp/imager.tar; then | |
| echo "Error: imager command failed" | |
| exit 1 | |
| fi | |
| if ! tar -C ../../../_out/assets -xzf /tmp/imager.tar; then | |
| echo "Error: failed to extract imager output" | |
| rm -f /tmp/imager.tar | |
| exit 1 | |
| fi | |
| rm -f /tmp/imager.tar |
| run-builder: | ||
| make -C ../builder/ apply wait-for-builder |
There was a problem hiding this comment.
💡 Codebase verification
Declare run-builder as .PHONY target and fix dependency
The run-builder target needs two improvements:
- It should be declared as .PHONY since it doesn't produce a file output
- The wait-for-builder target exists in the builder Makefile but it's not properly chained as a dependency
+.PHONY: run-builder
run-builder:
- make -C ../builder/ apply wait-for-builder
+ make -C ../builder/ apply
+ make -C ../builder/ wait-for-builderThe wait-for-builder target in the builder Makefile waits for Kubernetes resources to be ready, so it should be executed after the apply target completes, not as a single command.
🔗 Analysis chain
Declare run-builder as .PHONY target
The run-builder target should be declared as .PHONY since it doesn't produce a file output.
+.PHONY: run-builder
run-builder:
make -C ../builder/ apply wait-for-builderAlso, verify that the wait-for-builder target exists in the builder Makefile:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if wait-for-builder target exists in builder Makefile
grep -A 2 "^wait-for-builder:" ../builder/Makefile
Length of output: 105
Script:
#!/bin/bash
# First, let's find the builder Makefile location
fd Makefile --type f
# Then check for wait-for-builder target in all Makefiles
rg "^wait-for-builder:" --type make
Length of output: 2836
Script:
#!/bin/bash
# Let's check the implementation of wait-for-builder target
rg -A 3 "^wait-for-builder:" packages/core/builder/Makefile
Length of output: 249
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
New Features
Chores
Infrastructure