-
Notifications
You must be signed in to change notification settings - Fork 67
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
Allow overrides to update-check job spec #290
Comments
I'm happy to attempt work on these changes, wanted to open the issue to get the conversation started and see if this would be supported. The primary motivation for this is coming from my work on istio injecting Mattermost, without an ability to disable the sidecar (via labels) or terminate the sidecar (via command override) for the update-check upgrades will require manual intervention to successfully complete. |
Hey @mjnagel, thanks for submitting the issue. I agree that adding a way to configure an update job would be very useful, as well as other things in the A big chunk of So for exposing the whole |
@Szymongib definitely bring up some good points here...it's a balance between providing customization to the end user and making sure that things won't break with that customization. Could potentially go the route of throwing out any conflicting overrides (i.e. if I try to override the Definitely just throwing things out there...I might have to do some research into what other operators have done to handle these scenarios, since I know we make use of several where labels/annotations values are accessible. |
This will be a long comment...I've been poking around through each of the areas I identified and seeing what the most feasible path forward would be (in my mind). I realize some of this deviates from the initial issue (or at least the title) so I'm happy to open up other issues to tackle individual pieces of this. Here's some of my thoughts/conclusions... LabelsIt seems like adding labels is already available via I did a quick test adding: spec:
resourceLabels:
app: mm-test The result appears to be that the operator fails to reconcile the change due to mismatch with selector labels, throwing a log error: time="2022-07-20T15:27:22Z" level=info msg="[opr.controllers.Mattermost] Updating resource" Reconcile=mattermost Request.Name=mattermost Request.Namespace=mattermost kind="&TypeMeta{Kind:,APIVersion:,}" name=mattermost namespace=mattermost patch="{\"metadata\":{\"labels\":{\"app\":\"mm-test\"}},\"spec\":{\"template\":{\"metadata\":{\"labels\":{\"app\":\"mm-test\"}}}}}"
time="2022-07-20T15:27:22Z" level=error msg="[opr.controller.mattermost] Reconciler error" error="failed to update mattermost deployment: Deployment.apps \"mattermost\" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{\"app\":\"mm-test\", \"installation.mattermost.com/installation\":\"mattermost\", \"installation.mattermost.com/resource\":\"mattermost\"}: `selector` does not match template `labels`" name=mattermost namespace=mattermost reconciler group=installation.mattermost.com reconciler kind=Mattermost While maybe unintentional, the result is effectively that the user's override is thrown out/ignored due to the way the operator handles the labels vs selector labels:
My suggestion on the way forward would be to pursue the path of giving the operator defaults precedence (in all cases) and logging a warning message in the operator logs when the user attempts to override one of the operator defaults. If I'm not mistaken the only operator default labels are:
In my opinion preventing a user from overriding these labels would be within the normal role of an operator, and still would provide flexibility to the end user to add extra labels as needed. AnnotationsTo my knowledge the only annotations currently set on MM pods are the prometheus annotations when enterprise is enabled. The three annotations are:
I think I'm coming down to the same conclusion as with labels on this one, accept the end user's overrides and give precedence to the 3 defaults for prometheus. There may be room on this one for a special case with the scrape true/false annotation, but that could be done via a special flag for metrics if we want to support it? Security ContextFrom a review of the code I couldn't find any places where a default Update Check SpecCurrently the
SummaryThe path forward I'm proposing here would look like this in terms of changes to the spec: spec:
# Note that this already exists, I'm just suggesting better visibility/handling of these labels
resourceLabels:
# This override would be thrown out, result in a logged warning
app: mm-test
# This label would be added to all resources
owner: mjnagel
# Applies to MM pods in the deployment
podAnnotations:
# This override would be thrown out, result in a logged warning
prometheus.io/port: 8067
# This annotation would be added to MM pods in the deployment
owner: mjnagel
# Applies to MM pods in the deployment
podSecurityContext:
runAsNonRoot: true
# Applies to all containers in the MM deployment
containerSecurityContext:
capabilities:
drop:
- ALL
updateCheck:
# The below operate in the same way as their corresponding `spec.*` options
podLabels: ...
podAnnotations: ...
podSecurityContext: ...
containerSecurityContext: ...
# These commands would be appended to the default command run in the job
postUpdateCommands:
- echo "Stopping the istio proxy..."
- curl -X POST http://localhost:15020/quitquitquit @Szymongib This is definitely a lot (both to read and to consider adding) and I'm sure the naming/organization could be simplified in some ways. Definitely let me know your thoughts and I'm happy to spin off new issues that tackle individual portions of the changes here if that is more appropriate for discussion/working. |
Thanks a lot for diving into this, @mjnagel, and for such detailed write up. I definitely agree that we can improve with labels and annotations handling as a lot of this things were implemented very early on on never revisited, hence a weird behaviour with labels etc. We wanted to add support for Another thing you mentioned, that I strongly agree with are annotations, I like the idea of not overriding those that should not be overriden, perhaps we could also explore disabling them with something like Regarding the labels, With the update check job, the spec is copied as this is a simple way to ensure that:
I think we should allow some customization there as now it is pretty highly opinionated thing that users should have a way to customize / disable. I would be really cautious with changing the logic of copying the spec, so perhaps we could start with applying some overrides on top of that, and Regarding the shape of the config, we are trying to move more towards grouping things by resource as it makes it easier for user to find and understand particular config options, handle things from code perspective, as well as make it more extendable for the future. I could see therefore something like: spec:
pod/deployment: # depending how granular we want to go we could do deployment.podTemplate etc...
podSecurityContext: ...
annotations: ... # if we want something pod/deployment
...
updateCheck:
... |
To narrow the focus of this issue to just the update-check I opened #298 to handle the labels/annotations support for the main app server pods. |
Summary
From a quick scan through the code I don't believe there is currently any way to override parts of the spec for the update-check job that runs with version updates. It may be desirable to override various parts of this job, for example:
Ultimate it would be great to be able to override different portions of the pod spec.
Possible fixes
Add a new section to the Mattermost CRD spec that allows for update check pod spec overrides. This would honestly be beneficial for the primary Mattermost pods as well, since currently you have to utilize
resourcePatch
to add labels/annotations to the pods (which is not very beginner friendly or easy to navigate, especially with multiple changes.Proposed Change:
The text was updated successfully, but these errors were encountered: