Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make the entrypoint of pgBackRest image configurable #3486

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

polikeiji
Copy link

@polikeiji polikeiji commented Dec 7, 2022

Checklist:

  • Have you added an explanation of what your changes do and why you'd like them to be included?
  • Have you updated or added documentation for the change, as applicable?
  • Have you tested your changes on all related environments with successful results, as applicable?
    • Have you added automated tests?

Type of Changes:

  • New feature
  • Bug fix
  • Documentation
  • Testing enhancement
  • Other

What is the current behavior (link to any open issues here)?

There is currently no interface to modify the entry point of the container image for the Kubernetes job of the backup process. It might not be ideal to use a custom container image to deal with some environment-specific issues, such as one in the Istio service mesh reported in #2341.

What is the new behavior (if this is a feature change)?

  • Breaking change (fix or feature that would cause existing functionality to change)

Other Information:

We've used this patch to use a custom pgBackRest image for running PGO inside the Istio service mesh. As I posted in #2341, one thing we need to do to make the backup process correctly work in the Istio service mesh is to add some logic to end the Istio sidecar proxy. We prepared a custom container image just adding the logic after the original logic, and we wrote this patch to change the entry point for the image. We can override the original entry point, but I think allowing users to change the entry point through the resource definition might be reasonable.

If you've already had some idea to deal with it, I don't mind closing the PR. If so, please let me know :)

@polikeiji
Copy link
Author

Hello team!

If there's something that I need to do to get reviews and feedback, please let me know.
And if the project intends to use only the official containers and prohibit users from customizing the workloads, please let me know. If so, we'll try to find another approach for the issue or find another solution.

Thank you!

@tony-landreth
Copy link
Contributor

Hi @polikeiji, thank you for your submission!

We are currently pulling together requests, issues, PR's, etc. related to service meshes (Istio included) as we assess the best approach for supporting various service mesh use cases in future operator releases.

As we take a look at this PR, we first have a few questions:

  • With sidecar-less approaches to service meshes out there, e.g. Istio Ambient Mesh, do sidecars still best meet your needs?
  • Other projects, e.g. the envoy-sidecar-helper, have been introduced to address the issue you appear to be working around in this PR. Do you have any thoughts on this solution?

Thanks in advance for your feedback, and we look forward to discussing your PR and use case further.

@polikeiji
Copy link
Author

Hello @tony-landreth, thank you for sharing the situation and giving the question :)

Let me answer the questions.

With sidecar-less approaches to service meshes out there, e.g. Istio Ambient Mesh, do sidecars still best meet your needs?

The ambient mesh feature might become the solution for the job termination issue with Istio, and we've been looking into it since it was released. But the K8S job termination issue isn't specific to Istio, and the same issue happens if some controller or some mutation webhook transparently adds a sidecar whose main process is a daemon process. I haven't checked it recently, but we faced the same issue on the pod with the Vault agent sidecar before. So, I think we should have a way to customize the image to keep the flexibility of deployments.

Other projects, e.g. the envoy-sidecar-helper, have been introduced to address the issue you appear to be working around in this PR. Do you have any thoughts on this solution?

Thank you for sharing it :) Actually, we've already checked those kinds of approaches to cover the lack of the Kubernetes features to control the containers in a pod, and we've taken a similar approach to the one you shared.

Being specific to the backup job termination issue, there are alternative solutions. But I think the CRD has the customization point for the entrypoint of the container is natural. I personally think no customization point, while there is one for swapping the container image doesn't make sense.

@jmckulk
Copy link
Collaborator

jmckulk commented Jun 13, 2023

Thank you for this feedback. As we look to improve support for service meshes, we will consider your use case.

@Agalin
Copy link

Agalin commented Jun 26, 2023

We have the same issue using Linkerd which explicitly states no plans of Istio's Ambient Mesh equivalent. Running another sidecar just to kill a sidecar feels... wasteful. Then imagine the already mentioned Vault sidecar. Now you need two sidecars to kill two sidecars (or a modified implementation of the "killer" sidecar).

But while entrypoint override would be a welcome addition, maybe just adding a "post backup script" would be a simpler approach? This way no container modification is necessary - just make a curl request there.

@polikeiji
Copy link
Author

But while entrypoint override would be a welcome addition, maybe just adding a "post backup script" would be a simpler approach?

I agree that "post backup script" is a simpler enough approach for many sidecar termination issues, and I also considered it when I wrote the patch in this PR :) I chose the way of overriding the entry point by considering the following.

  • For Istio's case, we don't need it, but might there be some cases in which we need to add some process before the main process to control the order of the pod's job processes?
  • For Istio's case, curl is enough and the PGO backup image has it, but might there be some cases in which we need to build a custom image to add other tools?

But I personally haven't faced the above situations, so taking the simpler approach also sounds reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants