Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chart changes to fix race condition during helm install #75

Merged
merged 8 commits into from
Feb 23, 2024
Merged

chart changes to fix race condition during helm install #75

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b8b870a
make cert-manager a pre-requisite instead of direct dependency
rajatjindal Feb 20, 2024
a9f105c
remove helm-hooks to ensure issuer and serving-cert template are setu…
rajatjindal Feb 20, 2024
d57e4a5
move setting up SpinAppExecutor as post-install hook
rajatjindal Feb 20, 2024
b3ccfdf
add smoke test for helm install
rajatjindal Feb 20, 2024
9ff484b
setup spinappexecutor in default namespace
rajatjindal Feb 20, 2024
21c8b91
address review comments
rajatjindal Feb 21, 2024
01bfecb
use local docker spin-operator image
rajatjindal Feb 22, 2024
462fbfe
helm install implicitly generate and build helm chart with dependencies
rajatjindal Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions .github/workflows/helm-install-smoketest.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
name: Helm Install Smoketest

on:
pull_request:
branches: [main]

jobs:
helm-install-smoke-test:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup Go
uses: actions/setup-go@v5
with:
go-version: "1.22.x"

- name: Install helm
uses: Azure/setup-helm@v3
with:
version: v3.14.0

- name: setup k3d
uses: engineerd/configurator@v0.0.10
with:
name: k3d
url: https://github.com/k3d-io/k3d/releases/download/v5.6.0/k3d-linux-amd64

- name: create spin-operator docker image
run: make docker-build IMG=spin-operator:latest

- name: start k3d cluster
run: |
k3d cluster create wasm-cluster \
--image ghcr.io/deislabs/containerd-wasm-shims/examples/k3d:v0.11.0 \
--port "8081:80@loadbalancer" \
--agents 2

- name: import operator image into k3d cluster
run: k3d image import -c wasm-cluster spin-operator:latest

- name: install crd
run: make install

- name: apply runtime class
run: kubectl apply -f spin-runtime-class.yaml

- name: helm install cert-manager
run: |
helm repo add jetstack https://charts.jetstack.io
helm install cert-manager jetstack/cert-manager \
--namespace cert-manager \
--create-namespace \
--version v1.13.3 \
--set installCRDs=true

- name: helm install spin-operator
run: |
make helm-install IMG=spin-operator:latest

- name: debug
if: failure()
run: |
kubectl get pods -A
kubectl get pods -n spin-operator
kubectl logs -n spin-operator $(kubectl get pods -n spin-operator | grep spin-operator-controller-manager | awk '{print $1}')
kubectl describe -n spin-operator pod $(kubectl get pods -n spin-operator | grep spin-operator-controller-manager | awk '{print $1}')
kubectl logs -n spin-operator $(kubectl get pods -n spin-operator | grep wait-for-webhook-svc | awk '{print $1}')
kubectl describe -n spin-operator pod $(kubectl get pods -n spin-operator | grep wait-for-webhook-svc | awk '{print $1}')

- name: run spin app
run: |
kubectl apply -f config/samples/simple.yaml
kubectl rollout status deployment simple-spinapp --timeout 90s
kubectl get pods -A
kubectl port-forward svc/simple-spinapp 8083:80 &
timeout 15s bash -c 'until curl -f -vvv http://localhost:8083/hello; do sleep 2; done'

- name: Verify curl
run: curl localhost:8083/hello
3 changes: 2 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ CRD_DIR := ./config/crd/bases

