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

feat: Add the possibility to specify the used PostgreSQL version #75

Merged
merged 3 commits into from
May 25, 2024

Conversation

ZPascal
Copy link
Contributor

@ZPascal ZPascal commented Nov 28, 2023

fix: #74

TODO:

  • Adjust the docs
  • Adjust the version bump automation
  • Test the core functionality
    • Test PostgreSQL 13
    • Test PostgreSQL 15
    • Test PostgreSQL 16
  • Test from PostgreSQL 13 to 15
  • Check if a downgrade is possible -> Not possible

I'm currently working with @Jobsby together on the topic.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186569819

The labels on this github issue will be updated when the story is started.

@ZPascal
Copy link
Contributor Author

ZPascal commented Dec 3, 2023

Hi @jpalermo & community, we have implemented most of the new features and now have a few issues with the old features and updating the minor versions in general. I'm just asking myself the following questions:

  • Do you think it should be possible to specify the used minor version of PostgreSQL in this new feature?
    • From my understanding, the user only wants to specify the major version of PostgreSQL, what do you think?
  • Do you think we still need the minor version update?
  • Shouldn't this be done automatically by using a new blob/release version?

@jpalermo
Copy link
Member

jpalermo commented Dec 5, 2023

  • Specifying a minor would be strange/impossible since we only keep around the blobs for the latest minor of each supported major. It would make the release huge to keep around all the minors.
  • The minor version update is still needed when a patch is released right? Like how we updated from 15.4 to 15.5 in the most recent release, this function would have been run.
  • Yeah, the minor upgrade will happen during the first pre-start once the new release has been deployed.

*feat: Adjust the docs and the release process
@ZPascal ZPascal marked this pull request as ready for review May 19, 2024 18:57
@ZPascal
Copy link
Contributor Author

ZPascal commented May 20, 2024

@jpalermo Could you please have a look?

ci/tasks/bump-postgres-packages/task.sh Show resolved Hide resolved
jobs/bbr-postgres-db/templates/config.sh.erb Outdated Show resolved Hide resolved
jobs/postgres/spec Outdated Show resolved Hide resolved
ci/pipeline.yml Outdated Show resolved Hide resolved
ci/tasks/bump-yq-packages/task.sh Outdated Show resolved Hide resolved
ci/tasks/bump-yq-packages/task.sh Outdated Show resolved Hide resolved
packages/yq-4/packaging Outdated Show resolved Hide resolved
@ZPascal
Copy link
Contributor Author

ZPascal commented May 24, 2024

@jpalermo Could you please have a look again? I've added your recommendations.

@ZPascal ZPascal requested a review from jpalermo May 24, 2024 12:57
Copy link
Member

@jpalermo jpalermo left a comment

Choose a reason for hiding this comment

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

Seems good, I'll merge and update the pipeline

@jpalermo jpalermo merged commit 6f91866 into cloudfoundry:main May 25, 2024
1 check passed
@ZPascal ZPascal deleted the issue-74 branch May 26, 2024 11:44
@klakin-pivotal
Copy link

Hey @ZPascal , was it your intention to exit 1 (which I think will terminate the entire script) from the function here:

https://github.com/cloudfoundry/postgres-release/pull/75/files#diff-c177d803deec7dfa3f331f9dac76ddc450207164b97343dd51bd51ab6b545755R71-R77

and here:

https://github.com/cloudfoundry/postgres-release/pull/75/files#diff-494db505a21234c6f1ad9c9fc80efe3f168bf9f38e7d01809be23e0a7682f6f2R8-R14

rather than return 1 (which I think will simply signal the failure of the function)?

NB: My Bash knowledge is not great, but this part of the PR looked incorrect to me. It may be that I am the one who is incorrect.

Context: I'm currently trying to figure out why the CI job that runs the Acceptance Tests for this Release has been failing ever since this Release was released.

I don't know that this is actually the problem, but I was looking around, and this didn't look right, so I thought I'd ask.

@ZPascal
Copy link
Contributor Author

ZPascal commented Jun 19, 2024

Hey @ZPascal , was it your intention to exit 1 (which I think will terminate the entire script) from the function here:

https://github.com/cloudfoundry/postgres-release/pull/75/files#diff-c177d803deec7dfa3f331f9dac76ddc450207164b97343dd51bd51ab6b545755R71-R77

