Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade bitnami/postgresql subchart to 13.2.24 #36156

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

dnskr
Copy link
Contributor

@dnskr dnskr commented Dec 10, 2023

The PR upgrades postgresql dependency of Airflow helm chart:

  • bitnami/postgresql subchart upgraded from 12.10.0 to 13.2.24
  • PostgreSQL upgraded from PostgreSQL 11 to PostgreSQL 16.1.0 (default in bitnami/postgresql:13.2.24)

The change requires bitnami/postgresql subchart users (not intended for production) to perform manual major version upgrade using pg_dumpall or pg_upgrade.

closes: #34817


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Dec 10, 2023
@dnskr dnskr force-pushed the upgrade-bitnami-postgresql-subchart branch from e546cc0 to 61600f9 Compare December 10, 2023 18:53
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

I will check the failure in bandit check

@hussein-awala
Copy link
Member

it was fixed by #36162

@dnskr dnskr force-pushed the upgrade-bitnami-postgresql-subchart branch from 072b2b7 to 4a72d38 Compare December 11, 2023 20:32
@dnskr
Copy link
Contributor Author

dnskr commented Dec 11, 2023

Rebased onto main branch to include fix #36169.

Comment on lines -2280 to -2281
image:
tag: "11"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better use specific major Postgres version here, rather than use default from bitnami/postgres subchart.

  image:
    tag: "16"

16 - referenced to the latest minor version of bitnami/postgresql of Postgres 16, it is in opposite of 16.1.0-debian-11-r15 which are referenced to the specific major.minor version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review.
There is a good practice to use specific version rather than any sort of latest version as far as I know. Specific version provides better guarantees for deployment to be reproduceable and clear expectations on what exactly should be deployed:

  1. Two bitnami/postgresql deployments with {{ .Values.image.tag }} = 16 might run different database versions. For instance, today it deploys 16.1.0-debian-11-r15, but few days ago 16.1.0-debian-11-r13 was deployed with the same values.yaml file.
  2. The deployment with {{ .Values.image.tag }} = 16 not necessarily runs latest 16 version. For instance, because the chart was deployed a week ago when 16.1.0-debian-11-r15 didn't exist.
  3. Fresh deployment with {{ .Values.image.tag }} = 16 not necessarily runs latest 16 version, because docker image with tag 16 might be presented in host from previous or parallel deployments.

Also, default value from bitnami/postgres was used before the #29207 where 11 version was pinned to resolve the upgrade issue temporary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Differences between minor version of Postgres is a bug fixes include security one, data files are binary compatible between same major version.

So if only pin to the specific version then all users who doesn't care about specify version they would be get one of the first release of 16 Postgres, without fixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, this only my suggestion, but not strong opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. Let's see what others think.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine.. Postrgres chart is only used for testing/dev so we can get the default one.

@dnskr
Copy link
Contributor Author

dnskr commented Dec 21, 2023

@potiuk @jedcunningham Could you please have a look at the changes?
It would be great if you can review it, because you both were involved in the previous changes and discussions about the issue.

Comment on lines -2280 to -2281
image:
tag: "11"
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine.. Postrgres chart is only used for testing/dev so we can get the default one.

@potiuk potiuk merged commit 55f421b into apache:main Dec 21, 2023
58 checks passed
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 10, 2024
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.

Upgrade Helm chart dependency to bitnami/postgresql 13.x.x and PostgreSQL 16
5 participants