Oracle support in Cloud Controller #44

Merged
merged 13 commits into from May 16, 2013

Conversation

Projects
None yet
4 participants
Contributor

youngm commented May 2, 2013

This pull request adds support for Oracle to the Cloud Controller.

This is a single pull request that includes and invalidates all of the other pull requests I've submitted over the last could of weeks. This PR is based on Master from a few days ago. So, there may need to be some additional merge work once this PR is conceptually approved.

This pull request is a lot to digest. If you want you can review the single feature PR that lead to this PR for some more concise context into some of the decisions I made in creating this PR. Here is a summary of the more notable changes I had to make and some context for why I made those changes:

Schema Changes

30 Character Identifier Limit

Oracle limits all identifiers to 30 characters. So, this required me to go through and give static names to all of the indexes and fk constraints that names larger than 30 characters.

In order to support Oracle with the existing migration scripts I had to modify the past migrations. I also added a new migration script that renames all of the fk constraints I had to shorted. This should mean that all the databases have the same fk names. However, I did not rename any of the indexes. So an existing database and a freshly created database may have different names for many of the indexes.

See: #38

Timestamp a reserved word

timestamp is a reserved word in Oracle. So I had to rename 2 instances of timestamp. I also added a migration script that will optionally rename these columns if they have the old name. This will bring existing and fresh schemas into alignment.

I also added the needed def_column_alias in the models so the API didn't change.

See: #38

No Boolean in Oracle

Oracle doesn't have a Boolean type. So, I had to change all of the Boolean column types to TrueClass. This allowed Sequel to use a char(1) for Boolean in Oracle.

However, Sequel doesn't appear to have yet fully implement support for Boolean=>char(1). To fully support char(1) as boolean I had to create a monkey patch that basically makes Sequel look at all char(1) columns as a boolean type similar to what convert_tinyint_to_bool does for mysql. This decision removes the future ability to use char(1) as a column type in the cloud controller. I'm open to other ideas if this is unacceptable.

Oracle doesn't support case insensitive columns

Support for using lower() function in where clause

Oracle doesn't have a way to support individual case insensitive columns. The standard approach in Oracle is to use the lower() or upper() function in queries. To support this I added a sequel plugin that essentially allows for declaring in the model what fields are case insensitive. Then I updated the query.rb rest api facilities to look for this identifier and modify the query to use lower() for those columns declared as case insensitive.

I then modified the existing case_insensitive Sequel monkey patch to allow the drivers to declare if they wish to use the "lower" function or rely upon a case insensitive column type.

Sqlite's case insensitive column type is buggy so I have configured it to use "lower()" for where queries but continue to use its case insensitive column type for db constraint purposes.

I set the default to use lower() for where clauses since it seems to be a rather cross compatible way to provide case insensitivity across databases.

I considered changed all databases in the Cloud Controller to use lower(). However, mysql and sqlite don't support function based indexes which would have caused performance and db constraint problems in mysql and sqlite. So I opted to just use lower() where queries in oracle and sqlite.

To test this I enhanced the provided API and model test examples to allow for the declaring of case insensitivity in those test examples as well. This significantly enhances the Cloud Controllers previous case insensitivity testing. It actually found a bug/typo in the :stacks model. I have included a fix for that in the migration scripts of this PR.

See: #41

Case Insensitive Constraints and Indexes

To maintain case insensitivity in constraints and indexes I added support for enabling the :case_insensitive flag for indexes. This will change the index to use a lower() function based index in databases that support it/need it.

Oracle Empty Sometimes Equals NULL

In an Oracle varchar if you insert empty it will translate empty to null. If you attempt to query empty it will not match null. This cased a problem for Route.host. Which uses empty to denote a non wildcard host and null is not allowed. This was actually the most difficult and frustrating of all the problems I came accross and I think the solution is the ugliest.

What I ended up doing was changing non wildcard Route.host to * and disallowed through validation and db constraints empty. I believe I did so while maintaining API backwards compatibility. I also added several tests to help ensure the functionality. I am totally open to other ways to solve this problem. For more context on how I got to this solution see: #40 and #42

Summary

I believe I've detailed above the more significant changes and work that I put into this endeavor. I think I've provided a pretty good solution for Oracle in the Cloud Controller and I think I've backed up that solution by adding tests that not only support the Oracle changes but I think improve the overall test coverage of the Cloud Controller.

My team has a deadline for supporting Oracle in our CF deployment by the middle of the May. It would be great if someone from the team could invest some time in reviewing and applying this PR by then. I am also happy to help in whatever way I can to provide CI support for Oracle. AWS provides Oracle support in RDS perhaps that can be utilized?

Thanks,
Mike

Owner

mkocher commented May 2, 2013

Thanks @youngm, we're a bit distracted this week as a team, but we'll make sure someone looks at it today or tomorrow.

Contributor

cf-frameworks commented May 2, 2013

Thanks a lot for this great PR. As Matthew Kocher said, we had a look at this and looks good so far but are burdened with a couple of inflight stories. Hopefully we will able to look at this soon, it is at the top of our backlog, after our current stories finish.

Contributor

youngm commented May 2, 2013

Thanks guys. That would be great.

Looking at the commits you guys sure have been busy. Looks like I have some merging ahead of me.

sigh...

@cf-frameworks cf-frameworks commented on the diff May 2, 2013

spec/models/helpers/unique_attributes.rb
+ ci_opts = dup_opts.clone
+ opts[:ci_attributes].each do |attribute|
+ ci_opts[attribute] = ci_opts[attribute].swapcase
+ end
+ ci_opts
+ end
+
+ it "should fail if different case" do
+ expect {
+ described_class.create do |instance|
+ instance.set_all(ci_dup_opts)
+ end
+ }.to raise_error Sequel::ValidationFailed, /#{sequel_exception_match}/
+ end
+ end
+
@cf-frameworks

cf-frameworks May 2, 2013

Contributor

In the latest commit we have moved away from doing nested shared examples to get rid of logic in test (3cb79bb). For example: https://github.com/cloudfoundry/cloud_controller_ng/blob/master/spec/api/route_spec.rb#L9.

It would be greatly appreciated if you could modify your tests to adhere to this new standard!

@austinbv && Tim

@youngm

youngm May 2, 2013

Contributor

K, I am in the middle of a merge on latest master now.

@cf-frameworks

cf-frameworks May 2, 2013

Contributor

Your Fast! When you finish merging let us know and we will review the Pull Request for a merge.

@austinbv && Tim

Contributor

youngm commented May 3, 2013

  • Merged with latest head.
  • Updated API tests to match new pattern
  • Added support for api queries against aliased columns with tests
  • Added renaming of too long indexes to the oracle migration script (inspired by the app_events rename script) Ugly code but will hopefully disappear when migrations are next flattened.
  • Travis now running and passing.

David Sabeti and Kowshik Prakasam and others added some commits May 7, 2013