and here:

https://github.com/cloudfoundry/postgres-release/pull/75/files#diff-494db505a21234c6f1ad9c9fc80efe3f168bf9f38e7d01809be23e0a7682f6f2R8-R14

rather than return 1 (which I think will simply signal the failure of the function)?

NB: My Bash knowledge is not great, but this part of the PR looked incorrect to me. It may be that I am the one who is incorrect.

Context: I'm currently trying to figure out why the CI job that runs the Acceptance Tests for this Release has been failing ever since this Release was released.

I don't know that this is actually the problem, but I was looking around, and this didn't look right, so I thought I'd ask.

Yes, it will terminate the script and I think that's the right thing to do. If don't stop the process, It will downgrade the database and start rewriting the configuration files. The tool used for minor and major version upgrades is pg_upgrade, which does not support version downgrades.

FYI: I discussed the issue with @jpalermo in a Slack discussion and already tested the effects. If you don't stop the downgrade, you end up with a crashed instance and have to do a lot of work to restore the instance.

Can you please share the CI job issue of the acceptance test on your end? I will take a look and maybe we can solve the problem together.

@klakin-pivotal
Copy link

klakin-pivotal commented Jun 19, 2024

Oh, you work for SAP, so you probably have access to the Cloudfoundry Concourse instance... sorry about that, I should have checked before writing my question.

The most recent failing build (which fails in the same way as all the others) is here: https://bosh.ci.cloudfoundry.org/teams/main/pipelines/postgres-release/jobs/run-acceptance-tests/builds/96

The failure is very strange. The SSH attempts for both the "Validating the postgres-previous is not created" and "Validating the postgres-previous is created" tests in the upgrade_single_node_test.go file fail. Despite what it looks like, both tests are failing, but the "... is not created" test doesn't check to see that the exit status from ssh is NOT 255 (which indicates some sort of ssh failure, rather than failure of the command ssh ran), it just tests if the exit status was non-zero.

The build immediately before it succeeded. (https://bosh.ci.cloudfoundry.org/teams/main/pipelines/postgres-release/jobs/run-acceptance-tests/builds/95) For that build, I pinned back the postgres-release resource to 5e330a77ae691e37da4585c5e404d7a8acede23b, which is the commit just before Final Release 52 was released.

The release of Final Release 52 might be unrelated to the acceptance test failure... however, I do notice that part of the test involves an upgrade from the previous [most recent] Final Release to the code in the repo... which means that the commit that releases Final Release 52 was (at least for part of the test suite) the first time upgrading from the code in Release 52 was tested.

@ZPascal
Copy link
Contributor Author

ZPascal commented Jun 19, 2024

Oh, you work for SAP, so you probably have access to the Cloudfoundry Concourse instance... sorry about that, I should have checked before writing my question.

The most recent failing build (which fails in the same way as all the others) is here: https://bosh.ci.cloudfoundry.org/teams/main/pipelines/postgres-release/jobs/run-acceptance-tests/builds/96

@klakin-pivotal No, problem. I have access to the corresponding concourse instance, but not to the specific test pipeline itself. Do you know which profile or authorization is required?

The failure is very strange. The SSH attempts for both the "Validating the postgres-previous is not created" and "Validating the postgres-previous is created" tests in the upgrade_single_node_test.go file fail. Despite what it looks like, both tests are failing, but the "... is not created" test doesn't check to see that the exit status from ssh is NOT 255 (which indicates some sort of ssh failure, rather than failure of the command ssh ran), it just tests if the exit status was non-zero.

The build immediately before it succeeded. (https://bosh.ci.cloudfoundry.org/teams/main/pipelines/postgres-release/jobs/run-acceptance-tests/builds/95) For that build, I pinned back the postgres-release resource to 5e330a7, which is the commit just before Final Release 52 was released.

The release of Final Release 52 might be unrelated to the acceptance test failure... however, I do notice that part of the test involves an upgrade from the previous [most recent] Final Release to the code in the repo... which means that the commit that releases Final Release 52 was (at least for part of the test suite) the first time upgrading from the code in Release 52 was tested.

That sounds interesting and I would like to try it out. The structure of the test is particularly interesting for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants