potiuk commented on PR #31066: URL: https://github.com/apache/airflow/pull/31066#issuecomment-1539971138
> This PR bring consistency where by default it won't honor the standard naming convention (as-it has been) but optionally and at user's discretion they can opt in for standard naming conventions. This is also reflected in the unit tests, where no changes happened by default, only in the newly added tests when useStandardNaming is set to True. I am absolutely for consistency! I love that PR in general, in case it was not clear. It's just we have to care not only about the "target" but also how to guide our users to the "target" from current state, and possibly in the way that will not overwhelm is with "I upgraded to the new Helm Chart version from the previous one as usual and all hell broke loose" kind of issues :). Changing somethign is easy, but making it in the way that existing users wil not complain and have an easy path to follow is what I am worried about, and this is actually the GIST of that change and part of the change should be to consider the scenario, and have a ready answer for the users on what should they do. > To make this change non-disruptive, I've added on the next commit https://github.com/apache/airflow/commit/084b66cace565d758e2dfcef0d6d50d54be06b28 this change as an opt-in via an option useStandardNaming in values.yaml which defaults to False. We may consider setting that to true in the future? At least for new installations users could follow the standard naming conventions if they opt-in this. Yep. this is a good step I think, but I also think we need to be a bit more specific for the migration steps. The current proposal: > Use standard naming for all resources using airflow.fullname template Consider removing this later and default it to true to make this chart follow standard naming conventions using the fullname template. For now this is an opt-in switch for backwards compatiblity to leverage the standard naming convention and being able to use fully fullnameOverride and nameOverride in all resources For new installations - it is recommended to set it to True to follow standard naming conventions in all resources For existing installations, this will rename and redeploy your resources with the new names. Be aware that this will recreate your deployment/statefulsets along with their persistent volume claims and data storage migration may be needed to keep your old data This is cool, but I wonder if we can somehow be a more precise od what needs to be done (or maybe even just say don't do it - delete and recreate your installation if you want to use standard naming? - this is perfectly fine IMHO). Also, maybe we can even figure out a way to find out that someone did not have `useStandardNaming` set and fail such upgrade? That would be cool if we could do it. I would love `useStandardNaming` to be on by default. I have a feeling that by deferring setting the value to some unknown future, we are hiding the problem and not solving it. I.e. someone in the future will have to solve exactly the same problem of backwards compatibility, so either we solve it now, or let's not assume it will be easier in the future. I think if we don't set it on by default and find a good way how to address upgrades (even even by failing them if the value is nost set to false) then we are throwing the problem over the fence to future developers - who will have to bite the bullet. I would very much prefer to find a solution now. > Technically speaking old resources will be deleted by helm if these are renamed, so they will just be replaces with the new resources. The only thing we need to be careful is with the PVCs created by statefulset/deployments, as that means data needs to be moved from the old PVC to the new PVC. All right. I was not sure of that, but I did experiment and indeed helm nicely removed the old resources and added new ones, so that is cool. While doing the experiment noticed a few issues though (and I am sure our users will also do that, so I wanted to be able to go ahead and see what kind of problems they might have). 1. I used `airflow-override" fullnameOverride and installed airflow (from main). 2. Here are the list of resources I got: (got it by `helm install airflow ~/code/airflow/chart --dry-run | grep -E "^ name: airflow|^kind:"`) ``` kind: Secret name: airflow-fernet-key kind: Secret name: airflow-redis-password kind: Secret name: airflow-broker-url kind: Job name: airflow-create-user kind: Job name: airflow-run-airflow-migrations kind: ServiceAccount name: airflow-override-create-user-job kind: ServiceAccount name: airflow-override-migrate-database-job kind: ServiceAccount name: airflow-override-redis kind: ServiceAccount name: airflow-override-scheduler kind: ServiceAccount name: airflow-override-statsd kind: ServiceAccount name: airflow-override-triggerer kind: ServiceAccount name: airflow-override-webserver kind: ServiceAccount name: airflow-override-worker kind: Secret name: airflow-postgresql kind: Secret name: airflow-airflow-metadata kind: Secret name: airflow-webserver-secret-key kind: ConfigMap name: airflow-airflow-config kind: ConfigMap name: airflow-statsd kind: Role name: airflow-pod-launcher-role kind: Role name: airflow-pod-log-reader-role kind: RoleBinding name: airflow-pod-launcher-rolebinding name: airflow-pod-launcher-role kind: RoleBinding name: airflow-pod-log-reader-rolebinding name: airflow-pod-log-reader-role kind: Service name: airflow-postgresql-hl kind: Service name: airflow-postgresql kind: Service name: airflow-redis kind: Service name: airflow-statsd kind: Service name: airflow-triggerer kind: Service name: airflow-webserver kind: Service name: airflow-worker kind: Deployment name: airflow-scheduler kind: Deployment name: airflow-statsd kind: Deployment name: airflow-webserver kind: StatefulSet name: airflow-postgresql kind: StatefulSet name: airflow-redis kind: StatefulSet name: airflow-triggerer kind: StatefulSet name: airflow-worker ``` As you observed - lack of consistency. Not good. 2. I got your change (lates) and set "useStandardNaming": true. Nicely we get (as expected, and it's cool). With few small things - I tihnk `airflow-postgresgl*` and `airflow-airflow-config` should also be updated to follow the naming. ``` (kind-airflow-python-3.8-v1.23.17:KubernetesExecutor)> helm install airflow ~/code/airflow/chart --dry-run | grep -E "^ name: airflow|^kind:" kind: Secret name: airflow-override-fernet-key kind: Secret name: airflow-override-redis-password kind: Secret name: airflow-override-broker-url kind: Job name: airflow-override-create-user kind: Job name: airflow-override-run-airflow-migrations kind: ServiceAccount name: airflow-override-create-user-job kind: ServiceAccount name: airflow-override-migrate-database-job kind: ServiceAccount name: airflow-override-redis kind: ServiceAccount name: airflow-override-scheduler kind: ServiceAccount name: airflow-override-statsd kind: ServiceAccount name: airflow-override-triggerer kind: ServiceAccount name: airflow-override-webserver kind: ServiceAccount name: airflow-override-worker kind: Secret name: airflow-postgresql kind: Secret name: airflow-override-airflow-metadata kind: Secret name: airflow-override-webserver-secret-key kind: ConfigMap name: airflow-airflow-config kind: ConfigMap name: airflow-override-statsd kind: Role name: airflow-override-pod-launcher-role kind: Role name: airflow-override-pod-log-reader-role kind: RoleBinding name: airflow-override-pod-launcher-rolebinding name: airflow-override-pod-launcher-role kind: RoleBinding name: airflow-override-pod-log-reader-rolebinding name: airflow-override-pod-log-reader-role kind: Service name: airflow-postgresql-hl kind: Service name: airflow-postgresql kind: Service name: airflow-override-redis kind: Service name: airflow-override-statsd kind: Service name: airflow-override-triggerer kind: Service name: airflow-override-webserver kind: Service name: airflow-override-worker kind: Deployment name: airflow-override-scheduler kind: Deployment name: airflow-override-statsd kind: Deployment name: airflow-override-webserver kind: StatefulSet name: airflow-postgresql kind: StatefulSet name: airflow-override-redis kind: StatefulSet name: airflow-override-triggerer kind: StatefulSet name: airflow-override-worker ``` So far, so good. Run `helm upgrade`. Airflow installs but does not start. There are some config errors `CreateContainerConfigErrors` . I do not have time to investigate, but I presume it could be connected with the configmap name wrong?  -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
