-
Notifications
You must be signed in to change notification settings - Fork 593
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
base: main
Are you sure you want to change the base?
Conversation
86c8552
to
c95d8be
Compare
Hello team! If there's something that I need to do to get reviews and feedback, please let me know. Thank you! |
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:
Thanks in advance for your feedback, and we look forward to discussing your PR and use case further. |
Hello @tony-landreth, thank you for sharing the situation and giving the question :) Let me answer the questions.
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.
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. |
Thank you for this feedback. As we look to improve support for service meshes, we will consider your use case. |
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. |
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.
But I personally haven't faced the above situations, so taking the simpler approach also sounds reasonable to me. |
Checklist:
Type of Changes:
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)?
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 :)