David Sabeti and Kowshik Prakasam Orgs with paid quota can use 10mb rds instance [fixes #49420959]
Includes a significant refactor of Models::ServiceInstance#check_quota
and Models::Organization#service_plan_quota_remaining? methods.
3730a9f
David Sabeti and Kowshik Prakasam Remove hard-coding of trial_db identifier [finishes #49420959]
trial_db_guid is now configurable in the cc yaml with this stanza:

trial_db:
  guid: <trial db service_plan guid>
76caf60
Max Brunsfeld and Tony Hansmann Seed db with separate system and app domains
[#49220887]
ca54b2e
Max Brunsfeld and Tony Hansmann Allow setting system domain as an app domain 4b94069
Phan Le Update vcap_common and accept optional dashboard_url passed from the …
…gateway.
89f19e7
Gregg Van Hove and Tony Hansmann refactor to pull out some seed data stuff from the model classes bc6d93b
Gregg Van Hove and Tony Hansmann add the actual seeds 7d825cb
Gregg Van Hove and Tony Hansmann update schema to allow NilClass for system_domain_orginazation d53255d
Amit Gupta and Dmitriy Kalinin stager: add upload and download to the droplet and buildpack_cache ur…
…is so that nginx doesn't swallow download requests from deas
ee05314
Amit Gupta and Dmitriy Kalinin fixed quota definition flaky test df37de2
Mark Rushakoff and Tim Labeeuw Fix .ruby-version 7954755
@youngm youngm Merge remote-tracking branch 'github/master' into
OracleSupportSingleCommit

Conflicts:
	lib/cloud_controller/models/quota_definition.rb
	spec/models/quota_definition_spec.rb
3512da9
Contributor

youngm commented May 10, 2013

Merged in latest head. Hoping for some attention soon. :)

The commit log is getting a little messy. So, I may have to flatten again pretty soon.

Contributor

youngm commented May 10, 2013

I think CC has a problem with an unstable test. I think this commit is clear to merge even though Mysql failed in that last travis build.

Contributor

cf-frameworks commented May 10, 2013

Hi,

The travis build seems to be failing (https://travis-ci.org/cloudfoundry/cloud_controller_ng/builds/7060431). We will be adding an Oracle DB to Travis ASAP, to help you out for this PR. Sorry about the delay, we have rather busy this week but obviously have not forgotten about this PR!

  • Tim
Contributor

youngm commented May 10, 2013

Hi Tim,

I'm confident that failure is an unstable test in CC. As evidenced by the same or similar failures in:

https://travis-ci.org/cloudfoundry/cloud_controller_ng/builds/7059815
https://travis-ci.org/cloudfoundry/cloud_controller_ng/builds/7028158

Thanks for keeping in touch.

Mike

Contributor

cf-frameworks commented May 10, 2013

Thanks Mike,

We will look at these tests by ourselves. I'm working to add Oracle Travis right now. As soon as that passes will accept the merge.

Tim

Contributor

youngm commented May 10, 2013

That's awesome! let me know if you run into any trouble getting Oracle running.

@cf-bosh cf-bosh merged commit 7a372f6 into cloudfoundry:master May 16, 2013

1 check failed

default The Travis CI build failed
Details
Contributor

youngm commented May 16, 2013

Wahoo!!!! Let me know if you run into any trouble. I'll get right on it.

Contributor

cf-frameworks commented May 16, 2013

(@lookfirst & @kowshik): This has been merged to master. Thanks @youngm for your work!

The Travis servers are on the west coast and our Amazon Oracle instance is on the east coast. Due to network latency, tests run really slow (beyond that, the Oracle tests seem to run slowly when we ran tests on just the east coast AWS network). Therefore, we have setup travis to run a random 25% of all specs when the database is Oracle. We consider this an eventually consistent testing solution and hope you are ok with that. =)

Contributor

youngm commented May 16, 2013

I think running a random 25% of tests is a great solution. Oracle DDL operations are notoriously slow so I imagine that's the cause of most of the slowness. At least I hope my CC doesn't end up being 500% slower. :)

Contributor

cf-frameworks commented May 16, 2013

(@lookfirst & @kowshik):

@youngm - After noticing an issue with db migrations related to your changes, we decided to revert your pull request so that we don't block our internal processes. First, we have created a branch on master called 'oracle', then we are reverting your changes on master temporarily. Finally, we are going to fix the issue on the oracle branch and eventually re-merge to master.

Contributor

youngm commented May 16, 2013

ok. I'm happy to help if you need it. Just give me an error or something.

Let me know!

Contributor

cf-frameworks commented May 16, 2013

(@lookfirst & @kowshik):

@youngm We've talked about things here and it seems that we have some questions about how to best deal with migrating data in the context of accepting your commits. Would it be possible to do a conference call sometime tomorrow to discuss things? We've pre-scheduled a meeting for 10am PST. @ 415-777-4868 x2001 If that time/date doesn't work for you, let us know what works for you and we can certainly schedule something else. Thanks!

Contributor

youngm commented May 16, 2013

Yup, I'll be there.

Contributor

cf-frameworks commented May 16, 2013

Great thanks! We'll chat with you tomorrow.

Contributor

cf-frameworks commented May 16, 2013

Notes for discussion tomorrow (and maybe something for you to think about)... during our migrations we are seeing this error:

Tasks: TOP => db:migrate
(See full trace by running task with --trace)
rake aborted!
Mysql2::Error: Can't DROP 'quota_definitions_name_index'; check that column/key exists

Could it be that you missed something important here?

Contributor

youngm commented May 16, 2013

Ok, here is what is going on. I had to rename a number of indexes to shrink the name to conform to oracle's 30 char identifier limit.

In this script I attempt to go through and rename the old indexes to match the new shorter oracle name.

Because this index didn't have a name previously I'm relying on Sequel to derive what the default name might have been in the past so that I can subsiquently drop it and re-create it with the shorter name.

It appears that in one of your databases that index for some reason isn't using what Sequel thinks should be the default index name.

Do you know what the name of that index is in the database this migration is failing on? That will help me figure out why this index isn't using the default name.

Contributor

cf-frameworks commented May 17, 2013

It seems the pre-migration index name is qd_name_index, but your migration script is looking for quota_definitions_name_index

Contributor

cf-frameworks commented May 17, 2013

(@lookfirst & @kowshik ):

"Because this index didn't have a name previously I'm relying on Sequel to derive what the default name might have been in the past" .. Actually, the index did have a name previously. It can be found here: https://github.com/cloudfoundry/cloud_controller_ng/blob/oracle/db/migrations/20130131184954_new_initial_schema.rb#L67.

Contributor

youngm commented May 17, 2013

I had to give it a name retroactively in that migration script so that these scripts would run at all on Oracle. Here is what the script had before:

https://github.com/cloudfoundry/cloud_controller_ng/blob/master/db/migrations/20130131184954_new_initial_schema.rb#L66

Are you getting this error from an existing db or fresh db?

This is the line of code that should be stopping the rename from happening if the index already has that name (is a new database):

https://github.com/cloudfoundry/cloud_controller_ng/blob/oracle/db/migrations/20130429173703_support_oracle_migrations.rb#L26

Contributor

cf-frameworks commented May 17, 2013

We are getting this error when trying to migrate from a clean fresh database that is built from code before your changes on your branch to code that is from your branch. In other words, starting with master before we merged to migrating to the oracle branch.

Contributor

youngm commented May 17, 2013

Hmm....I thought I'd tested that scenario. Perhaps something changed. I'll try and duplicate that scenario before our call on Friday.

Thanks!
Mike

Contributor

youngm commented May 17, 2013

Some progress before our call:

I looked at it for a bit this morning and I was unfortunately unable to duplicate the error you are getting running mysql 5.5.29.

I did however find a different problem. :crash_events is a table added prior to the oracle migrations. In the Oracle migrations I need to rename the "timestamp" column to "event_timestamp". However, after the Oracle migration Crash Events gets renamed to :app_events. Because of this if :crash_events is renamed prior to oracle scripts running then the oracle script will fail to rename :crash_events.timestamp because it is not :app_events.timestamp. To fix this I've moved the Oracle migration to after the :crash_events rename.

Another small change I made was to convert the index name to a Symbol (https://github.com/youngm/cloud_controller_ng/blob/master/db/migrations/20130517093003_support_oracle_migrations.rb#L26) since I was leary of the string to Symbol comparison I was doing before.

However, this would not cause the error that you were seeing.

In my attempt to duplicate your error what I did was:

  • Fresh database
  • Clone https://github.com/youngm/cloud_controller_ng
  • Checkout commit 7c2cd379e68209e4ea2a5b08d6ab37a2622167f9
  • Run bundle exec rake db:migrate against mysql
  • Checkout master e96dca901d7e61e284b25ea1d5024026f2d77585
  • Run bundle exec rake db:migrate against mysql again.
  • This didn't create the error you were getting.

I also tried:

  • Fresh database
  • Clone and checkout master from https://github.com/youngm/cloud_controller_ng
  • Remove the support_oracle_migrations.rb script
  • Run bundle exec rake db:migrate against mysql
  • Add back the support_oracle_migrations.rb script
  • Run bundle exec rake db:migrate against mysql
  • This also worked for me.

Any other ideas on how I might be able to duplicate your problem? What results do you see when you execute those steps above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment