Fix outdated configuration reference (nginx -> traefik -> not relevant and removed) #1909
Conversation
mybinder/values.yaml
Outdated
| @@ -274,7 +277,7 @@ binderhub: | |||
| # - --default-target=http://$(PROXY_PATCHES_SERVICE_HOST):$(PROXY_PATCHES_SERVICE_PORT) | |||
| - --error-target=http://$(PROXY_PATCHES_SERVICE_HOST):$(PROXY_PATCHES_SERVICE_PORT)/hub/error | |||
| - --log-level=error | |||
| nginx: | |||
| traefik: | |||
There was a problem hiding this comment.
Hmmm wouldn't something be broken if this were the wrong key name though?
There was a problem hiding this comment.
The autohttps pod (running traefik) would not get the explicit resource requests, which could break something but doesn't have to break something.
The pod would still run with the default requests (guarantees/limits in CPU/memory). What the default requests would be depends on the k8s cluster configuration though.
It could be that this makes things more reliable on one but not all of our k8s clusters in the federation, or that it didn't matter, or it could matter in certain situations based on what other pods compete for resources on the node where the pod is scheduled.
There was a problem hiding this comment.
It could be that this makes things more reliable on one but not all of our k8s clusters in the federation, or that it didn't matter, or it could matter in certain situations based on what other pods compete for resources on the node where the pod is scheduled
Well that just about covers all possibilities then 😅
This seems like a reasonable fix to me! I may not be the most well qualified to determine this though
There was a problem hiding this comment.
What does this section of the config control? AFAIK mybinder.org does not use the autohttps pod, so if this config is only used by the autohttps pod then maybe we should just remove it because it is unused
There was a problem hiding this comment.
Ah, it is only used for the autohttps pod. Removed it in afee564
proxy.traefik relates to the autohttps pod, but that isn't used in mybinder.org so we remove this configuration that only applies to it.
consideRatio
changed the title
I was trying out jupyterhub/zero-to-jupyterhub-k8s#2200 which includes a schema validation that can also detect references to values that aren't recognized. Just using
helm template .made me spot this reference tonginxin the JupyterHub helm chart configuration. This probably refers totraefikwhich is now running in the autohttps pod rather thannginxwhich was running there before (perhaps 0.9.X or earlier).