Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Squash database migrations#6802

Merged
zrhoffman merged 48 commits intoapache:masterfrom
ocket8888:db/squash-migrations
Jun 14, 2022
Merged

Squash database migrations#6802
zrhoffman merged 48 commits intoapache:masterfrom
ocket8888:db/squash-migrations

Conversation

@ocket8888
Copy link
Copy Markdown
Contributor

Squashes Traffic Ops and Traffic Vault migrations that were in 6.1.0 into their respective create_tables.sql scripts.

Fixes #6698


Which Traffic Control components are affected by this PR?

  • Traffic Ops
  • Traffic Vault

What is the best way to verify this PR?

Make sure nothing's broken. All existing tests should still pass.
Upgrade a TO system from 6.1.0 to master and verify the databases still work and all tests pass.

PR submission checklist

  • This PR has tests
  • This PR has documentation
  • This PR has a CHANGELOG.md entry
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added Traffic Ops related to Traffic Ops Traffic Vault related to Traffic Vault tech debt rework due to choosing easy/limited solution database relating to setup/installation/structure of the Traffic Ops database labels Apr 28, 2022
@ocket8888 ocket8888 force-pushed the db/squash-migrations branch from b9acea1 to 8e75d3e Compare May 4, 2022 14:48
@mitchell852 mitchell852 added this to the 7.0.0 milestone May 10, 2022
@mitchell852
Copy link
Copy Markdown
Member

sounds like this can only be merged after #6717

Copy link
Copy Markdown
Contributor

@mattjackson220 mattjackson220 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Will need #6717 to be merged first then this is good to go

@zrhoffman
Copy link
Copy Markdown
Member

Looks good to me! Will need #6717 to be merged first then this is good to go

#6717 is merged

Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

#6802 seems to add some columns that don't exist on the master branch

--- apache.sql  2022-05-16 12:34:55.666481904 -0600
+++ 6802.sql    2022-05-16 12:35:08.473392689 -0600
@@ -58,6 +58,8 @@
     deliveryservice text NOT NULL,
     cdn text NOT NULL,
     version text NOT NULL,
+    provider text NOT NULL,
+    expiration timestamp with time zone DEFAULT now() NOT NULL,
     last_updated timestamp with time zone DEFAULT now() NOT NULL
 );

@@ -102,7 +104,7 @@
 -- Data for Name: sslkey; Type: TABLE DATA; Schema: public; Owner: traffic_ops
 --

-COPY public.sslkey (data, deliveryservice, cdn, version, last_updated) FROM stdin;
+COPY public.sslkey (data, deliveryservice, cdn, version, provider, expiration, last_updated) FROM stdin;
 \.

Current test version in #6802 (comment)

Test (click to expand)

import_and_dump_tv_schema() {
    output_file="$1";
    <<'DOCKER_COMMANDS' docker run --rm -i \
        -v "$(pwd)/traffic_ops/app/db/trafficvault/create_tables.sql:/create_tables.sql" \
        -e POSTGRES_USER=traffic_ops \
        -e POSTGRES_PASSWORD=twelve \
        -e PGPASSWORD=twelve \
        -e POSTGRES_DB=traffic_ops \
        postgres:13.6-alpine bash >"$output_file"
    trap 'echo "Error on line ${LINENO} of ${0}" >/dev/stderr; exit 1' ERR
    set -o errexit
    docker-entrypoint.sh postgres >/dev/stderr &
    echo 'Waiting for Postgres to start...' >/dev/stderr
    sleep 10
    psql -Utraffic_ops -fcreate_tables.sql >/dev/stderr
    pg_dump -Utraffic_ops
DOCKER_COMMANDS
};
git checkout apache/master
import_and_dump_tv_schema apache.sql
git checkout apache/pull/6802/merge
import_and_dump_tv_schema 6802.sql
diff -u apache.sql 6802.sql

@ocket8888
Copy link
Copy Markdown
Contributor Author

You don't appear to have run the migrations based on that shell session

@zrhoffman
Copy link
Copy Markdown
Member

Oh true

Comment thread traffic_ops/app/db/create_tables.sql Outdated
Comment on lines 154 to 161
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

real indentation > spaces

but the rest of create_tables.sql is fake-indented, so it's consistent.

Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

When performing a db dump after running create_tables.sql, and the up migrations, role_id_seq sequence's currentval is different on this branch than on the master branch:

--- apache.sql	2022-05-16 13:22:03.797158150 -0600
+++ 6802.sql	2022-05-16 13:22:33.451033729 -0600
@@ -3563,7 +3279,7 @@
 -- Name: role_id_seq; Type: SEQUENCE SET; Schema: public; Owner: traffic_ops
 --
 
-SELECT pg_catalog.setval('public.role_id_seq', 7, true);
+SELECT pg_catalog.setval('public.role_id_seq', 1, false);
 
 
 --

Comment thread traffic_ops/app/db/migrations/2021110210085600_add_permissions.up.sql Outdated
Comment thread traffic_ops/app/db/migrations/2021110210085600_add_permissions.up.sql Outdated
Comment thread traffic_ops/app/db/create_tables.sql Outdated
Comment thread traffic_ops/app/db/create_tables.sql Outdated
Comment thread traffic_ops/app/db/trafficvault/create_tables.sql Outdated
Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

Also, the 00000000000000_init migrations existed in 6.1.x, so they should be removed, too. A user upgrading to ATC 7 will need to upgrade to 6.1 first, but IMO that is a reasonable requirement.

Comment thread traffic_ops/app/db/create_tables.sql Outdated
Comment thread traffic_ops/app/db/migrations/2021110210085600_add_permissions.up.sql Outdated
Comment thread traffic_ops/app/db/create_tables.sql Outdated
@ocket8888
Copy link
Copy Markdown
Contributor Author

IMO that's a reasonable requirement

It's also been a long-standing actual requirement, to my knowledge.

I didn't collapse those because I thought they had some special sauce that just always needed to exist. I'll rectify that.

@ocket8888 ocket8888 force-pushed the db/squash-migrations branch from 8362feb to 7b3b800 Compare May 20, 2022 21:10
Comment thread traffic_ops/app/db/create_tables.sql Outdated
Comment thread traffic_ops/app/db/create_tables.sql Outdated
Comment thread traffic_ops/app/db/trafficvault/migrations/2021042200000000_init.up.sql Outdated
@zrhoffman
Copy link
Copy Markdown
Member

zrhoffman commented May 20, 2022

To reference in another comment, this is the test I use to compare DB dumps from master and #6802 (click to expand)
import_and_dump_schema() {
    output_file="$1";
    <<'DOCKER_COMMANDS' docker run --rm -i \
        -v "$(pwd)/traffic_ops/app":/app \
        -w /app \
        -e POSTGRES_USER=traffic_ops \
        -e POSTGRES_PASSWORD=twelve \
        -e PGUSER=traffic_ops \
        -e PGPASSWORD=twelve \
        -e POSTGRES_DB=traffic_ops \
        postgres:13.6-alpine bash |
    set -o errexit
    (trap 'echo "Error on line ${LINENO} of Docker stdin"; exit 1' ERR
    docker-entrypoint.sh postgres &
    echo 'Waiting for Postgres to start...'
    sleep 10
    db/admin -env=production load_schema
    db/admin -env=production upgrade) >/dev/stderr
    pg_dump
DOCKER_COMMANDS
  sed -E 's/[0-9]{4}-[0-9]{2}-[0-9]{2} [0-9]{2}:[0-9]{2}:[0-9]{2}.[0-9]+\+[0-9]{2}/A_TIMESTAMP/g' >"$output_file"
};
CGO_ENABLED=0 go build -o traffic_ops/app/db/ traffic_ops/app/db/admin.go
git fetch apache
git checkout apache/master
import_and_dump_schema apache.sql
git checkout apache/pull/6802/merge
import_and_dump_schema 6802.sql
diff -u apache.sql 6802.sql

