Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spawner upgrade fabric #9

Merged
merged 3 commits into from
Mar 21, 2020
Merged

Spawner upgrade fabric #9

merged 3 commits into from
Mar 21, 2020

Conversation

arthurian
Copy link
Member

@arthurian arthurian commented Mar 19, 2020

This PR upgrades from Fabric3 v1.14.post1 to Fabric v2.5. The goal is to swap the old fabric run() and sudo() calls for the new ones, without changing the functionality.

The main reason for upgrading, is the fact that the latest version of fabric is thread-safe. This is important for the spawner, because fabric calls are submitted to a thread pool and executed asynchronously. It's not clear that the v1.x forked version of fabric is actually thread-safe, and some intermittent issues that have cropped up when the system is under load may be related to this.

Additional Notes:

  • Fabric v2.x is a major rewrite, and the API has changed quite a bit. Thankfully, the upgrade specifics documentation is very thorough.
  • This is branched from spawner-logging. The expectation is that once spawner-logging has been merged into develop, then this PR can be updated so that the diff will show just the fabric-related updates.
  • I've tested this a little in in the miniconda cluster on the jupyterhub test site, but it needs more extensive testing to be sure it's working as expected.

Upgrading:

$ pip list | grep Fabric3           # Show that Fabric3==1.14.post1 is installed
$ pip install fabric2==2.5.0        # Install fabric v2.5.0 aliased to fabric2

I've used the name fabric2 instead of plain old fabric intentionally (this is documented in the upgrade guide), that way we can be sure that an error will occur if fabric2 is not actually installed, and also so it can exist alongside code that has not been migrated yet (e.g. code still importing fabric).

@joshuagetega @dodget

Fabric3 is an unauthorized fork of the main fabric project. It was
originally forked to add python3 support, but the main fabric project
now supports python3 and also has gone through a significant rewrite.

The motivation for upgrading, besides the fact that Fabric3 is
deprecated and no longer supported, is the fact that fabric is
thread-safe, providing better support for concurrency.
@arthurian arthurian requested a review from joshuagetega March 20, 2020 15:56
Copy link

@joshuagetega joshuagetega left a comment

Choose a reason for hiding this comment

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

@arthurian, this looks great. Thanks for being thorough. I am commencing testing this on the dssg cluster, after which we can merge the PR should no problems present themselves. I notice that there are a few other places in the CloudJhub codebase where "fabric" is mentioned. Looks like modification will be needed here and here. That can be work for another PR given the focus right now is on the spawner.

@arthurian
Copy link
Member Author

@joshuagetega Great point about the other instances of fabric being used by cloudJHub. I believe cull_idle_servers.py also imports fabric, but it's only actually used for managing rstudio sessions (which we aren't running). The good thing is that we can have fabric2 installed alongside the old version, so we can migrate things over time. I agree that we should prioritize the spawner first. Thanks for testing on the dssg cluster.

Copy link

@dodget dodget 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. I don't have a lot to add here.

@joshuagetega
Copy link

@arthurian, oh, yes, you are right. cull_idle_servers.py uses fabric, too. It is great that we can run the 2 versions side by side. I will write a story for doing the rest of the upgrades and put it in the backlog. At any rate, I think we can merge this PR now. The test on the dssg cluster is complete. And the spawner upgrade worked as expected. Great catch to add fabric2 to the requirements files as in the latest commit.

Merging...

@joshuagetega joshuagetega merged commit 71c9335 into develop Mar 21, 2020
@joshuagetega joshuagetega deleted the spawner-upgrade-fabric branch March 21, 2020 13:10
@joshuagetega
Copy link

Just realized this PR does not show I approved it, even though I have. I was to select the "Approve" option. Only remembering now, post-merging. At any rate, this comment is to indicate that I have approved the pull request.

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

Successfully merging this pull request may close these issues.

3 participants