.PHONY: helm-generate
helm-generate: manifests kustomize helmify ## Create/update the Helm chart based on kustomize manifests. (Note: CRDs not included)
$(KUSTOMIZE) build config/default | $(HELMIFY) -crd-dir -cert-manager-as-subchart -cert-manager-version v1.13.3 charts/$(CHART_NAME)
$(KUSTOMIZE) build config/default | $(HELMIFY) -crd-dir charts/$(CHART_NAME)
rm -rf charts/$(CHART_NAME)/crds
@# Copy the containerd-shim-spin SpinAppExecutor yaml from its canonical location into the chart
cp config/samples/shim-executor.yaml charts/$(CHART_NAME)/templates/containerd-shim-spin-executor.yaml
Expand Down Expand Up @@ -216,6 +216,7 @@ helm-install: helm-generate ## Install the Helm chart onto the K8s cluster speci
$(HELM) upgrade --install \
-n $(HELM_NAMESPACE) \
--create-namespace \
--wait \
--set controllerManager.manager.image.repository=$(IMG_REPO) \
--set controllerManager.manager.image.tag=$(IMG_TAG) \
$(HELM_RELEASE) charts/$(CHART_NAME)
Expand Down
7 changes: 2 additions & 5 deletions charts/spin-operator/Chart.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,5 @@ dependencies:
- name: kwasm-operator
repository: http://kwasm.sh/kwasm-operator
version: 0.2.3
- name: cert-manager
repository: https://charts.jetstack.io
version: v1.13.3
digest: sha256:656e3b0c5aadd5694ec9271cb9d257a619ab0ce6453466f61de060346144dd91
generated: "2024-01-25T13:44:43.972708-07:00"
digest: sha256:40d405e4fdf2625d9b164f29f5d777cf6598ed74f155c4cf4259914128318cbd
generated: "2024-02-20T21:28:07.788307+05:30"
5 changes: 0 additions & 5 deletions charts/spin-operator/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,3 @@ dependencies:
version: "0.2.3"
repository: "http://kwasm.sh/kwasm-operator"

