Project

Profile

Help

Refactor #131

Move pulp.server.db.connection.initialize() calls exclusively to entry points

Added by bmbouter almost 7 years ago. Updated over 2 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
High
Assignee:
Category:
-
Sprint/Milestone:
-
Start date:
Due date:
% Done:

100%

Estimated time:
Platform Release:
2.8.0
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Pulp 2
Sprint:
March 2015
Quarter:

Description

The Problem
Throughout the production and test code for Pulp and its plugins there are many calls to pulp.server.db.connection.initialize(). Starting with Pulp 2.6.0, mongoengine caches the last initialized connection as the connection all database interaction will use. At test run-time can cause the testing database to mistakenly attach to the production database through liberal calls to connection.initialize() that occur without the server.conf being mocked. This causes horrible things to occur to Pulp development environments. Also, its sloppy to not have a structured way to connect to the database.

This also leads to a second problem which is that many of these calls to pulp.server.db.connection.initialize() are at the module level which means that simply by importing that code you are connecting to the database! A very unexpected side-effect that makes the above test-environment database switching occur depending on what you import. Ouch!

The goal
Move all of the pulp.server.db.connection.initialize() calls to the required entry point for all Pulp components to run. Each component that starts shall make exactly one call to pulp.server.db.connection.initialize() as part of the entry startup.

The Entry Points
We need to consider entry points for both the development and production uses where Pulp code will be run that need the database connection to already be initialized.

Steps
1. Each one of the entry point components above need to have exactly one call to pulp.server.db.connection.initialize()

2. All other calls to pulp.server.db.connection.initialize() needs to be removed. This includes all platform and all plugins. Search and destroy!

3. Rework pulp.server.db.connection.PulpCollectionFailure to be PulpDatabaseFailure instead.

4. pulp.server.db.connection.initialize() needs to have a behavior added that raises an Exception if it has already been called. This should enforce that a second call to initialize() will fail hard. Maybe have the exception live in the db.connection similar to PulpDatabaseFailure from step (3).

5. Add a behavior to get_collection() get_database() and get_connection() whereby if the database has not been initialized successfully a PulpDatabaseFailure (step 3) is raised. This will ensure that when the database is to be used initialize has been called at least 1 time.

6. Add test coverage for changes 2-5.

7. Fix up all unit tests likely by having them inherit from a new base class that will initialize the database as part of its setup. Many many unit tests will need to use this so that they receive a valid connection to the test database. A single call cannot be added to ./run-tests.py or pulp.devel.test_runner because the call to subprocess which actually runs the tests will not contain the database state.

Testing Concerns
Any testing code should rely on the testing entry point to provide the call to connection.initialize(). A base class approach should be used so that tests can continue to be run using nosetests directly.

  • The WSGI scripts are so simple they likely don't need to be tested at all.
  • The celery worker entry point is also very simple and can likely not be tested.
  • The celerybeat entry point can be tested because it is guarded by scheduler.setup_schedules method. We can mock the connection.initialize() before we call setup_schedules().

Related issues

Related to Pulp - Task #989: Stop pulp from handing out uninitialized database connectionsCLOSED - WONTFIX

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

Associated revisions

Revision d17e9e5c View on GitHub
Added by rbarlow over 6 years ago

Ensure that tests share a single DB conenction.

Many of our tests independently connected to the database, and they did not always ensure that they were connecting to the test database. This commit fixes the tests so they all use the test database.

Additionally, this commit refactors all of the entry points such that the connection is no longer started as a side effect of importing a module. Most of our code already worked this way, but this commit refactors the Celery workers to start the database connection using a signal instead of through an import.

This commit also makes it an error for the connection to be initialized more than once.

https://pulp.plan.io/issues/131 https://pulp.plan.io/issues/460

fixes #131 fixes #460

Revision d17e9e5c View on GitHub
Added by rbarlow over 6 years ago

Ensure that tests share a single DB conenction.

Many of our tests independently connected to the database, and they did not always ensure that they were connecting to the test database. This commit fixes the tests so they all use the test database.

Additionally, this commit refactors all of the entry points such that the connection is no longer started as a side effect of importing a module. Most of our code already worked this way, but this commit refactors the Celery workers to start the database connection using a signal instead of through an import.

This commit also makes it an error for the connection to be initialized more than once.

https://pulp.plan.io/issues/131 https://pulp.plan.io/issues/460

fixes #131 fixes #460

Revision 39da8a91 View on GitHub
Added by rbarlow over 6 years ago

Initialize the DB connection in the test package.

Use the test package to start the database connection, rather than relying on test superclasses to do it. This way, only one database connection will happen during the unit tests rather than one per test class.

Additionally, mock all instances of alterations to the server config during testing.

https://pulp.plan.io/issues/131

re #131 re #940

History

#1 Updated by bmbouter almost 7 years ago

  • Description updated (diff)

