[stable] Add IPFS Chart#1192
Conversation
- Uses persistent volumes for the IPFS Path, so we retain objects across restarts. - Uses a StatefulSet rather than Deployment, since we want a PVC for each pod running. Using a StatefulSet lets us scale up and down without sacrificing a persistent cache.
|
Hi @yuvipanda. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| runAsUser: 1000 | ||
| containers: | ||
| - name: {{ .Chart.Name }} | ||
| image: jbenet/go-ipfs:release |
There was a problem hiding this comment.
Probably want to use ipfs/go-ipfs instead of from Juan's account, and also lock it to a version, v0.4.9 is latest available in Docker Hub.
There was a problem hiding this comment.
Good catch! Fixed it :)
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: {{ if .Values.service.nameOverride }} {{ .Values.service.nameOverride }} {{ else }} {{ template "fullname" . }} {{ end }} |
There was a problem hiding this comment.
Why would you want to do that? This should just be {{ template "fullname" . }}.
There was a problem hiding this comment.
Because I want to be allow users to override service names. In this case, end users will be asked to hit the DNS entry for the service name, and that should be overrideable so it can be something like 'ipfs' so other users in the same namespace can just specify 'ipfs'. The default is still the behavior seen in the current charts in the repo.
| apiVersion: apps/v1beta1 | ||
| kind: StatefulSet | ||
| metadata: | ||
| name: {{ template "name" . }} |
There was a problem hiding this comment.
Name should be {{ template "fullname" . }}.
| template: | ||
| metadata: | ||
| labels: | ||
| app: {{ template "fullname" . }} |
There was a problem hiding this comment.
Use this {{ template "name" . }} for the app label.
| labels: | ||
| app: {{ template "fullname" . }} | ||
| spec: | ||
| securityContext: |
There was a problem hiding this comment.
Can you explain the reason behind this?
There was a problem hiding this comment.
Yes - the image needs to write to the PVC to function, and it runs as UID 1000 (rather than root).
| runAsUser: 1000 | ||
| containers: | ||
| - name: {{ .Chart.Name }} | ||
| image: ipfs/go-ipfs:v0.4.9 |
| nameOverride: null | ||
|
|
||
| resources: | ||
| requests: |
There was a problem hiding this comment.
Don't specify defaults for resources.
| metadata: | ||
| name: {{ if .Values.service.nameOverride }} {{ .Values.service.nameOverride }} {{ else }} {{ template "fullname" . }} {{ end }} | ||
| labels: | ||
| chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}" |
There was a problem hiding this comment.
Use standard labels everywhere. See:
https://github.com/kubernetes/helm/blob/master/docs/chart_best_practices/labels.md
There was a problem hiding this comment.
I just got this from helm create, will read and update!
| spec: | ||
| replicas: {{ .Values.replicaCount }} | ||
| serviceName: {{ template "name" . }} | ||
| volumeClaimTemplates: |
There was a problem hiding this comment.
Could you make this more flexible and make persistence optional?
See e. g.: https://github.com/kubernetes/charts/blob/master/stable/mongodb-replicaset/templates/mongodb-statefulset.yaml#L175-L199
There was a problem hiding this comment.
I could if it is a requirement for merging :) but it's not particularly useful without persistent volumes however - but I guess so is the case for MongoDB!
There was a problem hiding this comment.
Done! Optional but enabled by default!
|
|
||
| service: | ||
| type: ClusterIP | ||
| nameOverride: null |
| targetPort: 8080 | ||
| name: gateway | ||
| selector: | ||
| app: {{ template "name" . }} |
There was a problem hiding this comment.
Requires an additional selector: release: {{ .Release.Name }}
| template: | ||
| metadata: | ||
| labels: | ||
| app: {{ template "name" . }} |
There was a problem hiding this comment.
At least the release label should be added here as well.
| volumeMounts: | ||
| - name: ipfs-storage | ||
| mountPath: /data/ipfs | ||
| {{- if .Values.resources }} |
There was a problem hiding this comment.
if not necessary. Add default to values.yaml:
resources: {}
|
|
||
| service: | ||
| type: ClusterIP | ||
| nameOverride: null |
| spec: | ||
| securityContext: | ||
| # The image runs as uid 1000 by default, and needs to be able to write to | ||
| # the persistent volume to be able to start. |
There was a problem hiding this comment.
I don't think this is necessary. Will test this. Otherwise we'd have this problem with lots of charts. I think this is only a problem if you mount a volume from the host, which is not the case.
There was a problem hiding this comment.
I ran into permissionDenied errors when I tried (on GKE). Perhaps the runAsUser isn't necessary but fs is?
| could just connect to it by specifying 'ipfs'. | ||
| */}} | ||
| {{- define "servicename" -}} | ||
| {{- if .Values.service.nameOverride -}} |
There was a problem hiding this comment.
On second thought, am not sure this needs truncation. The user is responsible for setting it, and the default fullname handles truncation already.
| | `replicaCount` | The number of replicas of go-ipfs to run | 1 | | ||
| | `service.type` | Type of the service: `ClusterIP`, `LoadBalancer` or `NodePort` | `ClusterIP` | | ||
| | `service.nameOverride` | The name to use for the service | The full name of the release | | ||
| | `storage.size` | Size of the PVC for each IPFS pod, used as persistent cache | `10Gi` | |
| heritage: {{ .Release.Service }} | ||
| release: {{ .Release.Name }} | ||
| spec: | ||
| type: {{ .Values.service.type }} |
There was a problem hiding this comment.
Statefulsets require a headless service. This means you should always have this:
type: ClusterIP
clusterIP: NoneThere was a problem hiding this comment.
Does the service need to be headless? We also need a service anyway, and this paradigm works well - ipfs.namespace round robins to one of them, and you can do ipfs-0.ipfs or ipfs-1.ipfs for each one if you need.
|
@k8s-bot ok to test |
|
Anything else I need to do before I can get it merged? :) |
unguiculus
left a comment
There was a problem hiding this comment.
I deployed it on GKE. Everything seems to come up successfully. The log looks ok. When I access it on port 8080 with port-forwarding, I get a 404.
| {{- else if contains "ClusterIP" .Values.service.type }} | ||
| export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app={{ template "fullname" . }}" -o jsonpath="{.items[0].metadata.name}") | ||
| echo "Visit http://127.0.0.1:8080 to use your application" | ||
| kubectl port-forward $POD_NAME 8080:{{ .Values.service.externalPort }} |
There was a problem hiding this comment.
service.externalPort is not used anywhere. Thus, a default installation prints this:
If you want to connect to it from your local computer, you can find a URL to connect with the
following (for the gateway service):
export POD_NAME=$(kubectl get pods --namespace default -l "app=wiggly-alpaca-ipfs" -o jsonpath="{.items[0].metadata.name}")
echo "Visit http://127.0.0.1:8080 to use your application"
kubectl port-forward $POD_NAME 8080:
Also note that the label selector is wrong. Use this: -l "app={{ template "name" . }},release={{ .Release.Name }}"
|
Marking as stale. Please update within one week. |
|
Whoops, shall do!
On Jul 2, 2017 11:06 AM, "Reinhard Nägele" <notifications@github.com> wrote:
Marking as stale. Please update within one week.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1192 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAB23v0WR0qhC1W-eEsAtaXrd2EI7Z81ks5sJ9wdgaJpZM4Npi2Z>
.
|
|
The 404 is expected, since it's only an API Gateway service. If you access, for example, /ipfs/Qmb8wsGZNXt5VXZh1pEmYynjB6Euqpq3HYyeAdw2vScTkQ, it'll lead you to the same page as https://ipfs.io/ipfs/Qmb8wsGZNXt5VXZh1pEmYynjB6Euqpq3HYyeAdw2vScTkQ. It can also serve a particular hash based on a Host Header. For example, if you do a |
|
Two things about the
Would you mind creating another PR for this? |
* upstream/master: Add IPFS Chart (helm#1192) [stable/ghost] Release 0.4.12 (helm#1429) [stable/opencart] Release 0.4.10 (helm#1425) [stable/redmine] Release 1.2.0 (helm#1414) [stable/wordpress] Release 0.6.8 (helm#1403) [stable/rabbitmq] Release 0.5.4 (helm#1395) [stable/mongodb] Release 0.4.12 (helm#1392) [stable/phabricator] Release 0.4.11 (helm#1384) [stable/drupal] Release 0.8.1 (helm#1340) [stable/odoo] Release 0.5.3 (helm#1323) update pilot svc name to default for nicer UX (helm#1464) [stable/grafana] Improve curl invocation (helm#1463)
|
@unguiculus I sent #1469. Thanks! |
Adds a chart that lets you install IPFS with a StatefulSet.