Add stack persistent volume claim #72

pmorillon · 2023-10-25T16:04:01Z

Description

A proposal to fix #42 like PR #63, taking into account the new method to download Hook artifacts with a Job (previously with an init container). PVC accessModes are checked to disable hook downloader Job and inform user when ReadWriteOnce is used (default to ReadWriteMany).

Why is this needed

Fixes: #42

How Has This Been Tested?

Only tested with ReadWriteOnce PVC.
Checked generated manifests with helm install --debug --dry-run...

How are existing users impacted? What migration steps/scripts do we need?

Actual default behavior should be preserved

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock · 2023-10-26T22:24:10Z

tinkerbell/stack/templates/NOTES.txt

+
+You are using no persistence or ReadWriteOnce access modes on PersistentVolumeClaim, Job `download-hook` is disabled.
+You will need to manage hooks artifacts manually.
+Also stack Deployment is not scalable.


Hey @pmorillon, thanks for this PR. What does this mean, the stack deployment is not scalable? is this only when the HookOS download job is false? or in general?

I will remove this confusing note and add comment into values file and update _helper to apply this behavior :

stack.hook.enabled to false : no download job, no PVC, no hook-artifacts Volume

stack.hook.enabled to true (default) :

stack.hook.persistence.enabled to false : volume is an empty dir, so no download job, no PVC

stack.hook.persistence.enabled to true (default) :

stack.hook.persistence.type = hostpath (default) : volume is an hostpath, download job enabled, no PVC. Only works with a one worker node cluster for hook downloader and scale up.

stack.hook.persistence.type = pvc : volume is based on a PVC

stack.hook.persistent.accessModes contains ReadWriteMany (default) : download job enabled. Will works on multinode cluster and scale up is possible.

