Adding kubernetes backend to stolon-chart by Flowkap · Pull Request #20 · lwolf/stolon-chart

Flowkap · 2018-03-29T10:01:40Z

Details can be found in issue #18

… sentinel (why?)

lwolf

Great job, I left few minor comments inline to make sure we don't forget to cleanup.

During testing on my cluster I noticed one weird thing happening to stolon pods when I use kubernetes backend. If I run kubectl get pods -w output constantly getting updated.
https://gist.github.com/lwolf/2981232c2ccaa87e3d15681bcc425fe0
It does not happen if I use etcd as backend on the same cluster. Do you observe the same thing?

lwolf · 2018-03-30T15:32:31Z

+### Experimental kubernetes backend:
+
+```bash
+$ git clone https://github.com/lwolf/stolon-chart


I think you can remove this duplicate installation steps, except for the helm install one.

lwolf · 2018-03-30T15:34:06Z

 | `clusterName`                           | Name of the cluster                            | `kube-stolon`                                                |
 | `debug`                                 | Debug mode                                     | `false`                                                      |
-| `store.backend`                         | Store backend to use (etcd/consul)             | `etcd`                                                       |
+| `store.backend`                         | Store backend to use (etcd/consul/kubernetes)  | `etcd`                                                       |


please add second option - kubeResourceKind, with default and only possible - configmap

lwolf · 2018-03-30T15:35:58Z

          - --cluster-name={{ template "stolon.clusterName" . }}
          - --store-backend={{ .Values.store.backend }}
+          {{- if eq .Values.store.backend "kubernetes" }}
+          - --kube-resource-kind={{ .Values.store.kubeRessourceKind }}


could you please change it to have configmap as default value, with possibility to override from .Values.store.kubeRessourceKind
i.e.
- --kube-resource-kind={{ default "configmap" .Values.store.kubeRessourceKind }}

lwolf · 2018-03-30T15:37:19Z

 ## ref: https://hub.docker.com/r/sorintlab/stolon/tags/
 ##
 imageTag: "v0.9.0-pg9.6"
+##


v.0.10 is released now, please set it as default, and remove following comments.

lwolf · 2018-03-30T15:37:54Z

            value: {{ .Values.store.backend | quote}}
+          {{- if eq .Values.store.backend "kubernetes" }}
+          - name: STSENTINEL_KUBE_RESOURCE_KIND
+            value: {{ .Values.store.kubeRessourceKind  | quote}}


please add default value here as well

lwolf · 2018-03-30T15:38:16Z

            value: {{ .Values.store.backend | quote}}
+          {{- if eq .Values.store.backend "kubernetes" }}
+          - name: STPROXY_KUBE_RESOURCE_KIND
+            value: {{ .Values.store.kubeRessourceKind  | quote}}


please add default value here as well

lwolf · 2018-03-30T15:38:28Z

            value: {{ .Values.store.backend  | quote}}
+          {{- if eq .Values.store.backend "kubernetes" }}
+          - name: STKEEPER_KUBE_RESOURCE_KIND
+            value: {{ .Values.store.kubeRessourceKind  | quote}}


please add default value here as well

lwolf · 2018-03-30T15:40:21Z

  ## store endpoints MUST be set!
  endpoints: "http://etcd-etcd-0.etcd-etcd:2379,http://etcd-etcd-1.etcd-etcd:2379,http://etcd-etcd-2.etcd-etcd:2379"

+  ## kubernetes You also need the new imageTag! (should work, use at own risk, new feature as of https://github.com/sorintlab/stolon/pull/433)


this entire block could be removed since we already have backend field on top and kubeResourceKind will have default value.

lwolf · 2018-03-30T15:43:18Z

  selector:
+    # Due to https://github.com/sorintlab/stolon/pull/433/commits/38ae6b13b5e161a5bfe0fbe01084ca060eaf2e76#diff-95cdd374e9440fde010ff35d65f8cf3fR54
+    {{- if eq .Values.store.backend "kubernetes" }}
+    app: "stolon-keeper"


With current condition we could not have 2 stolon clusters deployed in the same namespace with kubernetes backend.
Let's change labelling to be unified.

selector: app=stolon-keeper component: keeper release: "{{ .Release.Name }}"

and we should remove if-else in all deployments/daemonsets.

I did deploy two clusters in same namespace for K8. why'd you think this does not work?
Anyway, this could be a good approach to remove cluttered if statements. I introduced them in the first place in order to not change existing behaviour.

Also stolon-cluster label is necessary on specs as well in order for the service discovery to work. I also filed sorintlab/stolon#466

I'll change it according to your proposal. Might also be a solution to avoid the matching problematics.

Also I think I'll remove obsolete labels to remove unnecessary double lables. If applicable.

Without suggested rework of labels, services in both clusters will endup balancing traffic between all keepers (If I don't miss anything):

selector: app: stolon-keeper component: keeper

https://github.com/lwolf/stolon-chart/blob/master/stolon/templates/keeper-ro-service.yaml#L18-L20

lwolf · 2018-03-30T15:50:55Z


+  ## kubernetes You also need the new imageTag! (should work, use at own risk, new feature as of https://github.com/sorintlab/stolon/pull/433)
+  # backend: "kubernetes"
+  # kubeRessourceKind: "configmap"


it seems that you have typo in kubeRessourceKind which is copypasted everywhere.
kubeRessourceKind => kubeResourceKind

lwolf · 2018-03-30T16:11:51Z

Just made a diff of the same resource after few seconds.
resourceVersion and metadata.annotations.stolon-status are constantly getting change.

lwolf · 2018-03-30T16:19:00Z

I can confirm that the issue is present is original examples of stolon, so I'm going to create issue about it there.

greenrd · 2018-04-01T11:39:47Z

          - name: STKEEPER_PG_SU_PASSWORDFILE
            value: "/etc/secrets/stolon/pg_su_password"
-          - name: STSENTINEL_DEBUG
+          - name: STKEPPER_DEBUG


Typo: Should be STKEEPER_DEBUG

Fixed typo and added a typo... Fail ... :D

Flowkap · 2018-04-03T07:17:27Z

I can confirm the continuous change of the resources as well. I'll later go through all your valid comments and try out if the labeling is fixed (seems to be at least in master)

Flowkap · 2018-04-03T11:04:05Z

Updated the PR

lwolf · 2018-04-03T17:54:20Z

    release: "{{ .Release.Name }}"
+    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
    heritage: "{{ .Release.Service }}"
-    component: keeper


Please, keep component label in place, just set it to the same value as current app.

Hopefully your RFE will be addressed inside the stolon repo, and we could reverse app label later. Essentially it is more logical to have app label corresponding to the fullname and component to, well, component.

Why should it be there? It's only useful on pods right?

I intentionally removed component + stolon-cluster labels from all spec excluding pods, as well as I removed chart and heritage from pods in order to not have all labels doubled in the chart templates.

Maybe I'm missing something

If this is not intended I add component and the stolon-cluster label to all labels sections again.

lwolf · 2018-04-03T17:55:47Z

-    app: {{ template "stolon.keeper.fullname" . }}
+    app: "stolon-keeper"
+    release: "{{ .Release.Name }}"
    component: keeper


you did not delete component from label selector, but did delete it from the labels in keeper.

Component is still in place in all pod specs which are used for selection. I only deleted the cluster-name as well as component from the StatefulSets and according resources. K8 service discovery works fine for keepers as is.

lwolf · 2018-04-03T17:57:23Z

  backend: "etcdv3"
  ## store endpoints MUST be set!
  endpoints: "http://etcd-etcd-0.etcd-etcd:2379,http://etcd-etcd-1.etcd-etcd:2379,http://etcd-etcd-2.etcd-etcd:2379"
+  ## for kubernetes backend (experimental)


Please remove this comments

Flowkap · 2018-04-04T07:49:39Z

I added the RBAC stuff as well. I left the two open points in regard to the labels open and wait for your response first.

lwolf

left one comment about labels, but I'll take another look tomorrow

lwolf · 2018-04-04T21:25:29Z

@@ -4,24 +4,21 @@ metadata:
  name: {{ template "stolon.sentinel.fullname" . }}
  labels:
    app: {{ template "stolon.sentinel.fullname" . }}


what is the reasoning behind having half of the components with hardcoded app label and half with templated as before?

lwolf

I put all of the labels in the google-doc:
https://docs.google.com/spreadsheets/d/1pXjzZEElWs6Dib7qrxGdHHF_61OOhiCKYlTWVhvi4mw/edit?usp=sharing

I see your point about redundant labels on pods, and I'm fine with removing heritage from pods. But I still prefer to have component and chart labels everywhere. This labels are useful for manual selection.

lwolf · 2018-04-05T18:00:48Z

  labels:
-    app: {{ template "stolon.keeper.fullname" . }}
-    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+    app: "stolon-keeper"


I suppose this should be app: {{ template "stolon.keeper.fullname" . }}

lwolf · 2018-04-05T18:02:01Z

  labels:
-    app: {{ template "stolon.keeper.fullname" . }}
-    chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+    app: "stolon-keeper"


I suppose this should be app: {{ template "stolon.keeper.fullname" . }}

Flowkap · 2018-04-09T12:50:20Z

Sorry I was sick the last few days.

I see your point about redundant labels on pods, and I'm fine with removing heritage from pods. But I still prefer to have component and chart labels everywhere. This labels are useful for manual selection.

Ok see that as well. Will change it as well as fix the two wrong labels you mentioned above :)

Flowkap · 2018-04-09T13:17:46Z

Shall we wait for sorintlab/stolon#466 to resolve until merge or change it when it's finished (don't know when this could be part of a next release)

We anyway already use my fork for internal testing as our system is under heavy development still. I would'nt mind.

lwolf · 2018-04-09T18:16:47Z

Let's release it as is, and update labels if and when that issue is resolved.
Thank you for the work on this.

Florian Kappes added 2 commits March 5, 2018 15:16

First implementation of backend kubernetes, keeper not registering on…

062ba7c

… sentinel (why?)

Fixes lwolf#18

33f4ff3

lwolf reviewed Mar 30, 2018

View reviewed changes

greenrd reviewed Apr 1, 2018

View reviewed changes

Review enhancements for PR

766098d

lwolf reviewed Apr 3, 2018

View reviewed changes

Florian Kappes added 2 commits April 4, 2018 09:39

Added rbac roles

1ec5fe4

Removed comments

e7a2bca

lwolf reviewed Apr 4, 2018

View reviewed changes

lwolf reviewed Apr 5, 2018

View reviewed changes

Florian Kappes added 2 commits April 9, 2018 14:54

Fix erroneous labels

32a766c

Added extra chart and component labels as of discussion in PR

919548e

lwolf merged commit d42a1c5 into lwolf:master Apr 9, 2018

Conversation

Flowkap commented Mar 29, 2018

Uh oh!

lwolf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flowkap Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwolf commented Mar 30, 2018

Uh oh!

lwolf commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flowkap commented Apr 3, 2018

Uh oh!

Flowkap commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flowkap Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flowkap commented Apr 4, 2018

Uh oh!

lwolf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwolf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flowkap Apr 3, 2018 •

edited

Loading

Flowkap Apr 4, 2018 •

edited

Loading