- name: cert-manager
repository: https://charts.jetstack.io
condition: certmanager.enabled
alias: certmanager
version: "v1.13.3"
11 changes: 5 additions & 6 deletions charts/spin-operator/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,16 @@ Prior to installing the chart, you'll need to ensure the following:
kubectl apply -f https://github.com/spinkube/spin-operator/releases/download/v0.1.0-rc.1/spin-operator.runtime-class.yaml
```

## Chart prerequisites

- [Cert Manager](https://github.com/cert-manager/cert-manager) to automatically provision and manage TLS certificates (used by spin-operator's admission webhook system). Cert Manager must be running and the corresponding CRDs must be present on the cluster before installing the spin-operator chart.

## Chart dependencies

The spin-operator chart currently includes the following sub-charts:

- [Kwasm Operator](https://github.com/kwasm/kwasm-operator) to install WebAssembly support on Kubernetes nodes
- [Cert Manager](https://github.com/cert-manager/cert-manager) to automatically provision and manage TLS certificates (used by spin-operator's admission webhook system)
- If you'd like to manage Cert Manager completely separate from spin-operator, you can disable installation via:
`--set certmanager.enabled=false` on `helm install`.
- Or, if you'd like to install Cert Manager separate from its CRDs, you can opt-out of installing the CRDs via:
`--set certmanager.installCRDs=false` on `helm install`.
- In either case, Cert Manager must be running and the corresponding CRDs must be present on the cluster before installing the spin-operator chart.


## Installing the chart

Expand Down
4 changes: 4 additions & 0 deletions charts/spin-operator/templates/containerd-shim-spin-executor.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ apiVersion: core.spinoperator.dev/v1
kind: SpinAppExecutor
metadata:
name: containerd-shim-spin
namespace: default
Copy link
Contributor

@vdice vdice Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We either need to add these same annotations to the canonical source (https://github.com/spinkube/spin-operator/blob/main/config/samples/shim-executor.yaml) or perhaps remove this logic and maintain a separate, chart-specific version. (Otherwise, these changes are lost on the next chart (re-)generation by helmify.)

Copy link
Contributor

@vdice vdice Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like if I don't add --wait to the helm install command, I get the timing error:

helm upgrade --install \
		-n spin-operator \
		--create-namespace \
		--set controllerManager.manager.image.repository=vdice/spin-operator \
		--set controllerManager.manager.image.tag=latest \
		spin-operator charts/spin-operator
Release "spin-operator" does not exist. Installing it now.
Error: failed post-install: warning: Hook post-install spin-operator/templates/containerd-shim-spin-executor.yaml failed: 1 error occurred:
	* Internal error occurred: failed calling webhook "mspinappexecutor.kb.io": failed to call webhook: Post "https://spin-operator-webhook-service.spin-operator.svc:443/mutate-core-spinoperator-dev-v1-spinappexecutor?timeout=10s": no endpoints available for service "spin-operator-webhook-service"


make: *** [helm-install] Error 1

Do we ensure docs/CI/etc all use --wait to avoid this error? Or do we look at not including this CR in the helm chart and rather documenting how it needs to be installed in any namespace that users wish to run SpinApps? (Well, we should doc that behavior anyways... looks like this is being tracked by #55)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: install->uninstall->install, currently the SpinAppExecutor CR is stuck terminating when deleted (currently tracked in #7 (comment)) so the subsequent install fails with:

Release "spin-operator" does not exist. Installing it now.
Error: failed post-install: warning: Hook post-install spin-operator/templates/containerd-shim-spin-executor.yaml failed: 1 error occurred:
	* object is being deleted: spinappexecutors.core.spinoperator.dev "containerd-shim-spin" already exists

I guess this is a heads up for now? i.e. should resolve once this CR is reliably removed...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, --wait is indeed required if we want the post-install hook of setting up CR succeed. My suggestion would be to add --wait and also make it clear using doc that CR is required in all namespaces where we wish to install spinapps.

one other way could be to make available a helm chart variable "spin-app-namespaces", we can then iterate over that list and install CR automatically during helm release installation. wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We either need to add these same annotations to the canonical source (https://github.com/spinkube/spin-operator/blob/main/config/samples/shim-executor.yaml) or perhaps remove this logic and maintain a separate, chart-specific version. (Otherwise, these changes are lost on the next chart (re-)generation by helmify.)

👍 thank you for catching this. Made the change to the canonical source.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one other way could be to make available a helm chart variable "spin-app-namespaces", we can then iterate over that list and install CR automatically during helm release installation. wdyt?

That's a creative idea to explore in the future!

annotations:
"helm.sh/hook": post-install,post-upgrade
"helm.sh/hook-weight": "1"
spec:
createDeployment: true
deploymentConfig:
Expand Down
1 change: 1 addition & 0 deletions charts/spin-operator/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ spec:
value: {{ quote .Values.kubernetesClusterDomain }}
image: {{ .Values.controllerManager.manager.image.repository }}:{{ .Values.controllerManager.manager.image.tag
| default .Chart.AppVersion }}
imagePullPolicy: {{ .Values.controllerManager.manager.imagePullPolicy }}
livenessProbe:
httpGet:
path: /healthz
Expand Down
3 changes: 0 additions & 3 deletions charts/spin-operator/templates/selfsigned-issuer.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
name: {{ include "spin-operator.fullname" . }}-selfsigned-issuer
annotations:
"helm.sh/hook": post-install,post-upgrade
"helm.sh/hook-weight": "1"
labels:
{{- include "spin-operator.labels" . | nindent 4 }}
spec:
Expand Down
3 changes: 0 additions & 3 deletions charts/spin-operator/templates/serving-cert.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: {{ include "spin-operator.fullname" . }}-serving-cert
annotations:
"helm.sh/hook": post-install,post-upgrade
"helm.sh/hook-weight": "2"
labels:
{{- include "spin-operator.labels" . | nindent 4 }}
spec:
Expand Down
4 changes: 1 addition & 3 deletions charts/spin-operator/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
certmanager:
enabled: true
installCRDs: true
controllerManager:
kubeRbacProxy:
args:
Expand Down Expand Up @@ -37,6 +34,7 @@ controllerManager:
image:
repository: ghcr.io/spinkube/spin-operator
tag: latest
imagePullPolicy: IfNotPresent
resources:
limits:
cpu: 500m
Expand Down
1 change: 1 addition & 0 deletions config/manager/manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ spec:
- --enable-webhooks
image: ghcr.io/spinkube/spin-operator:latest
name: manager
imagePullPolicy: IfNotPresent
securityContext:
allowPrivilegeEscalation: false
capabilities:
Expand Down
4 changes: 4 additions & 0 deletions config/samples/shim-executor.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ apiVersion: core.spinoperator.dev/v1
kind: SpinAppExecutor
metadata:
name: containerd-shim-spin
namespace: default
annotations:
"helm.sh/hook": post-install,post-upgrade
"helm.sh/hook-weight": "1"
spec:
createDeployment: true
deploymentConfig:
Expand Down
Loading