Skip to content
This repository has been archived by the owner on Aug 22, 2022. It is now read-only.

OC-1541: Keep the old instance running while deploying a new one #63

Merged
merged 17 commits into from
May 19, 2016

Conversation

bradenmacdonald
Copy link
Member

@bradenmacdonald bradenmacdonald commented Apr 28, 2016

Changes:

  • Fix occasional pylint warning about undocumented Meta classes
  • Combine LogEntry classes
  • Separated [OpenEdX]Instance into Instance (long-lived, mutable, creates AppServers and marks one as active), and AppServer (immutable, owns one VM)
  • Factored Open edX specific code into OpenEdXInstance and OpenEdXAppServer respectively
  • Factored Open edX specific code out of the ansible mixin
  • Change the ansible mixin so it can optionally run multiple playbooks
  • Removed instance.last_provisioning_started since you can compute that from the most recent AppServer.created field
  • Combined forum_version, notifier_version, xqueue_version, and certs_version into a single openedx_release setting, since these were always set to the same value, and if one needs to be set differently it can be done using ansible_extra_settings anyways.
  • Separated PR-watching code to a separate app (OpenEdXInstance should not need to know about pull requests)
  • Updated API
  • Allows provisioning a new AppServer at any time
  • Updated UI accordingly
    • Lovely new home screen ("empty state"):
      screen shot 2016-05-08 at 7 54 01 pm
    • Instance list indicates the state of the active app server for each instance, as well as the state of any newer app server waiting to be provisioned / reviewed / made active.
      screen shot 2016-05-08 at 7 54 57 pm
    • New "Pull Request" tab:
      screen shot 2016-05-08 at 7 56 40 pm
    • When you click "Update Instance Settings from PR", it displays an animation while fetching updates from GitHub, then switches to the "Settings" tab to show you the new settings / commit hash.
    • New Settings tab for instance settings (should be made editable in the future)
      screen shot 2016-05-08 at 7 58 03 pm
    • New App Servers tab to review app servers, launch new ones, or change which one is active:
      screen shot 2016-05-08 at 7 59 37 pm
    • Deep linking to any instance or app server :)
  • Behavior: The first time a PR is seen, an instance will be auto-provisioned; on failure, the provisioning will be retried (on a new AppServer) up to 1 additional time; on success, the AppServer will automatically be marked active. On the other hand, whenever the user manually provisions a new instance or AppServer, there is no automatic retry or activation. (To give users time to inspect the errors or the new appserver.)

TODO:

  • Update Jasmine tests

Deployment notes:

  • Run this SQL before deploying to create a backup of the instance_singlevmopenedxinstance data.

Suggestions for future work:

  • Add tests for # of queries used by logging, and by each API view. Add in select_related() calls where needed to optimize
  • Allow stopping an AppServer build
  • Implement the WatchedPullRequest TODO items listed in the code



# pylint: disable=too-many-instance-attributes
class OpenEdXDatabasesMixin(MySQLInstanceMixin, MongoDBInstanceMixin, SwiftContainerInstanceMixin):
Copy link
Member

Choose a reason for hiding this comment

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

Why does the databases mixin contain Swift and S3?

Copy link
Member Author

Choose a reason for hiding this comment

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

SwiftContainerInstanceMixin was already in a module called database.py. I could rename this to "PersistenceMixin" and then it would make more sense. (Or I could split it up into DatabaseMixin / StorageMixin but I don't think that that would improve anything)

Copy link
Member

Choose a reason for hiding this comment

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

@bradenmacdonald I would be in favor of splitting up, the two are handled differently, even if they both persist data. It would also help to remove the pylint warning here, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

@antoviaque Ok, split up in f2eb2fd

@antoviaque
Copy link
Member

@bradenmacdonald Cool set of changes : ) I had a quick read through the diff and really liked what I saw. Much cleaner approach overall.

else:
return msg, kwargs

app_server = self.extra['obj']
Copy link
Member

Choose a reason for hiding this comment

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

@bradenmacdonald Looks like this portion of the code is not reachable.

Copy link
Member Author

Choose a reason for hiding this comment

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

@itsjeyd Whoops, thanks. I will fix. A lot of this code isn't actually running yet, so I haven't caught little things like that, but I would have :)

@bradenmacdonald
Copy link
Member Author

Rebased from 03b4b22 to 5e1b8dc in preparation for merge (code is identical).

it should be a git branch or tag that exists in all of those repositories.
* `DEFAULT_CONFIGURATION_REPO_URL` The repository containing the Open edX
ansible scripts to use. Defaults to
`'https://github.com/edx/configuration.git'`
Copy link
Member

Choose a reason for hiding this comment

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

@bradenmacdonald No need for the single quotes here. Also (nit): "ansible" ⟶ "Ansible". Sentence is missing a full stop.

@itsjeyd
Copy link
Member

itsjeyd commented May 18, 2016

@bradenmacdonald Thanks for updating the README! The changes look good, I only found one piece of info to be missing (as mentioned in this comment). Otherwise this is ready to go! 👍 🎆

@bradenmacdonald bradenmacdonald force-pushed the provision-while-running branch 2 times, most recently from 54f4873 to b7e860e Compare May 18, 2016 18:07
@bradenmacdonald
Copy link
Member Author

@itsjeyd Ok, I addressed your nits. Doing manual testing now, and then I'll merge.

@bradenmacdonald
Copy link
Member Author

Found one small bug during manual testing which I fixed with:
manual-testing

Now just waiting for the build to go green again (ppa python 2.7 mirror seems to be offline :-/ ), then I will hit merge.

@bradenmacdonald bradenmacdonald merged commit 52bf7a1 into master May 19, 2016
@bradenmacdonald bradenmacdonald deleted the provision-while-running branch May 19, 2016 00:51
@bradenmacdonald
Copy link
Member Author

🎉

@bradenmacdonald
Copy link
Member Author

I have deployed this to our stage server, and it seems to be working.

Note that for any migrated instances (created before this PR was deployed), all the ansible logs will appear at the Instance level, in the log tab. But going forward, for new instances, ansible logs will be separated by AppServer and appear in each AppServer's "Log" section.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants