router
router copied to clipboard
apollographql
chore: add configmap annotation to restart deployment on config changes
This updates the helm chart to add a configmap checksum annotation which will trigger a restart of the router deployment when configuration changes are made. Currently, configuration only changes do not cause the deployment to roll over.
@hobbsh: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/
If hot-reload worked correctly when mounted as a configmap this shouldn't be necessary. #1695
If hot-reload worked correctly when mounted as a configmap this shouldn't be necessary. #1695
@kmcrawford I haven't tried specifying a "local" supergraph so I can't speak to that (we are using Apollo Studio), but this PR is for the actual router.yaml that gets loaded as a configmap by default in the helm chart. Since it's likely you're creating the configmap via some external process, you could try adding the same annotation to your supergraph configmap.
@hobbsh if the chart has hot-reload enabled, and any deployment updates the configmap, a restart of the router pod should not be necessary. The hash of the configuration in an annotation, is forcing the pod to be restarted. This should not be required if the router worked correctly with hot-reload enabled. When using the router with many different subgraphs, if a subgraph from another team is deployed they can update the supergraph configmap, the router should pickup the changes and hot-reload.
@garypen do you mean add a new entry to CHANGELOG.md?
Also, I'm wondering if it would be better to just wait until hot reloading is fixed, this is a fairly brute-force approach to the issue. Configmaps mounted via volumes should automatically update within pods (although I haven't actually verified this given I can't get a shell in the container) so I'm not sure this is a great approach.
@hobbsh Sorry, I mis-lead you there. I meant NEXT_CHANGELOG.md. (BTW: If you want to test things out in your docker image with a shell, we also make debug images which contain a busybox that provides a shell...)
As to whether we should do this or should rely on hot-reload. That requires a bit of thinking...
Heya folks, just circling back on this: Is this potentially no longer necessary with the hot reload hopefully in a better state thanks to https://github.com/apollographql/router/pull/2276?
I think the fixes to --hot-reload (#2276) could make this redundant. We additionally now have to capability to restart based on changes to a supergraph config-map (#2223), so I think restart requirements are covered.
I suppose this could still be helpful in the scenario where --hot-reload is not enabled, so happy to consider adopting it for that use case.
Thanks for opening this originally. We can easily re-open this and restore the branch, if we find we need it in the future, but at this point I think this has been open for a bit too long.