Sidecar experimental #157

LimKianAn · 2021-03-05T20:35:11Z

@majst01 @eberlep
This branch shows the possibility of having the items in ConfigMap aligned with yaml manifests of target golang struct and then unmarshal them in the code so that we avoid setting the values manually. We might not need to merge this branch into main, but it serves as an example for further discussion.

LimKianAn · 2021-03-17T19:13:48Z

Ready for a merge if this direction is appreciated: Shift the logic of global sidecars configmap to helm yaml.

eberlep · 2021-03-18T13:58:49Z

charts/postgreslet/templates/configmap-sidecars.yaml

-  postgres-fluentbit-requests-memory: {{ .Values.sidecars.fluentbit.resources.requests.memory | quote }}
-  postgres-fluentbit-limits-cpu: {{ .Values.sidecars.fluentbit.resources.limits.cpu | quote }}
-  postgres-fluentbit-limits-memory: {{ .Values.sidecars.fluentbit.resources.limits.memory | quote }}
+  per-namespace-sidecars-configmap-name: {{ .Values.sidecars.perNamespaceConfigmapName | quote }}


Maybe move this into the operatormanager

~~We still need the variable here and here. By the linkage of per-namespace-sidecars-configmap-name go code and yaml share a single source of truth.~~

~~This item is removed and instead derived dynamically in go code.~~

eberlep · 2021-03-18T14:02:28Z

pkg/operatormanager/operatormanager.go


 	sccm := &v1.ConfigMap{}
-	if err := m.SetName(sccm, pg.SidecarsCMName); err != nil {


Maybe move this var to operatormanager.

~~As above, when go code and yaml share a variable, use the one in the yaml.~~

As above, the name is dynamically derived.

now it's getting even more complicated? we create that configmap ourselves, and then derive it's name from another config we also created?

Final decision: Use hard-coded const.

eberlep · 2021-03-18T14:06:07Z

pkg/operatormanager/operatormanager.go

+			Protocol: v1.ProtocolTCP,
+			TargetPort: intstr.IntOrString{
+				Type:   intstr.String,
+				StrVal: "exporter",


Hmm, maybe derive this value from the container definition? If someone changes the configmap an renames this port there, this service won't work anymore...

The configurable .Values.sidecars.exporter.containerPort in values.yaml is used now.

eberlep · 2021-03-18T14:09:58Z

api/v1/postgres_types.go

+	// Deal with dynamically assigned name
+	for i := range sidecars {
+		for j := range sidecars[i].Env {
+			if sidecars[i].Env[j].Name == "DATA_SOURCE_PASS" {


this check should be more general: If there is a sidecars[i].Env[j].ValueFrom, we should then check if it is of the kind SecretKeyRef, and if so, we should proceed.

This implementation is just a workaround for one specific case.

eberlep · 2021-03-19T12:46:18Z

api/v1/postgres_types.go

+	for i := range sidecars {
+		for j := range sidecars[i].Env {
+			if sidecars[i].Env[j].ValueFrom != nil && sidecars[i].Env[j].ValueFrom.SecretKeyRef != nil {
+				sidecars[i].Env[j].ValueFrom.SecretKeyRef.Name = "postgres." + p.ToPeripheralResourceName() + ".credentials"


This is still hardcoded for this usecase. I would prefer if this was completely generic. The name of the Secret should move back into the YAML/ConfigMap (the operator already supports the use of variables, in this case that string would look something like postgres.{{ main.teamid }}-{{ main.name }}.credentials).

And while you're at it, you could also add ConfigMapKeyRef, FieldRef and ResourceFieldRef (https://pkg.go.dev/k8s.io/api@v0.20.4/core/v1?utm_source=gopls#EnvVarSource).

Now that we talk about it: remove that complete logic and only use postgres.{{ main.teamid }}-{{ main.name }}.credentials in the ConfigMap

OK, my bad: Try {cluster}

No problem! The current setting in the ConfigMap is as follows: secret_name_template: '{username}.{cluster}.credentials'

We can't use this feature in this case.

eberlep · 2021-03-19T12:56:24Z

pkg/operatormanager/operatormanager.go

@@ -683,3 +703,19 @@ func (m *OperatorManager) UpdateAllOperators(ctx context.Context) error {
 	m.log.Info("Done updating postgres operators in managed namespaces")
 	return nil
 }
+
+func perNamespaceSidecarsConfigMapName(cm *v1.ConfigMap) string {


we should also talk about this, as already mentioned above. that looks too complicated to me?

As above, use hard-coded const instead.

eberlep · 2021-03-19T12:57:43Z

pkg/operatormanager/operatormanager.go


 	sccm := &v1.ConfigMap{}
-	if err := m.SetName(sccm, pg.SidecarsCMName); err != nil {


now it's getting even more complicated? we create that configmap ourselves, and then derive it's name from another config we also created?

LimKianAn · 2021-03-19T18:55:05Z

Tests:

3 containers in DB pod are running. No errors in each log eventually.
Service postgres-exporter is created. One can create an in-cluster curl pod to pin the end-point and get html response.

Thus, merge this PR.

LimKianAn added 3 commits March 5, 2021 20:21

Exclude .tgz

8a118a2

Use zalando.Sidecar yaml in ConfigMap

26246b3

Implement experimental additional volumes

9b8aae9

LimKianAn marked this pull request as draft March 5, 2021 20:35

LimKianAn mentioned this pull request Mar 8, 2021

Sidecars #146

Merged

Base automatically changed from sidecars to main March 9, 2021 15:00

LimKianAn added 3 commits March 17, 2021 15:27

Merge remote-tracking branch 'origin/main' into sidecar-experimental

bff6aab

Merge remote-tracking branch 'origin/main' into sidecar-experimental

eeac2b5

Shift the logic of global cm to yaml

9512a03

LimKianAn marked this pull request as ready for review March 17, 2021 19:11

LimKianAn requested a review from eberlep March 17, 2021 19:13

LimKianAn added 4 commits March 18, 2021 08:22

Better congimap key names

b0fdfec

Add explanation to sidecars-volumes-configmap

0e6c259

Better name

0b26e76

List sidecars directly in configmap yaml

cfa893d

eberlep requested changes Mar 18, 2021

View reviewed changes

LimKianAn added 5 commits March 18, 2021 17:03

Cleaner syntax

ec46544

Correct case

215b172

Remove redundant items from configmap

29a5557

Use more general form

030511c

Fix bug

329d65e

LimKianAn requested a review from eberlep March 18, 2021 20:31

eberlep reviewed Mar 19, 2021

View reviewed changes

Use const

282cb2e

LimKianAn merged commit b84c71d into main Mar 19, 2021

LimKianAn deleted the sidecar-experimental branch March 19, 2021 18:57


		sccm := &v1.ConfigMap{}
		if err := m.SetName(sccm, pg.SidecarsCMName); err != nil {

Sidecar experimental #157

Sidecar experimental #157

Uh oh!

Conversation

LimKianAn commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LimKianAn commented Mar 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LimKianAn Mar 18, 2021 • 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!

LimKianAn Mar 18, 2021 • 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!

LimKianAn Mar 18, 2021 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

LimKianAn commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LimKianAn commented Mar 5, 2021 •

edited

Loading

LimKianAn Mar 18, 2021 •

edited

Loading

LimKianAn Mar 18, 2021 •

edited

Loading

LimKianAn Mar 18, 2021 •

edited

Loading

LimKianAn commented Mar 19, 2021 •

edited

Loading