[MM-16722] Use github.com/banzaicloud/k8s-objectmatcher for all resource reconciliation comparisons #72
Conversation
|
@jozuenoon thanks for the PR! Let me know when this is ready for review and I'll add reviewers |
There is race interaction between mysql operator and mattermost operator in case of applying patch to whole resource. We should only update here what is expected to change otherwise it wrecks mysql operator. Observed behaviour was for example: - mysql operator stops responding to any changes in resource, - database remains in read only mode.
|
How's this going @jozuenoon ? Need any help? |
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Updating mysql.cluster with objectMatcher breaks mysql operator. | ||
| // Only fields that are expected to be changed by mattermost-operator should be included here. |
There was a problem hiding this comment.
Until other problems around database resource are resolved, mysql reconcile connot be changed to objectMatcher.
|
@cpanato @gabrieljackson what do you think about @jozuenoon's thoughts in the summary? |
|
I think your changes make sense, especially regarding the mysql cluster. We should do our best to not interfere with the internal workings of the mysql operator and just reconcile the parts of the spec we are setting For the follow-ups:
I agree with not using a static name for the db cluster. The problem with using a variable name is that the mysql slaves use a really long name to connect to the master (see bitpoke/mysql-operator#170) which is why we started using the static name.
I'll leave that to @cpanato's feedback
I'm all for that 👍
Not sure what you mean on how this saves an API call? The reason we separated those out was so they could be reused if needed
Makes sense to me |
There was a problem hiding this comment.
Very nice work @jozuenoon!
@jwilander has covered most of your questions/thoughts and I agree with him. Primarily, we are currently constrained by the mysql operator behavior as he noted, so it makes sense to improve that objectmatching in a follow-up PR.
As for the e2e tests, they should be flushed out since we have added quite a bit to the operator since their inception, so we will keep your thoughts in mind for when we get around to that.
Thanks again! 🎉
Resolves: mattermost/mattermost#11919
Some comments about changes:
mattermost.com/last-applied.currentanddesired- this change is up to discussion. I found it much more readable this way and less likely to make mistake when passing down toupdatefunction. By function context it should be always obvious whatcurrentanddesiredare.mysql-clusterresource if used with objectmatcher have negative impacts onmysqloperator, resulting in either cluster being stalled in read only mode or completely ignoring subsequent changes to spec. This happen in situation whenTestMattermost/mattermost_upgrade_teststarts. Sincedbresource have same name for every deploymentmysql-clusterresource gets override (fromtest-mmtotest-mm2) with new password secret and owner reference. I found that ifobjectmatcheris in usemysqlannotationmysql.presslabs.org/versiongets dropped, resulting inmysqloperator ignoring any changes to resource. If this annotation is restored manually it makes some progress however cluster stays in read only mode.Some followup ideas:
mysql-clusterresource name variable, depending on name of mattermost deployment<name>-dband not using staticdb(this would probably prevent problems with objectmatcher on this resource),test-mm2overrides with downgraded version, however database schema doesn't change since it operates on same resource, then upgrade is performed, but it performs upgrades (if any) over already existing upgraded version. If testing schema upgrades is not intended here that may need a comment in tests, otherwise this should be fixed with variable db resource name likely also triggering some changes to e2e test,mysql-operatorinteractions with CRD and look through logs for any possible strange behaviors,create<...>IfNotExistsfunctions since embedding them directly into reconcile functions would save one call to API Get and improve line of sight,