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

Fix the readiness probe using a sidecar container #109

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

lorenzboguhn
Copy link

Hi,
This MR aims to fix the readiness check regarding the helm chart.

The original solution caused the StatefulSet to be not upgradable by standard k8s methods and further problems using GitOps tools(ArgoCD and its health checks).
Additionally, readiness alerts and further things cause problems with the current solution.

(https://community.ibm.com/community/user/integration/discussion/ibm-mq-nativeha-on-kubernetes-with-ibm-messagingmq-helm-chart)

Original Solution:
Only one pod has a readiness check that runs successfully -> service will only serve the leader instance

This Solution:
Patch the label in a sidecar container -> service will only serve the leader instance (via selector)

Thanks for reading this far. I will happily welcome any feedback.
Best Regards,
Lorenz

@lorenzboguhn
Copy link
Author

Hi @callumpjackson, are there any interests in adjustments/improvements of the helm chart ?

@callumpjackson
Copy link
Collaborator

Hi @lorenzboguhn - thanks for raising this pull request. This is an interesting approach to the challenge and an area we have considered improving in the past. Until now we have taken the approach of providing sample scripts to assist users with the challenge. On reviewing the approach I can see that this introduces a requirement for access to the Kubernetes APIs to update the labels. In some cases this would not be ideal as many customers run in a multi-tenant environment where they wouldn't want to give these permissions. I would like to hold this Pull Request out at the moment and understand if there are others who would like to see this enhancement and if this solution would address their concerns or not.

@lorenzboguhn
Copy link
Author

Hi @callumpjackson, i can understand that the access to the kubernetes api is not optimal for multi tenant environments. Nevertheless, this access is namespaced provided via the role + rolebinding. And if you are able to install a helm chart, you might as well are able to access the get list and patch APIs for pods.

However, the current solution for updates and changes is based on scripts which are also not(!) ideal for multi tenant environments. They can also easily be executed for the wrong instance, or adapted wrong or else broken.

@htaunay
Copy link

htaunay commented Dec 5, 2024

+1 to this, even if with some templating conditionals to make it an opt-in configuration

@lorenzboguhn
Copy link
Author

Hi @callumpjackson,
I have proceeded as @htaunay has proposed: this is now an opt In feature. It is disabled by default and only enabled by overriding the default values.
Additionally, I have limited the role to just be able to access the mq pods via the resourceNames in the k8s role.
I really would appreciate your feedback.
Best regards,
Lorenz

@lorenzboguhn lorenzboguhn force-pushed the feature/fix-readiness-check branch from be6c71a to a927f74 Compare December 16, 2024 18:42
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.

4 participants