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

Add stack persistent volume claim #72

Closed
wants to merge 5 commits into from
Closed

Add stack persistent volume claim #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1b92d4c
Add stack persistent volume claim
pmorillon Oct 25, 2023
4b88eaf
Update stack helm chart after review on PR #72
pmorillon Oct 27, 2023
90c539d
Update stack helm chart after review on PR #72
pmorillon Oct 27, 2023
206af63
Update stack helm chart after review on PR tinkerbell#72
pmorillon Dec 7, 2023
92dd6a9
Fix storageClassName
pmorillon Dec 7, 2023
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
Empty file added tinkerbell/stack/templates/NOTES.txt
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

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

Empty file.
14 changes: 14 additions & 0 deletions tinkerbell/stack/templates/_hook.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{{- define "stack.hookDownloadJobEnabled" -}}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{{- $result := false }}
{{- if and .Values.stack.hook.enabled .Values.stack.hook.persistence.enabled -}}
{{- $result = true -}}
{{- if and .Values.stack.hook.persistence.enabled (eq .Values.stack.hook.persistence.type "pvc") }}
{{- range .Values.stack.hook.persistence.pvc.accessModes }}
{{- if eq . "ReadWriteOnce" }}
{{- $result = false}}
{{- end }}
{{- end }}
{{- end }}
{{- end -}}
{{- $result }}
{{- end -}}
14 changes: 13 additions & 1 deletion tinkerbell/stack/templates/hook.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{{- if .Values.stack.hook.enabled }}
{{- $hookDownloadJobEnabled := eq (include "stack.hookDownloadJobEnabled" .) "true" -}}
Copy link
Member

Choose a reason for hiding this comment

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

is this comparison redundant?

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

{{- if and .Values.stack.hook.enabled $hookDownloadJobEnabled }}
Copy link
Member

Choose a reason for hiding this comment

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

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

---
apiVersion: v1
kind: ConfigMap
Expand Down Expand Up @@ -48,9 +49,20 @@ spec:
restartPolicy: OnFailure
volumes:
- name: hook-artifacts
{{- if .Values.stack.hook.persistence.enabled }}
{{- if eq .Values.stack.hook.persistence.type "hostpath" }}
hostPath:
path: {{ .Values.stack.hook.downloadsDest }}
type: DirectoryOrCreate
{{- else if eq .Values.stack.hook.persistence.type "pvc" }}
persistentVolumeClaim:
claimName: {{ .Values.stack.hook.persistence.pvc.existingClaim | default "hook-artifacts" }}
{{- else }}
{{- fail "value for .Values.stack.hook.persistence.type is unsupported" }}
{{- end }}
{{- else }}
emptyDir: {}
{{- end }}
- name: configmap-volume
configMap:
defaultMode: 0700
Expand Down
11 changes: 11 additions & 0 deletions tinkerbell/stack/templates/nginx.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,20 @@ spec:
path: nginx.conf.template
{{- if .Values.stack.hook.enabled }}
- name: hook-artifacts
{{- if .Values.stack.hook.persistence.enabled }}
{{- if eq .Values.stack.hook.persistence.type "hostpath" }}
hostPath:
path: {{ .Values.stack.hook.downloadsDest }}
type: DirectoryOrCreate
{{- else if eq .Values.stack.hook.persistence.type "pvc" }}
persistentVolumeClaim:
claimName: {{ .Values.stack.hook.persistence.pvc.existingClaim | default "hook-artifacts" }}
{{- else }}
{{- fail "value for .Values.stack.hook.persistence.type is not as expected" }}
{{- end }}
{{- else }}
emptyDir: {}
{{- end }}
{{- end }}
initContainers:
- name: relay-macvlan-interface
Expand Down
29 changes: 29 additions & 0 deletions tinkerbell/stack/templates/nginx_persistentvolumeclaim.yaml
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

mind renaming this file to nginx_pvc.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, why not

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{{- if and .Values.stack.hook.enabled .Values.stack.hook.persistence.enabled (not .Values.stack.hook.persistence.pvc.existingClaim) (eq .Values.stack.hook.persistence.type "pvc") }}
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
name: hook-artifacts
labels:
{{- with .Values.stack.hook.persistence.pvc.extraLabels }}
{{- toYaml . | nindent 4 }}
{{- end }}
{{- with .Values.stack.hook.persistence.pvc.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
spec:
accessModes:
{{- range .Values.stack.hook.persistence.pvc.accessModes }}
- {{ . | quote }}
{{- end }}
resources:
requests:
storage: {{ .Values.stack.hook.persistence.pvc.size | quote }}
{{- with .Values.stack.hook.persistence.pvc.storageClassName }}
storageClassName: {{ . }}
{{- end }}
{{- with .Values.stack.hook.persistence.pvc.selector }}
selector:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- end }}
32 changes: 32 additions & 0 deletions tinkerbell/stack/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ 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


hook:
enabled: true
name: hook-files
Expand All @@ -21,6 +23,36 @@ stack:
# downloadURL only works with the > 0.8.1 Hook release because
# previous Hook versions didn't provide a checksum file.
downloadURL: https://github.com/tinkerbell/hook/releases/download/latest
persistence:
enabled: true
# type must be either "hostpath" or "pvc"
# hostpath: only works on a single worker node cluster
# https://kubernetes.io/docs/concepts/storage/volumes/#hostpath
type: hostpath
Copy link
Member

@chrisdoherty4 chrisdoherty4 Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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: ""
      ...

Copy link
Member

@chrisdoherty4 chrisdoherty4 Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


pvc:
# PVC accessModes :
# If you include only ReadWriteMany access mode it will allow to download Hook artifacts,
# it will also allow to scale up stack deployment on multinode cluster.
# If ReadWriteOnce is included, you will need to download manually Hook artifacts into
# stack Pod (/usr/share/nginx/html/).
# https://kubernetes.io/docs/concepts/storage/persistent-volumes/#access-modes
accessModes:
- ReadWriteMany
size: 1Gi
# storageClassName: default
# 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: {}

# existingClaim:
extraLabels: {}

kubevip:
enabled: true
name: kube-vip
Expand Down