-
Notifications
You must be signed in to change notification settings - Fork 101
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
db upgrage: upgrade postgres 15 to postgres 16 #1415
base: master
Are you sure you want to change the base?
Conversation
@vh05, can you please improve the PR description? What are the changes, technical details on the env used, etc. |
sure |
ec79248
to
009642f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that instead of deciding on upgrade and applying changes inside SetDesiredNooBaaDB
it will be more manageable, easy to follow, and less error-prone if you implement an UpgradeDB
function that will be performed before the DB reconciliation. This method will ensure that everything is upgraded correctly, and will only return nil if the reconciliation is ready to proceed.
Sure Danny. Let me check and do the changes |
999ccc6
to
e0c629d
Compare
a7aa91c
to
2155c02
Compare
pkg/system/phase2_creating.go
Outdated
// Note: Intention of the below call is to fetch the desired image from Noobaa CR or env var | ||
// Passing empty Arg "" to GetDesiredDBImage() in the below case | ||
desiredImage := GetDesiredDBImage(r.NooBaa, "") | ||
if desiredImage != options.DBImage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this condition. options.DBImage
is hardcoded to the upstream DB image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only going ahead with upgrade if the desired image is what is provided in options.DBImage
. If not we will continue with the desired image. With this condition, we only upgrade to PG16
.
Do we have to consider upgrade whateven the desired image comes to be ?
ex: If desired image comes to be as postgres15
, Do we have to consider postgres15
for upgrade by checking against the sts image ? not the options.DBImage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the downstream the desired image will definitely be different than quay.io/sclorg/postgresql-16-c9s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, on downstream, it will be registry.redhat.io/rhel9/postgresql-16
For downstream build, dont we use above image for options.DBImage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. I thought we will go for whatever is there in options.DBImage for upgrade. Got clarity after discussing with you that we have to match the noobaa db image and use the image given in the noobaa CR. Updating the PR according to that
5307eec
to
7873068
Compare
pkg/system/phase2_creating.go
Outdated
// Note: Intention of the below call is to fetch the desired image from Noobaa CR or env var | ||
// Passing empty Arg "" to GetDesiredDBImage() in the below case | ||
desiredImage := GetDesiredDBImage(r.NooBaa, "") | ||
if !strings.Contains(desiredImage, "postgresql-16") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the name of the DS image? can we rely on this comparison?
As an alternative for the image name comparison, you can first compare the desired image to the one set in the DB STS:
- if the images are the same, nothing to do
- if they differ AND the current major version is not 16 yet, start the upgrade and modify the upgrade phase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both upstream and downstream image names contain the string postgresql-16"
upstream: quay.io/sclorg/postgresql-16-c9s
downstream:registry.redhat.io/rhel9/postgresql-16
We are doing the alternative image comparison in Switch case: ""
, function named: isDBUpToDate()
.
Even if we remove this, upgrade will be checked in case: ""
I will rename it to something like isPostgressUpgradeReqd()
and use it as appliable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isPostgresUpgradeReqd()
does
if the images are the same, nothing to do
if they differ AND the current major version is not 16 yet, start the upgrade and modify the upgrade phase
This check is done in 2 cases
- case "": This is for the 1st time when we upgrade to the new build, the
PostgresUpdatePhase
is empty - case "UpgradePhaseNone": This check is done everytime until the Postgres is upgrade to 16. As soon as desired image is
postgres-16
, we upgrade topostgres-16
and set the status to upgrade finished.
pkg/system/phase2_creating.go
Outdated
// Check the desired image, if it is not as same as db image to upgrade, | ||
// don't consider for upgrade | ||
// Note: Intention of the below call is to fetch the desired image from Noobaa CR or env var | ||
// Passing empty Arg "" to GetDesiredDBImage() in the below case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a first condition, check the postgres major version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is added in isPostgresUpgradeReqd()
270cfd9
to
88d449c
Compare
The Postgres upgrade from 15 to 16 or any later versions can be done by adding the env variable named POSTGRES_UPGRADE = copy in the pg db pod. More about upgrade process is here: https://www.postgresql.org/docs/current/pgupgrade.html In our case, we achieve the upgrade by setting this env var in noobaa-db-pg STS. Once after upgrade is done, we have to remove the POSTGRES_UPGRADE env from the STS. Otherwise pg db pod fails to come up because the db pod check PG directory version against upgrade image version and exits if there is no upgrade required and POSTGRES_UPGRADE is still provided. To remove this env, we store the upgrade status in noobaa-core CR after the PG upgrade and refer the same status during db reconcile. If the upgrade is already done, we remove this env from desired sts status. This will help to upgrade the db smoothly. Signed-off-by: Vinayak Hariharmath <[email protected]>
The Postgres upgrade from 15 to 16 or any later versions can be done by adding the env variable named
POSTGRES_UPGRADE = copy
in the pg db pod.More about upgrade process is here: https://www.postgresql.org/docs/current/pgupgrade.html
In our case, we achieve the upgrade by setting this env var in
noobaa-db-pg
STS.Once after upgrade is done, we have to remove the
POSTGRES_UPGRADE
env from the STS. Otherwise pg db pod fails to come up because the db pod check PG directory version against upgrade image version and exits if there is no upgrade required andPOSTGRES_UPGRADE
is still provided.To remove this env, we store the upgrade status in
noobaa-core
CR after the PG upgrade and refer the same status during db reconcile. If the upgrade is already done, we remove this env from desired sts status.This will help to upgrade the db smoothly