#2 Updated by rbarlow almost 7 years ago

On 01/22/2015 03:31 PM, Brian Bouterse wrote:

  • The test runner used by platform and all plugins
    pulp.devel.test_runner.run_tests

<https://github.com/pulp/pulp/blob/master/devel/pulp/devel/test_runner.py#L13>

Since the test runner is not the same process as the tests, it cannot
make the call to initialize().

#3 Updated by bmbouter almost 7 years ago

  • Description updated (diff)

@rbarlow, you are right about this. Because subprocess is used any pre-fork call to db.initialize() will not be kept around when the tests are run in a subprocess. I removed the step indicating that the test_runner should have the call to db.initialize(), and am instead suggesting that a base class be used by any test that needs the DB.

#4 Updated by bmbouter over 6 years ago

  • Tags Sprint Candidate added

#5 Updated by mhrivnak over 6 years ago

  • Priority changed from Normal to High

#6 Updated by cduryee over 6 years ago

  • Blocks deleted (Refactor #132: Ensure all tests can be run independantly)

#7 Updated by bmbouter over 6 years ago

  • Tags Groomed added
  • Tags deleted (Sprint Candidate)

#8 Updated by bmbouter over 6 years ago

  • Tags Sprint Candidate added

#9 Updated by rbarlow over 6 years ago

  • Assignee set to rbarlow
  • Sprint/Milestone set to 14
  • Platform Release set to master

#10 Updated by rbarlow over 6 years ago

  • Status changed from NEW to ASSIGNED

#11 Updated by rbarlow over 6 years ago

  • Platform Release changed from master to 2.7.0

#12 Updated by rbarlow over 6 years ago

  • Description updated (diff)
  • Platform Release changed from 2.7.0 to master

#13 Updated by bmbouter over 6 years ago

  • Groomed set to Yes
  • Tags deleted (Groomed)

#14 Updated by bmbouter over 6 years ago

  • Sprint Candidate set to Yes
  • Tags deleted (Sprint Candidate)

#15 Updated by rbarlow over 6 years ago

This pull request is going to be huge. Steps 3 and 5 are not really related to the Subject of this ticket. I don't disagree with making those changes, but I believe we should do them as a separate effort to reduce the complexity of what is happening already. Separating concerns would benefit us here!

#16 Updated by rbarlow over 6 years ago

I have submitted the platform PR for this[0], and now I will move on to testing the plugins against it to determine what else must be done.

[0] https://github.com/pulp/pulp/pull/1844

#17 Updated by rbarlow over 6 years ago

The following plugins' tests pass with these changes in platform:
pulp_docker
pulp_openstack
pulp_ostree
pulp_puppet
pulp_python

It seems that pulp_rpm is the only plugin that will need to be adjusted for the change in platform!

#18 Updated by bmbouter over 6 years ago

rbarlow wrote:

This pull request is going to be huge. Steps 3 and 5 are not really related to the Subject of this ticket. I don't disagree with making those changes, but I believe we should do them as a separate effort to reduce the complexity of what is happening already. Separating concerns would benefit us here!

@rbarlow Having a separation of concerns sounds good. Can new/tasks stories be made to track the work that isn't done with this issue?

#19 Updated by rbarlow over 6 years ago

This is the change for pulp_rpm:

https://github.com/pulp/pulp_rpm/pull/682

It will fail until the platform one is merged, but merging the platform one will also break pulp_rpm's current master. Therefore, there is no way to have the Jenkins tests pass on pulp_rpm without merging the platform code, which will in turn break pulp_rpm. Chicken and egg, etc.

#20 Updated by rbarlow over 6 years ago

On 05/05/2015 04:45 PM, Pulp wrote:

@rbarlow Having a separation of concerns sounds good. Can new/tasks
stories be made to track the work that isn't done with this issue?

They can be, but I'm not sure what the motivation for that change is as
it's not documented in this ticket. Perhaps it would be best if you
filed that ticket?

--
Randy Barlow

#21 Updated by rbarlow over 6 years ago

  • Status changed from ASSIGNED to POST

#22 Updated by rbarlow over 6 years ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#23 Updated by bmbouter over 6 years ago

  • Related to Task #989: Stop pulp from handing out uninitialized database connections added

#24 Updated by rbarlow about 6 years ago

  • Platform Release changed from master to 2.8.0

#25 Updated by dkliban@redhat.com almost 6 years ago

  • Status changed from MODIFIED to 5

#26 Updated by dkliban@redhat.com over 5 years ago

  • Status changed from 5 to CLOSED - CURRENTRELEASE

#27 Updated by bmbouter over 3 years ago

  • Sprint set to March 2015

#28 Updated by bmbouter over 3 years ago

  • Sprint/Milestone deleted (14)

#29 Updated by bmbouter over 2 years ago

  • Tags Pulp 2 added

Please register to edit this issue

Also available in: Atom PDF