Skip to content

Conversation

@jedcunningham
Copy link
Member

@jedcunningham jedcunningham commented Jan 28, 2023

The recent postgres subchart upgrade had some issues: #29071

The biggest was the upgrade from PG 11 -> 15 resulted in the default password_encryption switching from md5 to scram-sha-256. Simply moving back to 11 is the easiest remediation, and is acceptable since it is the oldest version Airflow supports (as of today in 2.5).

Additionally, when updating the subchart, updating existing releases using built-in postgres was broken. As production use cases shouldn't be using built-in postgres, and it's easy to work around when hit, I've just documented the steps to proceed with the upgrade.

cc @snjypl

The recent postgres subchart upgrade had some issues.

The biggest was the upgrade from PG 11 -> 15 resulted in the default
`password_encryption` switching from `md5` to `scram-sha-256`. Simply
moving back to 11 is the easiest remediation, and is acceptable since it
is the oldest version Airflow supports (as of today in 2.5).

Additionally, when updating the subchart, updating existing releases
using built-in postges were breaking. As production use cases shouldn't
be using built-in postgres, and it's easy to work around when hit, I've
just documented the steps to proceed with the upgrade.
@dnskr
Copy link
Contributor

dnskr commented Jan 28, 2023

I recently upgraded Postgres instance, used by Airflow, from 11 to 15 version, so I would like to share my thoughts.

  1. pgbouncer.auth_type shoud be scram-sha-256 by default for Postgres 15, so current md5 value is a bug and should be fixed in values.yaml for Postgres 15 deployed by the subchart. Changing default value to scram-sha-256 fixes fresh installations with enabled pgbouncer, so, for example, the following command deploys the helm chart correctly:

    helm install test chart --set pgbouncer.enabled=true --set pgbouncer.auth_type=scram-sha-256
    
  2. Pinning subchart Postgres version to 11 is postponing upgrade problem for the future. Users who use postgres subchart (not recommended for production) need to pay this cost now or later anyway, for example when the oldest supported Postgres version will be 12. So I don't see any reason to use the oldest supported Postgres version in the helm chart by default, because default config should provide (in my opinion) "best minimal config" rather than "oldest compatible config".

I propose the following solution:

  • keep default Postgres version for postgres subchart
  • change pgbouncer.auth_type value to scram-sha-256 in values.yaml
  • provide suggestion how to migrate postgres data files from 11 to 15 using pg_upgrade or pg_dumpall (I think I can help here).

@potiuk
Copy link
Member

potiuk commented Jan 28, 2023

I propose the following solution:

Would you like to add a PR for that @dnskr ? I think what @jedcunningham is trying to do is to prepare a new relese of the Helm Chart soon. I think delaying it because the upgrade postgres to 15 with easy workaround is not a good idea - because we need to work out the migration paths and find out whether everything works, which I think will require an automated tests to be written and run with the Helm Chart. It's quite risky to release a new version of Helm Chart where we know our users might flood us with support issues (who will handle those?) if the migration is problematic.

I agree change from @jedcunningham is postponing the upgrade, but for a good reason, and from the description and findings of Jed it seems that this migration is far from straightforward.

So yes - it is a tactical solution for now, while there is nothing preventing us (you @dnskr ) to work on a parallel (strategic) change to upgrade Postgres. Postgres 11 is supported till November 2023, so we still have quite some time to complete it.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree it's a good tactical solution. While strategically we should think about adding migration (and testing scenarios of doing so) for Postgres 11 -> Postgres 15 as a separate PR.

@jedcunningham jedcunningham merged commit 43d7529 into apache:main Jan 30, 2023
@jedcunningham jedcunningham deleted the fix_pgbouncer_2 branch January 30, 2023 18:34
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:helm-chart Airflow Helm Chart changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants