Support for per-cluster and operator global sidecars #331

alexeyklyukin · 2018-06-26T15:21:19Z

Based on the work of @theRealWardo to to implement per-cluster sidecards
Add support for operator-global sidecards (currently crude, as we only allow the list of docker images, and the optional variables (besides mandatory POD_NAME, POD_NAMESPACE, POSTGRES_USER and POSTGRES_PASSWORD ) can only be propagated via either pod_environment_configmap or Pod Presets. Should be improved once Allow configuring the operator via the YAML manifest. #326 is merged).
Do refactoring around the code that generates the statefulset, particularly, decouple most of the helper routines from the cluster object in order to simplify their testing in the future, as well as to convert the scalar configuration internally to a sidecar structure to use the common code path.

# Conflicts: # docs/user.md

Right now only the name and the docker image can be defined, but with the help of the pod_environment_configmap parameter arbitrary environment variables can be passed to the sidecars.

Jan-M · 2018-06-27T10:42:00Z

pkg/util/config/config.go

+	WatchedNamespace string            `name:"watched_namespace"`    // special values: "*" means 'watch all namespaces', the empty string "" means 'watch a namespace where operator is deployed to'
+	EtcdHost         string            `name:"etcd_host" default:""` // special values: the empty string "" means Patroni will use k8s as a DCS
+	DockerImage      string            `name:"docker_image" default:"registry.opensource.zalan.do/acid/spilo-cdp-10:1.4-p8"`
+	Sidecars         map[string]string `name:"sidecars"`


If I see this right, this is a map of sidecar container name to docker images? Maybe we should make it more clear from the variable name? That is not sidecar name to yaml?

While this is documented I wouldn't mind a better name. Perhaps sidecar_docker_images?

Jan-M · 2018-06-27T10:42:40Z

How would this solution help us with our Scalyr Sidecar? I dont see how to inject the key on global level?

alexeyklyukin · 2018-06-27T11:19:45Z

Well, outside of making the code less horrible by converting the scalyr hard-code to the same sidecar structure used to inject other sidecars so far it doesn't. However, I don't think it should be our goal to cram those things into the operator; what I propose instead is to rely on the Kubernetes feature called PodPresets to inject environment variables and remove all duplication of it (namely, https://postgres-operator.readthedocs.io/en/latest/administrator/#custom-pod-environment-variables) from the operator altogether.

Make it more explicit that this parameter only controls the docker images, not the YAML manifest definiting other aspect of sidecar configuration (such as environment variables.)

alexeyklyukin · 2018-06-27T12:16:20Z

👍

sdudoladov · 2018-07-02T12:36:36Z

docs/reference/operator_parameters.md

+* **sidecar_docker_images**
+  a map of sidecar names to docker images for the containers to run alongside
+  Spilo. In case of the name conflict with the definition in the cluster
+  manifest the cluster-specific one is preferred.


is preferred == is used ?

sdudoladov · 2018-07-02T12:45:31Z

pkg/cluster/k8sres.go

+	return envVars
+}
+
+// deduplicateEnvVars makes sure there are no duplicate in the target envVar array. While Kubernetes already does, it


While Kubernetes already does,

already does deduplication i assume?

yes, looks like I've lost a word or two :-(

sdudoladov · 2018-07-02T12:49:43Z

pkg/cluster/k8sres.go

+			names[va.Name] += 1
+			result = append(result, input[i])
+		} else if names[va.Name] == 1 {
+			names[va.Name] += 1


why do we need to count the number of definitions if we seem even not to log this number ?

To avoid producing this log message on each occurrences of the duplicate parameter (as opposed to only once when the duplicate is detected.)

sdudoladov · 2018-07-02T12:50:44Z

pkg/cluster/k8sres.go

+			result = append(result, input[i])
+		} else if names[va.Name] == 1 {
+			names[va.Name] += 1
+			logger.Warningf("variable %q is defined in %q more than once, the subsequent definitions are ignored",


is it worth logging here the first definition here to showcase what is actually applied ?

I would say the purpose of this warning is to prevent such cases, not to assist in constructing them to get what you want. I don't want to overload it with an extra debugging info.

sdudoladov · 2018-07-02T12:55:47Z

pkg/cluster/k8sres.go

+	// resolve conflicts between operator-global and per-cluster sidecards
+	sideCars := c.mergeSidecars(spec.Sidecars)
+
+	resourceRequirementsScalyrSidecar := makeResources(


generating the scalyr sidecar probably deserves a separate function; this method is already way too long

On the other hand, the function to generate Scalyr sidecars has only 5 parameters instead of 9. I preferred smaller functions that build parts of the manifest and them combine them all in a reasonably long generateStatefulSet, rather than a small generateStatefulSet that calls rather complex functions; the first approach makes the code easier to test and to understand.

sdudoladov · 2018-07-02T12:59:17Z

pkg/cluster/k8sres.go

+
+	// generate scalyr sidecar containers
+	if scalarSidecar, present :=
+		generateScalarSidecarSpec(c.Name,


Scalar

Scalyr :)

sdudoladov · 2018-07-02T13:02:56Z

pkg/cluster/k8sres.go

+}
+
+func generateScalarSidecarSpec(clusterName, APIKey, serverURL, dockerImage string, containerResources *spec.Resources) (sidecar *spec.Sidecar, present bool) {
+	if APIKey == "" || serverURL == "" || dockerImage == "" {


does this mean that if this particular sidecar is misconfigured, we silently ignore this fact w/o logging it?

Yes. Probably the better solution would be to complain if one of those parameters is set but not all.

sdudoladov · 2018-07-02T13:07:39Z

pkg/util/config/config.go

+	WatchedNamespace string            `name:"watched_namespace"`    // special values: "*" means 'watch all namespaces', the empty string "" means 'watch a namespace where operator is deployed to'
+	EtcdHost         string            `name:"etcd_host" default:""` // special values: the empty string "" means Patroni will use k8s as a DCS
+	DockerImage      string            `name:"docker_image" default:"registry.opensource.zalan.do/acid/spilo-cdp-10:1.4-p8"`
+	Sidecars         map[string]string `name:"sidecar_docker_images"`


what about adding a short example to the operator manifest ?

not sure it actually needs sidecars.

Per review by @zerg-junior

coveralls · 2018-07-02T14:00:45Z

Coverage decreased (-0.2%) to 4.678% when pulling 98da75a on refactoring/sidecars into 74b19b4 on master.

sdudoladov · 2018-07-02T14:05:39Z

👍

Per review by @zerg-junior

alexeyklyukin · 2018-07-02T14:15:03Z

👍

sdudoladov · 2018-07-02T14:15:25Z

👍

theRealWardo · 2018-07-05T16:22:29Z

awesome! thanks for carrying this through!

theRealWardo and others added 10 commits June 6, 2018 01:34

trying to make a more generic sidecar

cfcae65

fix compilation errors

940b3c9

update for new docs

20a8348

forgot ports

232359d

ports not posts silly

beec6e2

sidecars not sideCars

6b2a02b

Merge branch 'master' into global_and_per_cluster_sidecards

2a32eb4

# Conflicts: # docs/user.md

Define sidecars in the operator configuration.

1e4f046

Right now only the name and the docker image can be defined, but with the help of the pod_environment_configmap parameter arbitrary environment variables can be passed to the sidecars.

Refactoring around generatePodTemplate.

0940d36

Deduplicate variables for sidecars.

57297dc

alexeyklyukin mentioned this pull request Jun 26, 2018

arbitrary sidecar support #308

Closed

Jan-M reviewed Jun 27, 2018

View reviewed changes

alexeyklyukin added 3 commits June 27, 2018 13:21

Merge branch 'master' into refactoring/sidecars

b2ea3b6

Document the per-cluster sidecar options

3a2c570

Rename the sidecars operator parameter.

fa1259f

Make it more explicit that this parameter only controls the docker images, not the YAML manifest definiting other aspect of sidecar configuration (such as environment variables.)

sdudoladov reviewed Jul 2, 2018

View reviewed changes

Fix a few typos and improve the error messags.

e4f60e8

Per review by @zerg-junior

Minor refactoring

98da75a

Per review by @zerg-junior

alexeyklyukin merged commit 25a3062 into master Jul 2, 2018

alexeyklyukin deleted the refactoring/sidecars branch July 2, 2018 14:25

frittentheke mentioned this pull request Jan 6, 2020

Allow configuration of global_sidecars / sidecar_docker_images #781

Closed

benkroeger mentioned this pull request Jan 27, 2022

Feature Suggestion: generic sidecar support reactive-tech/kubegres#89

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for per-cluster and operator global sidecars #331

Support for per-cluster and operator global sidecars #331

alexeyklyukin commented Jun 26, 2018

Jan-M Jun 27, 2018

alexeyklyukin Jun 27, 2018

Jan-M commented Jun 27, 2018

alexeyklyukin commented Jun 27, 2018

alexeyklyukin commented Jun 27, 2018

sdudoladov Jul 2, 2018

alexeyklyukin Jul 2, 2018

sdudoladov Jul 2, 2018

alexeyklyukin Jul 2, 2018

sdudoladov Jul 2, 2018

alexeyklyukin Jul 2, 2018

sdudoladov Jul 2, 2018

alexeyklyukin Jul 2, 2018

sdudoladov Jul 2, 2018

alexeyklyukin Jul 2, 2018

sdudoladov Jul 2, 2018

sdudoladov Jul 2, 2018

alexeyklyukin Jul 2, 2018

sdudoladov Jul 2, 2018

alexeyklyukin Jul 2, 2018

coveralls commented Jul 2, 2018 •

edited

Loading

sdudoladov commented Jul 2, 2018

alexeyklyukin commented Jul 2, 2018

sdudoladov commented Jul 2, 2018

theRealWardo commented Jul 5, 2018

Support for per-cluster and operator global sidecars #331

Support for per-cluster and operator global sidecars #331

Conversation

alexeyklyukin commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jan-M commented Jun 27, 2018

alexeyklyukin commented Jun 27, 2018

alexeyklyukin commented Jun 27, 2018

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

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

coveralls commented Jul 2, 2018 • edited Loading

sdudoladov commented Jul 2, 2018

alexeyklyukin commented Jul 2, 2018

sdudoladov commented Jul 2, 2018

theRealWardo commented Jul 5, 2018

coveralls commented Jul 2, 2018 •

edited

Loading