Skip to content

Upgrade builder-datastore to use core/postgresql17 #1892

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

jasonheath
Copy link
Contributor

  • Adds components/builder-datastore/hooks/install
  • Removes components/builder-datastore/hooks/init
  • Updates components/builder-datastore/hooks/run
  • Uses core/postgresql17 as the package ident everywhere
  • Removes unused keys from components/builder-api/habitat/default.toml
  • Reworks components/builder-api/habitat/hooks/init
  • Updates or removes postgres conf items from postgresql.conf and default.toml files as appropriate
  • Updates changed items from postgresql.conf and applicabale default.toml files
  • Removes ext_semver_version from datastore plan

@jasonheath jasonheath changed the title Upgrade builder-datasore to use core/postgresql17 Upgrade builder-datastore to use core/postgresql17 May 14, 2025
@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch 2 times, most recently from 106f8fe to 70c5a7f Compare May 14, 2025 16:48
Copy link
Contributor

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

I'm still going through the install hook but thought I would put these comments out now.

Copy link
Contributor

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

I still feel like I need to go over this a little more but I'm running out of steam. One thing to consider is if we want to keep the dbinit and createdb in the init hooks. The advantage is that sometimes it's nice to blow away the data directory of builder-db and restart the instance to get a fresh clean instance. The init hooks will just recreate the empty database.

@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch 4 times, most recently from b3c4a65 to 2ac2e18 Compare May 16, 2025 17:42
Copy link
Contributor

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

Went over some of the install hook some more. My gut feeling is that we are trying to d oa little too much here or maybe just making it overly complex. Roughly it seems like it should be doing this:

Look at PG_VERSION
does it exist?
no? do nothing
is it less than our version?
no? - do nothing.
yes?
install older postgress
dump
initialize new postgres
load dump

@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch 2 times, most recently from 32c9588 to 26f83d4 Compare May 19, 2025 19:23
Copy link
Contributor

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

Went over some of the install hook some more. My gut feeling is that we are trying to d oa little too much here or maybe just making it overly complex. Roughly it seems like it should be doing this:

Look at PG_VERSION
does it exist?
no? do nothing
is it less than our version?
no? - do nothing.
yes?
install older postgress
dump
initialize new postgres
load dump

@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch 5 times, most recently from 6572f75 to 31d5687 Compare May 21, 2025 23:52
Copy link
Contributor

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

I only saw one thing that jumped out at me.

@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch 5 times, most recently from 6a2f95d to dc09dda Compare June 3, 2025 14:36
@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch 4 times, most recently from f75800b to 1b235d6 Compare June 9, 2025 20:08
@jasonheath
Copy link
Contributor Author

@jasonheath what is pending for this?

It's currently pending review and/or approval.

@@ -8,6 +8,9 @@ pkg_origin=habitat
pkg_maintainer="The Habitat Maintainers <[email protected]>"
pkg_license=('Apache-2.0')
pkg_bin_dirs=(bin)
pkg_include_dirs=(include)
pkg_lib_dirs=(lib lib64)
pkg_pconfig_dirs=(lib/pkgconfig lib64/pkgconfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

did we find these lines necessary? I'd think these would only be needed if we were building other binaries that had a dep on builder-api, which we don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing in a future commit. Also removing the pkg_bin_dirs because that's the default value.

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 removed those. I did go extra and remove the pkg_bin_dirs=(bin) too because that's the default value.

install_hab_pkg core/sed
install_hab_pkg core/sudo
install_hab_pkg core/zeromq
install_hab_pkg core/zlib
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it has quite a few more packages than we were using in the past (eg libb2, sed, etc). Are we sure we need all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With these I feel I can confidently say yes. Because of the way I was locking everything down I would occasionally hit something new a script that was trying to run. For instance I remember being surprised by the two you've happened to call out.

@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch from 0e80638 to c04f629 Compare June 18, 2025 19:42
jasonheath and others added 19 commits June 18, 2025 15:44
- Adds components/builder-datastore/hooks/install
- Removes components/builder-datastore/hooks/init
- Updates components/builder-datastore/hooks/run
- Uses core/postgresql17 as the package ident everywhere
- Removes unused keys from components/builder-api/habitat/default.toml
- Reworks components/builder-api/habitat/hooks/init
- Updates or removes postgres conf items from postgresql.conf and default.toml
  files as appropriate
- Updates changed items from postgresql.conf and applicabale default.toml files
- Removes ext_semver_version from datastore plan

Signed-off-by: Jason Heath <[email protected]>
I keep noticing this $(run) so I've removed it because it keeps catching
my attention and I've learned that it should have been removed several
years ago as part of a commit that removed the targets that built out
and used $(run).

Signed-off-by: Jason Heath <[email protected]>
- Fixes the environment for test/run_clippy restoring the lint step in
  the verify pipeline
- Fixes the environment for test/run_cargo_test restoring our unit test
  focused steps in the verify pipeline
- Fixes environment for .expeditor/scripts/verify/builder-api-functional.sh
  restoring our functional testing focused step in the verify pipeline
  - The hardcoding of core/rust/1.79.0/20250606210134 is done as part of
    this. When we upgrade rust we will restore our established
    'core/rust/"$(tail -n 1 "../../../rust-toolchain" | cut -d'"' -f 2)"'
  - Uses "dynamic_shared_memory_type = 'mmap'" for testing

- Updates .studiorc to make it rely on the environment variable that
  have been set
- Updates .studiorc such that the psql function can use
  core/postgresql17 or core/postgresql17-client depending on what's
  installed
- Updates .expeditor/scripts/post_habitat_release/cargo_update.sh to use
  shared build environment
- Adds a check for already running sup in .expeditor/templates/studiorc
- Updates ./build.sh to use shared build environment

Signed-off-by: Jason Heath <[email protected]>
@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch from c04f629 to feac6ca Compare June 18, 2025 19:45
@jasonheath jasonheath force-pushed the jah/postgresql@17-upgrade branch from feac6ca to 9b8a89e Compare June 18, 2025 20:12
# https://www.postgresql.org/support/versioning/
# https://endoflife.date/postgresql

read -ra installed_version <<<"$(sudo -u hab hab pkg exec "$INSTALLED_PG_IDENT" pg_config -- --version | awk '{print $2}')"
Copy link
Contributor

Choose a reason for hiding this comment

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

While upgrading (and this would probably be the case on a fresh install too), I get this error:

hab : user NOT in sudoers ; PWD=/hab/svc/builder-datastore ; USER=hab ; COMMAND=/usr/bin/hab pkg exec core/postgresql96 pg_config -- --version

This is going to fail on all sudo calls in the hook. This is run by the hab user that the supervisor uses for services which is not in sudoers so we need t ofigure out how to get this to work outside of sudo.

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

Successfully merging this pull request may close these issues.

3 participants