Skip to content

CLOUDSTACK-10012: Load SQL schema scripts from JAR - #2247

Merged
yadvr merged 2 commits into
apache:masterfrom
exoscale:CLOUDSTACK-10012-exoscale
Dec 19, 2017
Merged

CLOUDSTACK-10012: Load SQL schema scripts from JAR#2247
yadvr merged 2 commits into
apache:masterfrom
exoscale:CLOUDSTACK-10012-exoscale

Conversation

@marcaurele

@marcaurelemarcaurele commented Aug 24, 2017

Copy link
Copy Markdown
Member

Load the SQL schema files from the fatjar directly to simply file path discovery.

@yadvr

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖centos6 ✖centos7 ✔debian. JID-1025

@yadvr

Copy link
Copy Markdown
Member

@marcaurele it might be an issue as cloudstack-setup-databases reads from that folder, and also it will become an issue to verify the changes in various older upgrade paths. Let me review and get back to you on this next week.

@borisstoyanovborisstoyanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@marcaurele can you please edit the title to start with the JIRA number

@marcaurelemarcaurele changed the title Load SQL schema scripts from JARCLOUDSTACK-10012: Load SQL schema scripts from JARSep 8, 2017
@marcaurele

Copy link
Copy Markdown
MemberAuthor

From our experience on this change that is running in production for us, we can deploy a new SQL schema/lab with the usual mvn developer script, as well as upgrading the DB on schema changes. If a customization is needed, we should invert the way the upgrades are executed and do a package scan on classes rather than declaring them "statically" in DatabaseUpgradeChecker. It should help anyone to extend an upgrade class to add extra changes. The upgrade classes should declare themselves the version from which they should be run, and the DB version after the upgrade. With the help of a Comparable, we could sort them in the correct order, and even skip over unneeded ones if a class declares a bigger upgrade path.

@yadvr

Copy link
Copy Markdown
Member

Thanks @marcaurele there are several conflicts and large changes, I'll work with you on this after the original fat-jar PR is tested and merged.

@asfgit
asfgitforce-pushed the CLOUDSTACK-10012 branch 10 times, most recently from d6a3dbf to a04da57CompareNovember 3, 2017 14:11
@yadvr

yadvr commented Nov 3, 2017

Copy link
Copy Markdown
Member

@marcaurele with the fatjar PR merged, please rebase and switch the base branch of this PR to master. Let's get this reviewed, tested and merged as next step. Thanks.

@marcaurele
marcaurele changed the base branch from CLOUDSTACK-10012 to masterNovember 7, 2017 10:10
@marcaurele
marcaureleforce-pushed the CLOUDSTACK-10012-exoscale branch 2 times, most recently from 6a8eba7 to 0e6e030CompareNovember 7, 2017 10:32
@marcaurele

Copy link
Copy Markdown
MemberAuthor

@rhtyd rebase done + target branch changed

@yadvr

yadvr commented Nov 7, 2017

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@yadvr

yadvr commented Nov 7, 2017

Copy link
Copy Markdown
Member

@marcaurele this will require two additional changes:

  • Fix cloud(stack)-setup-databases script to use com.cloud.upgrade.DatabaseUpgradeChecker and remove assumptions that a setup folder will be available to read/run the sql scripts from. We'll need to do this while making sure cloudstack-setup-databases is backward compatible.
  • Fix various packaging scripts to not copy/bundle setup as a separate directory

I don't have time this week, but can collaborate with you next week. I think I certainly can fix the packging bits.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✖centos6 ✖centos7 ✔debian. JID-1219

@marcaurele

Copy link
Copy Markdown
MemberAuthor

@rhtyd cloud-setup-databases is good to go as it is since I left the SQL files in setup/db directory to create the schema. Only the migration files are moved into the JAR. I found one trace of this sub directory in the centos packaging and remove the lines.

@marcaurele

Copy link
Copy Markdown
MemberAuthor

@rhtyd all checks are green, can you have a look or push further testing on this branch.

@yadvr

yadvr commented Dec 8, 2017

Copy link
Copy Markdown
Member

Thanks @marcaurele - I'm wondering if it's still a good idea to keep the subdirectory with initial seed schema files while having the upgrade paths shaded/included in jar? I'll kick some tests.
@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1353

@yadvryadvr added this to the 4.11 milestone Dec 10, 2017
@yadvr

Copy link
Copy Markdown
Member

@marcaurele can you fix the conflicts? I'll help run tests then.

@marcaurele
marcaureleforce-pushed the CLOUDSTACK-10012-exoscale branch from 877a8f8 to 53d6089CompareDecember 18, 2017 07:52
@marcaurele

Copy link
Copy Markdown
MemberAuthor

@rhtyd rebase done

@yadvr

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1404

@yadvr

Copy link
Copy Markdown
Member

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-1818)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31511 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2247-t1818-kvm-centos7.zip
Smoke tests completed. 63 look OK, 4 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File
test_01_vpc_privategw_aclFailure61.22test_privategw_acl.py
test_02_vpc_privategw_static_routesFailure287.69test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanupFailure162.67test_privategw_acl.py
test_04_rvpc_privategw_static_routesFailure378.41test_privategw_acl.py
test_02_create_template_with_checksum_sha1Error5.14test_templates.py
test_03_create_template_with_checksum_sha256Error5.14test_templates.py
test_04_create_template_with_checksum_md5Error5.14test_templates.py
test_01_volume_usageFailure316.04test_usage.py
test_01_vpc_remote_access_vpnError60.60test_vpc_vpn.py

@yadvryadvr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, +1 on initial phase to include all sql upgrade paths in fatjar. Including the default schema/sql files and using the db creator class could be next step for future.

@yadvr
yadvr merged commit a372040 into apache:masterDec 19, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@marcaurele@yadvr@blueorangutan@borisstoyanov