-
Notifications
You must be signed in to change notification settings - Fork 210
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 Postgres 9.2 to 9.6 #1310
Conversation
# Note: the checkpoint_segments setting was removed. | ||
# https://www.postgresql.org/docs/9.6/static/release-9-5.html says | ||
# max_wal_size = (3 * checkpoint_segments) * 16MB | ||
# would be a usable conversion rule, but it also says the new setting's default |
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 defaults are sane according to several related docs I read (official, mailng list threads).
291a0e8
to
6e57566
Compare
I thought there was a problem when, for whatever reason, the upgrade process would fail; I think I saw a situation where we'd still get the sentinel file. However, I've tried reproducing this and couldn't (recorded a video to convince myself). |
e62713e
to
b41db1a
Compare
bdf676c
to
c15289e
Compare
want to give this another round of qa-chef-server-cluster tests, then I'll remove the |
2b4634a
to
2d7578f
Compare
🏓 @chef/chef-server-maintainers |
@@ -0,0 +1,62 @@ | |||
# |
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.
At this point it looks like we might bre able to use the omnibus-software definition of postgres? Or are we still stuck because of the version-specific prefix?
I'm wonder if we should just port that change over to the official definition? Or make an alternative official definition (postgresql-versioned.rb?) that mirrors the pg one except for install path?
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 good idea. I'll do that.
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.
⏩ chef/omnibus-software#845 Any hints on how to make this less copy-pasty are very welcome!
@@ -14,7 +14,12 @@ | |||
# limitations under the License. | |||
# | |||
|
|||
name "postgresql92" |
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.
Similarly, a shared postgres-versioned-bin-only
in omnibus-software?
# | ||
# pg_upgrade will helpfully generate a cleanup script which removes | ||
# the previous database cluster. This is nice, because we don't have | ||
# to worry about keeping track of the location of the old cluster. |
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.
Removing this gets messy because of enterprise/private/old-oss chef server installations that could be getting upgraded from.
I'm concerned that the cleanup of the old data will no longer happen - because the postgres data was not always in a versioned directory IIRC - there may be older install that have the path as /var/opt/opscode/postgresql/data instead of postgresql/9.2/data.
Perhaps updating the new postgres_upgrade_cleanup
to handle both would work?
This might also merit a modification of the upgrade pipeline to verify that old directories are removed.
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.
It's merely moved to a generic location -- because it's now for the cleanup of any postgres upgrade. The comments there made me think that this script will contain the old data dir, irregardless of whether it is /var/opt/opscode/postgresql/data
or versioned. (So it wouldn't care about going from 9.1 (unversioned) to 9.2, or 9.1 (unversioned) to 9.2, or 9.2 to 9.6.)
But I can look the left overs in the upgrade pipelines 👍
RELEASE_NOTES.md
Outdated
* [Upgrade to PostgreSQL 9.6](https://github.com/chef/chef-server/pull/1310), | ||
Chef Server now uses the latest stable version of the 9.6 series (9.6.3). Upgrades of existing | ||
installations are done automatically, but creating backups is advised. | ||
Note that the `checkpoint_segments` configuration setting is gone, so if you before have set |
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.
A preface like "The information below only applies if you have set a custom value for checkpoint_segments
in your /etc/opscode/chef-server.rb
. If you have not set a custom value, there is nothing to change" might make it clearer that most folks don't have to worry about this.
Separately:Maybe a preflight check against checkpoint_segments
being present?
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.
Yeah you're right. 🙄 (Just kidding. You simple are right there, and I'll add this 😉)
1328977
to
db8fef2
Compare
@marcparadise I've checked off all the things from my list now ✔️ Well, a missing bit is checking the deleted old data dir in upgrade tests. But if I find the right place for that test, it'll be in qa-chef-server-cluster anyways. So, could you throw some 👀 at those changes, please? I've intentionally not messed with the history yet, I'll squash and drop before merge. |
omnibus_overrides.rb
Outdated
@@ -10,3 +10,6 @@ | |||
override :rubygems, version: "2.6.8" | |||
# This SHA is the last commit before the 6.0 release | |||
override :'berkshelf-no-depselector', version: '6016ca10b2f46508b1b107264228668776f505d9' | |||
|
|||
override :'postgresql-bin-versioned', version: "9.2.21" # needed for upgrading |
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.
[minor] I wonder if we want to put the major version in the package name. That is, this package would be called postgresql92-bin
.
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.
That's actually what was there before -- but then commenters convinced me that there's a value in having postgres-bin-versioned
in omnibus-software, which can be used for any version we'd like to upgrade from. However, that bit hasn't passed review yet either
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.
("commenters" = @marcparadise here, and @sdelano in slack)
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.
Overall this looks good to me.
This looks good to me with the assumption that the omnibus/omnibus-software changes on your branches are merged upstream and we go back to following the master branch. |
@ryancragun yeah I'll take care of cross-testing those things before merging both tomorrow 😃 |
30c44fb
to
667118e
Compare
667118e
to
e27882f
Compare
Re: |
add1d29
to
c651468
Compare
Signed-off-by: Stephan Renatus <srenatus@chef.io>
7f226c8
to
6055bce
Compare
* warn if removed pg setting is configured * check requirements >= 9.2, not 9.6 Rationale: let's burn that incompatibility bridge when we get there. Currently, we ship 9.6 but we don't require any of its features. Fixes #441. Signed-off-by: Stephan Renatus <srenatus@chef.io>
remove checkpoint_segments, expose max_wal_size, min_wal_size, and checkpoint_flush_after. This follows the logic we've had before: all options regarding "Checkpoints" and "Archiving" (19.5.2 and 19.5.3 in https://www.postgresql.org/docs/9.6/static/runtime-config-wal.html) are exposed, and given their defaults as default attribute values. I have not filled in the values from 19.5.1, because we haven't done that before either. Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
I've moved that resource from post_11_upgrade_cleanup. While it would in just the same way from _there_, this should make the intention clearer. Also, we'll delete the old log files of 9.2 now. The addition the the release notes would theoretically be unnecessary since this is what the postinst[1] instructions say anyways. However, I believe it doesn't hurt to point it out again. [1] https://github.com/chef/chef-server/blob/master/omnibus/package-scripts/chef-server/postinst#L30-L52 Signed-off-by: Stephan Renatus <srenatus@chef.io>
This brings in changes from chef-backend (at 55caa8742b6). While that new old_data_dir handling doesn't depend on chef-server-running.json being written to disk successfully, I'm not convinced that the filter logic in that code actually works if there's more than one data dir on disk. Given that reviewers agree, I'll "backport" this change to chef-backend. (Maybe we should look into a shared cookbook again, though?) Signed-off-by: Stephan Renatus <srenatus@chef.io>
Signed-off-by: Stephan Renatus <srenatus@chef.io>
6055bce
to
664cc29
Compare
I looks like the old machinery made for going from 9.1 to 9.2 still works just fine! 🎉
Pending: