Option to disable Pod restart when coordinator or worker configmap changes

[sdaberdaku]

Currently, both the coordinator and worker Deployments add an annotation to their Pods with the corresponding configmap checksum, which ensures that the Pods are restarted upon configuration changes. When managing configuration files with the additionalConfigFiles property, sometimes it can be useful to avoid restarts if the content of one of these files is changes (i.e. configurations with the refresh-period setting that are automatically refreshed by Trino).

Could we add an option to disable these annotations if the users desires to do so?

[nineinchnick]

If only the contents of files referenced in additionalConfigFiles change, and no other setting, the checksum should be the same, right?

I'm not against the suggestion, I'm only thinking how to write a test case for it.

[sdaberdaku]

If only the contents of files referenced in additionalConfigFiles change, and no other setting, the checksum should be the same, right?

not really, I've tested it locally and the checksum changes if you change the content of an additionalConfigFile, even if it is templated.

Sorry I misread your comment. If the files referenced change and they are not mounted through this configmap then yes, the checksum does not change.

I think it would be hard to test, how would we simulate a configuration change at runtime?

[sdaberdaku]

We could just test if the annotation is present or not...

[sdaberdaku]

So let me rephrase the use case: if I want to mount a custom templated config file, I have to do it through Helm, which would make this feature useful. If the mounted file does not need templating, I would mount it with a custom configmap external to the Trino Chart.

[sdaberdaku]

After thinking about this, I came to the conclusion that one could just add the "checksum" annotations to override in the coordinator and/or worker annotations with an fixed string value (could be the empty string even) and effectively disable the restart on configuration change. Since user-provided annotations are added after the computed annotations, this should override the previous values.

coordinator:
  annotations:
    checksum/access-control-config: ""
    checksum/catalog-config: ""
    checksum/coordinator-config: ""

One thing I do not like is that this is kind of undocumented behavior (but maybe I'm wrong).

@nineinchnick what do you think?

[nineinchnick]

We definitely should document what annotations we add by default and this use case when users would want to overwrite them.