Fully speced global sidecars

[fischerman]

This addresses #781. It also works really nicely with #736

I've followed the same approach as with init containers, by fully exposing the Kubernetes container specification. This, for example, enables volume mounts required for #736.

I've deprecated sidecar_docker_images in favor of the new sidecars. I think the transition is straightforward and we can eventually remove some of this cumbersome code.

With the flexibility of sidecars, it might also be worth considering to specify Scalyr in this fashion (maybe put an example in the docs). An immediate win would be, that the API key is not stored in plaintext, but can be put in a secret instead (and less custom code).

The operator does not manipulate the resource specification of the sidecars. I think this is reasonable because...

• ... taking the default resources of the Spilo container is most likely overkill. Sidecars tend to be small.
• ... the default resources and globally defined sidecars are managed by the same role; let's call him operator operator ;)

As far as I can judge this PR is non-breaking and preserves the existing behavior.

[FxKu]

Thanks @fischerman for your PR. In general, we think allowing full specs to our CRs is also a source of misuse and errors. But, I do see the appeal of making things like monitoring easier for users. Do you want us to focus on #736 getting merged so that it can be reflected in this PR here, too? Not sure if one want all specified volumes in sidecars, too.

[fischerman]

@FxKu thanks for looking into this. Can you elaborate on the possible misuse of this feature? Sure, I can easily break the deployments if I configure sidecars wrongly but that is true for many operator-level settings.

I took a closer look at #736 and see what you mean. The way that targetContainer and the required mountPath work wouldn't allow us to mount the volumes on the "container side". So you would just leave volumeMounts empty and have it filled by #736, in which case no change is required.

Independent of volume mounts, one ability of this PR which I consider useful is the ability to specify ports. Although ports can be reached even if not specified, it's very hacky and complicates configuration of (for example) Prometheus.

[FxKu]

@fischerman #736 is now merged

[Jan-M]

Sounds like a great PR. Removing Scalyr code in the scope of this PR would be fine from our side. W would then include this an the release notes.

[fischerman]

Awesome, I will solve the merge conflicts.

I can also remove the Scalyr code, and optionally the old style global sidecars. Note however, that both of them are breaking changes to the config CRD. Apart from considerations about CRD versioning, the following "no downtime" approach would not be possible:

• Upgrade to v1.5
• Change config to new style
• Restart operator
• Eventually upgrade to v1.6 (which removes the deprecated properties)
• Operator removes old properties from CRD (possibly increases CRD version)

Should I remove either for v1.5?

[fischerman]

Rebased.

Let me know regarding the above and I make the necessary changes.

[fischerman]

I've fixed #924 with the last commit. I did not write a test for it, since it's deprecated anyways.

[FxKu]

Could you add a mention and link from the user docs to this section?

[FxKu]

LGTM! Aside from very minor things, I'd like to merge this PR.

Could you add a commented example in the default configuration manifest? I will add more deprecation notes for the Scalyr fields in a latter polishing PR befor the release. A PR to remove Scalyr is also there already.

[fischerman]

@FxKu Done!

[FxKu]

👍

[sdudoladov]

👍

[sdudoladov]

@fischerman thank you for the contribution :)