@ocket8888 ocket8888 force-pushed the db/squash-migrations branch 3 times, most recently from 992d4a0 to 4a64370 Compare May 24, 2022 20:21
@ocket8888 ocket8888 force-pushed the db/squash-migrations branch from 4a64370 to 7658a55 Compare June 1, 2022 16:19
Comment thread traffic_ops/app/db/create_tables.sql Outdated
@ocket8888 ocket8888 force-pushed the db/squash-migrations branch from 9420061 to fd74037 Compare June 2, 2022 23:03
Comment thread traffic_ops/app/db/create_tables.sql Outdated
@ocket8888 ocket8888 force-pushed the db/squash-migrations branch from fd74037 to 6aafee0 Compare June 14, 2022 20:41
Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

Looks good! The only non-white-space difference post-squash is that the admin role does not have some permissions

--- 1-apache    2022-06-14 15:47:29.501648659 -0600
+++ 2-6802      2022-06-14 15:47:33.371722449 -0600
@@ -1,7 +1,4 @@
 1      ALL     A_TIMESTAMP
-1      DNS-SEC:DELETE  A_TIMESTAMP
-1      DNS-SEC:READ    A_TIMESTAMP
-1      PARAMETER-SECURE:READ   A_TIMESTAMP
 2      ASN:READ        A_TIMESTAMP
 2      ASYNC-STATUS:READ       A_TIMESTAMP
 2      CACHE-GROUP:READ        A_TIMESTAMP

which should be okay, because it has the ALL permission.

@zrhoffman zrhoffman merged commit db64377 into apache:master Jun 14, 2022
@ocket8888 ocket8888 deleted the db/squash-migrations branch June 14, 2022 22:07
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
* Squash update_interfaces_multiple_routers

* Squash remove_tm_path

* Squash change_mtu_check_interfaces

* Squash cdn_notification

* Squash add_async_status_table

* Squash server_capability_update_cascade

* Squash cdn_notifications_multiple

* Squash generate_uuid_for_xmpp_id_where_empty

* Squash dsrs_originals

* Squash add_cdn_lock

* Squash ds_tls_versions

* Squash remove_ds_mso_alg

* Squash add_tm_user_last_authenticated

* Squash fix_indices

* Squash le_dns_challenge_xml_id

* Squash roles_and_permissions

* Squash no_null_username

* Squash add_job_start_time_idx

* Squash remove_user_role

* Squash add_permissions

* Squash refetch

* Squash admin_all_permission

* Squash add_secure_param_read_permission

* Squash remove_null_last_authenticated

* Squash add_expiration_and_provider_to_sslkeys

* Squash set_provider_not_null

* Fix cdn_notification primary key constraint name

* Fix sslkey table column order

* Fix incorrect column owner for cdn_notification_id_seq

* Fix incorrect cdn_notification table owner

* re-add now-unseeded Roles

* Fix erroneously dropping 'NOT NULL' constraint

* Squash init and trafficvault init

* Fix attempting to run migrations when there are no migrations

* unexport things that don't need to be exported

* Wrap errors, compare with errors.Is instead of ==, actually check errors

* Ensure terminating newlines from subprocess output

* print errors to stderr

* Don't use a print function that shouldn't be used

refer to https://go.dev/ref/spec#Bootstrapping

* Simplify

* Fix comments not ending with a period

* Fix segmentation fault in admin

* Fix incorrect initial db version in db tests

* Fix checking for wrong timestamp to find out if db is empty

* Remove duplicated script content

* Add back missing Permissions

* add back missing "portal" user Permissions

* Fix extra Permissions given to 'portal' users
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

database relating to setup/installation/structure of the Traffic Ops database tech debt rework due to choosing easy/limited solution Traffic Ops related to Traffic Ops Traffic Vault related to Traffic Vault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Squash database migrations

4 participants