stack.hook.persistent.accessModes contains ReadWriteOnce : download job is disabled (job won't be able to mount already mounted volume), scale up will not be possible.

jacobweinstock · 2023-10-26T22:25:24Z

tinkerbell/stack/templates/hook.yaml

@@ -1,4 +1,5 @@
-{{- if .Values.stack.hook.enabled }}
+{{- $hookDownloadJobEnabled := eq (include "stack.hookDownloadJobEnabled" .) "true" -}}
+{{- if and .Values.stack.hook.enabled $hookDownloadJobEnabled }}


Its hard to tell where the corresponding {{- end }} is for this. mind putting the corresponding{{- end }} at the same level indent?

jacobweinstock · 2023-10-26T22:28:04Z

tinkerbell/stack/values.yaml

@@ -12,6 +12,20 @@ stack:
  lbClass: kube-vip.io/kube-vip-class
  # Once the Kubernetes Gateway API is more stable, we will use that for all services instead of nginx.
  image: nginx:1.25.1
+  persistence:


if i understand correctly, persistence is only related to the HookOS download? if that is the case, would this block be more ideal under hook:?

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock · 2023-11-02T15:01:13Z

Hey @pmorillon , thanks for this. Looks good. Just going to run a few test scenarios locally and then we'll merge it.

chrisdoherty4 · 2023-11-02T15:39:02Z

tinkerbell/stack/templates/_helpers.tpl

Can we rename this file _hook.tpl.

chrisdoherty4 · 2023-11-02T15:40:14Z

tinkerbell/stack/templates/hook.yaml

+          persistentVolumeClaim:
+            claimName: {{ .Values.stack.hook.persistence.existingClaim | default "hook-artifacts" }}
+            {{- else }}
+              {{- fail "value for .Values.stack.hook.persistence.type is not as expected" }}


Suggested change

{{- fail "value for .Values.stack.hook.persistence.type is not as expected" }}

{{- fail "value for .Values.stack.hook.persistence.type is unsupported" }}

chrisdoherty4 · 2023-11-02T15:45:02Z

tinkerbell/stack/values.yaml

+      ## type hostpath or pvc
+      # hostpath : only works on a single worker node cluster
+      type: hostpath
+      ## When type is pvc
+      # storageClassName: default


Suggested change

## type hostpath or pvc

# hostpath : only works on a single worker node cluster

type: hostpath

## When type is pvc

# storageClassName: default

# The storage type to use for persisting HookOS artifacts. The hostpath

# option can only be used with single worker node clusters.

# Options: hostpath, pvc.

type: hostpath

# The PVC storage class to use. Only valid with type=pvc.

# storageClassName: default

I think it may also be useful to include links to Kubernetes documentation.

chrisdoherty4 · 2023-11-02T15:51:40Z

tinkerbell/stack/values.yaml

+      # storageClassName: default
+
+      ## PVC accessModes : 
+      # Require ReadWriteMany in order to download Hook and scale up kubernetes tink-stack Deployment resource.


This sentence has little context which makes it a somewhat confusing on what I should set. Specifically, I'm left wondering what ReadWriteMany has to do with scaling a tink-stack deployment and subsequently need to dive into the templates to try and understand.

Perhaps this could be worded as "If you include ReadWriteMany it will allow X to happen. This will accommodate scaling the Tink Stack because Y".

Also, what happens if multiple access mode are included?

chrisdoherty4 · 2023-11-02T16:00:43Z

tinkerbell/stack/values.yaml

+      # annotations: {}
+      # selectorLabels: {}


Suggested change

# annotations: {}

# selectorLabels: {}

# Annotations to include on the PVC object.

annotations: {}

# An optional selector to narrow the Volumes considered for binding to the PVC.

# Should be structured the same as a raw PVC selector (see documentation for more info).

#

# https://kubernetes.io/docs/concepts/storage/persistent-volumes/#selector.

selector: {}

I'm suggesting changing the selector so users can define the selector using expressions as well as labels.

chrisdoherty4 · 2023-11-06T13:02:30Z

tinkerbell/stack/values.yaml

+      enabled: true
+      ## type hostpath or pvc
+      # hostpath : only works on a single worker node cluster
+      type: hostpath


Sorry to throw a spanner in the works, but when I looked at this again it made me wonder if we'd be better using keys to represent the persistence type avoiding the 'only use this if using pvc' commentary about each of the items below.

E.g.

hook: persistence: hostpath: {} pvc: storageClassName: "" ...

The user should specify only 1. We would want a comment on persistence indicating how to use it.

@jacobweinstock Do you have a preference?

With your suggestion we could still use type: and then both hostpath: and pvc: could be populated without issue.

hook: persistence: # type must be either "hostpath" or "pvc" # hostpath: only works on a single worker node cluster type: hostpath hostpath: {} pvc: storageClassName: "" ...

I'm open to that, though I don't think its strictly necessary because we can test for the presence of either hostpath or pvc keys. I'm not sure which is clearer so I'm good either way.

That said, I don't think there would be a need for hostpath: {} at all because we don't expose options yet. We have a path forward if we do want to specify options though.

Signed-off-by: Pascal Morillon <[email protected]>

pmorillon · 2023-12-07T13:08:33Z

I took into account all your comments in the last commit

Signed-off-by: Pascal Morillon <[email protected]>

chrisdoherty4 · 2023-12-07T13:43:32Z

Thank you, this is awesome.

jacobweinstock

@pmorillon, thanks for this. Almost there. I left a few comments for your review. Thank you!

jacobweinstock · 2023-12-17T18:39:25Z

tinkerbell/stack/templates/_hook.tpl

@@ -0,0 +1,14 @@
+{{- define "stack.hookDownloadJobEnabled" -}}


can we get a code comment here about what this is and why? Reading the code is a little hard to follow and will require that I re-understand it every time I come back to it. Also, i don't know if I understand it in the first place.

Is this logic to determine if downloading HookOS should occur?
Does this logic say that if there is a pvc with accessMode of ReadWriteOnce then we won't download HookOS? if so, why is that?

Is this logic to determine if downloading HookOS should occur?

Yes

Does this logic say that if there is a pvc with accessMode of ReadWriteOnce then we won't download HookOS? if so, why is that?

Because you can't determine on which node will be scheduled the Kubernetes Job, so stack Deployment resource already mounting the PVC, the download job will fail to mount the PVC

jacobweinstock · 2023-12-17T18:41:42Z

tinkerbell/stack/templates/hook.yaml

@@ -1,4 +1,5 @@
-{{- if .Values.stack.hook.enabled }}
+{{- $hookDownloadJobEnabled := eq (include "stack.hookDownloadJobEnabled" .) "true" -}}


is this comparison redundant?

true == true is true
false == true is false

couldn't we just use include "stack.hookDownloadJobEnabled" directly?

jacobweinstock · 2023-12-17T18:42:56Z

tinkerbell/stack/templates/nginx_persistentvolumeclaim.yaml

mind renaming this file to nginx_pvc.yaml?

ok, why not

jacobweinstock · 2023-12-17T18:44:05Z

tinkerbell/stack/templates/NOTES.txt

curious if this empty file provides us something or if it can be removed.

jacobweinstock · 2023-12-17T18:47:04Z

Looks like this branch needs a rebase of main.

mddeff · 2024-05-07T03:56:52Z

@chrisdoherty4 @jacobweinstock

I came to this PR after following #42 and #63 while in search of a solution for hook that didnt use hostPath. It looks like we (and by we, I mean @pmorillon) were SO close to getting this merged.

If the final code-comment you requested is added and re-base complete, can we get this merged? Using hostPath could be considered a blocker to running Tinkerbell in many production environments.

I love what the Tinkerbell project is doing, awesome stuff!

Signed-off-by: Pascal Morillon <[email protected]>

Persistence: ## Description  Update Tink-stack to use a PVC for the HookOS artifacts download. This allows single node and multi-node Kubernetes cluster to work for HookOS artifact serving. What used to be a Kubernetes Job to download HookOS, is now a container in the tink-stack pod. This removes the challenges of getting a Kubernetes Job to download HookOS on the same node as the tink-stack pod. This is only an issue with the local persistent volume. ## Why is this needed  Fixes: #72 Fixes: #63 ## How Has This Been Tested?    ## How are existing users impacted? What migration steps/scripts do we need?   ## Checklist: I have: - [ ] updated the documentation and/or roadmap (if required) - [ ] added unit or e2e tests - [ ] provided instructions on how to upgrade

Add stack persistent volume claim

1b92d4c

Signed-off-by: Pascal Morillon <[email protected]>

pmorillon force-pushed the persistence branch from 97e9bf2 to 1b92d4c Compare October 25, 2023 16:06

jacobweinstock reviewed Oct 26, 2023

View reviewed changes

Update stack helm chart after review on PR tinkerbell#72

4b88eaf

Signed-off-by: Pascal Morillon <[email protected]>

pmorillon marked this pull request as draft October 27, 2023 08:17

Update stack helm chart after review on PR tinkerbell#72

90c539d

Signed-off-by: Pascal Morillon <[email protected]>

pmorillon marked this pull request as ready for review October 27, 2023 08:21

chrisdoherty4 reviewed Nov 2, 2023

View reviewed changes

tinkerbell/stack/templates/_helpers.tpl Outdated

Copy link

Member

chrisdoherty4 Nov 2, 2023

Choose a reason for hiding this comment

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

Can we rename this file _hook.tpl.

chrisdoherty4 reviewed Nov 2, 2023

View reviewed changes

chrisdoherty4 reviewed Nov 6, 2023

View reviewed changes

pmorillon added a commit to Eskemm-Numerique/tinkerbell-charts that referenced this pull request Dec 7, 2023

Update stack helm chart after review on PR tinkerbell#72

888654b

Update stack helm chart after review on PR tinkerbell#72

206af63

Signed-off-by: Pascal Morillon <[email protected]>

pmorillon force-pushed the persistence branch from 888654b to 206af63 Compare December 7, 2023 13:06

Fix storageClassName

92dd6a9

Signed-off-by: Pascal Morillon <[email protected]>

chrisdoherty4 requested a review from jacobweinstock December 7, 2023 13:43

chrisdoherty4 approved these changes Dec 7, 2023

View reviewed changes

jacobweinstock requested changes Dec 17, 2023

View reviewed changes

jacobweinstock pushed a commit to jacobweinstock/charts that referenced this pull request Oct 5, 2024

Update stack helm chart after review on PR tinkerbell#72

316b3ea

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock pushed a commit to jacobweinstock/charts that referenced this pull request Oct 5, 2024

Update stack helm chart after review on PR tinkerbell#72

c0a53ae

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock pushed a commit to jacobweinstock/charts that referenced this pull request Oct 5, 2024

Update stack helm chart after review on PR tinkerbell#72

acfdc91

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock mentioned this pull request Oct 5, 2024

Persistence #128

Merged

3 tasks

jacobweinstock pushed a commit to jacobweinstock/charts that referenced this pull request Oct 21, 2024

Update stack helm chart after review on PR tinkerbell#72

f59c10f

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock pushed a commit to jacobweinstock/charts that referenced this pull request Oct 21, 2024

Update stack helm chart after review on PR tinkerbell#72

258be32

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock pushed a commit to jacobweinstock/charts that referenced this pull request Oct 21, 2024

Update stack helm chart after review on PR tinkerbell#72

27c6bef

Signed-off-by: Pascal Morillon <[email protected]>

jacobweinstock closed this in #128 Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stack persistent volume claim #72

Add stack persistent volume claim #72

pmorillon commented Oct 25, 2023

jacobweinstock Oct 26, 2023

pmorillon Oct 27, 2023

jacobweinstock Oct 26, 2023

jacobweinstock Oct 26, 2023

jacobweinstock commented Nov 2, 2023

chrisdoherty4 Nov 2, 2023

chrisdoherty4 Nov 2, 2023

chrisdoherty4 Nov 2, 2023

chrisdoherty4 Nov 2, 2023

chrisdoherty4 Nov 2, 2023 •

edited

Loading

chrisdoherty4 Nov 2, 2023 •

edited

Loading

chrisdoherty4 Nov 6, 2023 •

edited

Loading

jacobweinstock Nov 7, 2023

chrisdoherty4 Nov 7, 2023 •

edited

Loading

pmorillon commented Dec 7, 2023

chrisdoherty4 commented Dec 7, 2023

jacobweinstock left a comment

jacobweinstock Dec 17, 2023

pmorillon Dec 18, 2023

jacobweinstock Dec 17, 2023

pmorillon Dec 18, 2023

jacobweinstock Dec 17, 2023

pmorillon Dec 18, 2023

jacobweinstock Dec 17, 2023

jacobweinstock commented Dec 17, 2023

mddeff commented May 7, 2024

	{{- fail "value for .Values.stack.hook.persistence.type is not as expected" }}
	{{- fail "value for .Values.stack.hook.persistence.type is unsupported" }}

-      # annotations: {}
-      # selectorLabels: {}
+      # Annotations to include on the PVC object.
+      annotations: {}
+      # An optional selector to narrow the Volumes considered for binding to the PVC.
+      # Should be structured the same as a raw PVC selector (see documentation for more info).
+      #
+      # https://kubernetes.io/docs/concepts/storage/persistent-volumes/#selector.
+      selector: {}

		@@ -0,0 +1,14 @@
		{{- define "stack.hookDownloadJobEnabled" -}}

		@@ -1,4 +1,5 @@
		{{- if .Values.stack.hook.enabled }}
		{{- $hookDownloadJobEnabled := eq (include "stack.hookDownloadJobEnabled" .) "true" -}}

Add stack persistent volume claim #72

Add stack persistent volume claim #72

Conversation

pmorillon commented Oct 25, 2023

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobweinstock commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

chrisdoherty4 Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

chrisdoherty4 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdoherty4 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

pmorillon commented Dec 7, 2023

chrisdoherty4 commented Dec 7, 2023

jacobweinstock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobweinstock commented Dec 17, 2023

mddeff commented May 7, 2024

chrisdoherty4 Nov 2, 2023 •

edited

Loading

chrisdoherty4 Nov 2, 2023 •

edited

Loading

chrisdoherty4 Nov 6, 2023 •

edited

Loading

chrisdoherty4 Nov 7, 2023 •

edited

Loading