Env per pod used to exist but has been removed - why? #629
Comments
|
@mb-m it's been a while since that change, but I think the main reason was to reduce the risk that people set inconsistent airflow configs across their cluster, either because they don't define them consistently, or because they don't restart all the Pods after a secret which set an env is updated (as Secret/ConfigMap updates are not propagated to I am not against introducing an
Also, I would love to know what extra features we are missing from the |
|
@thesuperzapper thanks for getting back to me so quickly (and apologies for my delayed response back - I was totally useless a few days ago after a positive test result and symptoms - and then I thought I'd sent this response and hadn't). That's an interesting set of concerns, for sure, I've always tended to err on the side that the user should know what they are doing, but I can totally see why protecting them from themselves is worthwhile. To be honest, I'm expecting the xxxx.extraEnv to be small though, and its usage to be limited. As to the syncer - the big one that's missing for me is "roles" (so actual permissions) - for instance, I turn off There's an extra part here, which is that we've been doing this weird hack (we currently use the google oauth) where we ask users to register, we capture their google oauth id and only then update our list of users and roles to sync. As I'd like us to move away from Google, being able to do the sync for "any account with this email address" coming from oauth is something we're likely to put in our version of the syncer soon. This also means we can (to some extent) pre-prepare users - they can register, and then within 5-10 minutes of registering, they'll have the right permissions - because we can potentially export the email address permissions from elsewhere. I like your suggestions though - I'd probably disagree that you need to go as far as a hashmap because the number of these extraEnvs is still relatively small, and there are so many other things that are slow in helm that looping all of these bits is the least of my worries - but I think that a template for each possible place where the envs might be set is enough. I'll have a go on getting the other bits together as a PR. |
|
@mb-m I am interested to see your approach or any suggestions you have to make the chart better for you (whether as a PR, or in an issue). Also, there is actually a PR to add syncing for FAB Roles #537, but I haven't looked into the problem yet, so I have no idea if that PR attacks the problem in the best way. |
|
Before I wrap this fully up into a PR and have a proper testcase, how does the attached look - am I capturing the kinds of things you were thinking? |
|
@mb-m it's quite difficult to look at in a patch form, can you make a PR so we can discuss it there? (even if its not fully ready yet, we can discuss it more in your PR) |
As per airflow-helm#629, add support for .extraEnv for the various pod types, including some of the validation and warnings. This is generally better where we either use an environment variable that's something new, or that we are overriding something in airflow.config (eg. setting the log level at DEBUG on workers but INFO everywhere else) Signed-off-by: Matthew Byng-Maddick <[email protected]>
…d types As per airflow-helm#629, add support for .extraEnv for the various pod types, including some of the validation and warnings. This is generally better where we either use an environment variable that's something new, or that we are overriding something in airflow.config (eg. setting the log level at DEBUG on workers but INFO everywhere else) Signed-off-by: Matthew Byng-Maddick <[email protected]>
…d types As per airflow-helm#629, add support for .extraEnv for the various pod types, including some of the validation and warnings. This is generally better where we either use an environment variable that's something new, or that we are overriding something in airflow.config (eg. setting the log level at DEBUG on workers but INFO everywhere else) Signed-off-by: Matthew Byng-Maddick <[email protected]>
|
@thesuperzapper hopefully you've seen that a PR now exists for this (it's minorly changed from my original patch, but I have it demonstrably working (using a patched version of the chart) in our environment). I'd love to hear your thoughts! |
|
This issue has been automatically marked as stale because it has not had activity in 60 days. Thank you for your contributions. Issues never become stale if any of the following is true:
|
|
This is still waiting on the PR (comment just to stop staleness) |
…d types As per airflow-helm#629, add support for .extraEnv for the various pod types, including some of the validation and warnings. This is generally better where we either use an environment variable that's something new, or that we are overriding something in airflow.config (eg. setting the log level at DEBUG on workers but INFO everywhere else) Signed-off-by: Matthew Byng-Maddick <[email protected]>
…d types As per airflow-helm#629, add support for .extraEnv for the various pod types, including some of the validation and warnings. This is generally better where we either use an environment variable that's something new, or that we are overriding something in airflow.config (eg. setting the log level at DEBUG on workers but INFO everywhere else) Signed-off-by: Matthew Byng-Maddick <[email protected]>
Checks
User-Community Airflow Helm Chart.Motivation
I'm trying to (finally) move from a patched older version of the community chart to the latest version (and working on other patches - issue and PR to hopefully follow), but I noticed that there's no longer a way to specify an environment variable for a single pod. In my case, I set the AIRFLOW__CORE__LOGGING (I'm still running 1.10.14) to INFO everywhere except the workers where I set it to DEBUG.
I also have my entity syncer (which has a little bit extra from the ones now in the 8.x.x chart), which has a couple of extra useful environment variables that I don't want to specify everywhere (obviously copying in the custom template is easy enough for the latter, but annoying for the workers). There are overrides for many of the other things, and there's presumably a good reason (that I'm missing?) why these used to exist in the 7.x.x versions of the chart and were removed. But is there a better way to solve my logging problem above, for example that I've missed?
Implementation
Are you willing & able to help?
The text was updated successfully, but these errors were encountered: