Getting started hacking on OpenStack Nova
In recent months I have spent more of my time working on projects immediately above/related to the core libvirt library, such as libvirt-glib, libosinfo and virt-sandbox. To that list I have now added OpenStack, where my goal is to ensure that the libvirt driver is following all the best practices and start to take advantage of libosinfo for optimizing virtual hardware configuration. I’m familiar with hacking on python so that’s no big issue, but what is new about OpenStack is dealing with Gerrit. For the sake of reference, here were the steps I went through on Fedora 16 for my first patch (a tweak to the tools/install_venv.sh file)
- Get the initial Nova GIT checkout
$ mkdir $HOME/src/cloud $ cd $HOME/src/cloud $ git clone git://github.com/openstack/nova.git $ cd nova
- Install some basic pre-reqs, and ensure python-distutils-extra is not present since that conflicts with part of the openstack build system
$ sudo yum install gcc python-pep8 python-virtualenv m2crypto libvirt libvirt-python libxslt-devel libxml2-devel $ sudo yum remove python-distutils-extra
- Visit the OpenStack Gerrit Website, and follow ‘Sign In’ link which redirects to LaunchPad for authentication
- Back on Gerrit site, now signed in, follow ‘Settings’ link, select ‘SSH Public Keys’ page, and paste your SSH public key (eg contents of
$HOME/.ssh/id_rsa.pub) - Test SSH connectivity from the CLI
$ ssh -p 29418 berrange@review.openstack.org The authenticity of host '[review.openstack.org]:29418 ([173.203.103.119]:29418)' can't be established. RSA key fingerprint is ee:2f:ac:1b:f8:25:d0:39:be:55:02:c7:76:5e:39:53. Are you sure you want to continue connecting (yes/no)? yes Warning: Permanently added '[review.openstack.org]:29418,[173.203.103.119]:29418' (RSA) to the list of known hosts. **** Welcome to Gerrit Code Review **** Hi Daniel Berrange, you have successfully connected over SSH. Unfortunately, interactive shells are disabled. To clone a hosted Git repository, use: git clone ssh://berrange@review.openstack.org:29418/REPOSITORY_NAME.git Connection to review.openstack.org closed.
- Install commit hook to ensure ‘ChangeId’ fields get added to your commits
$ scp -p -P 29418 berrange@review.openstack.org:hooks/commit-msg .git/hooks/
- Add the gerrit remote to GIT config
$ git remote add gerrit ssh://berrange@review.openstack.org:29418/openstack/nova.git
- Start a new branch for your work
$ git checkout -b venv-install-fixes
- Make whatever code changes you need todo
$ vi tools/virtual_venv.py $ git add -u (Don't forget to add yourself to Authors if this is your first change)
- Commit the changes, checking the commit message gets a ‘Change-Id’ line added just prior to the signed-off-by line
$ git commit -s $ git show commit fd682a28fb4591c65f20129d4bfb4eccf1232cb8 Author: Daniel P. Berrange <berrange@redhat.com> Date: Thu Jan 5 13:15:15 2012 +0000 Tell users what is about to be installed via sudo Rather than just giving users the sudo password prompt immediately, actually tell them what is about to be installed, so they know what privileged action is being attempted. Change-Id: Ic0c1de812be119384753895531a008075b13494e Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
If the commit is fixing a OpenStack bug, then the commit message should include a line “BugXXXX” where XXXX is the bug number. Gerrit uses this to link to the bug tracker
- Run the unit test suite, and the python pep8 syntax test suite; Be prepared to wait a long time
$ ./run_tests.sh $ ./run_tests.sh --pep8
- Send the changes to Gerrit for review
$ git push gerrit HEAD:refs/for/master
- Wait for email notifications of review, or watch the OpenStack Gerrit Website.
- If problems are found by reviewers, or the automated smoke stack tests. Repeat steps 9->l;12, but use ‘git commit –amend’ to ensure you preserve the original “Change-Id” line in the commit message. This lets gerrit track followup patches.
- If everything passes review & testing, it will be automatically merged into master.
There is also a GIT plugin “git review” available in the git-review RPM, which can provide syntactic sugar for step 12, but personally I don’t find it adds significant value to be worth my while using.
I can see the attraction of Gerrit, but I personally still prefer the practice of using git send-email for reviewing on mailing lists. My problems with Gerrit are
- The email notifications sent out for new patches are almost worse than useless as an information source
- While very pretty, the web UI for browsing the diffs is really quite cumbersome to use
- Poor support for reviewing large patch series
- Use of merge commits makes navigating GIT history cumbersome, forcing the use of the graphical gitk viewer tool
Nice writeup on the setup for Fedora! Have you considered submitting the same content to the documentation project? I’m sure it would be useful for others…
I think git-review also helps you through steps 6 and 7 (git review -s).
Hi!
git-review actually handles steps 5, 6, and 7 for you, in addition to step 12, and also provides a feature to help in downloading changes from gerrit in case you want to either review or edit them locally. BUT, one of the nice things of course is that everything can actually be done directly via git commands as you do above. For the reference of others, git review -v will print out all of the git and other commands it runs to do things.
Submitting code to a mailing list works really well for many people – but does require a human to eventually actually apply those commits, which means it would also be that person’s responsibility to either manually run tests or ensure that the automated tests for that change had run … which is honestly a pretty boring job (we did that step by hand for Drizzle and I was often on merge duty) One of the goals we’ve tried to achieve with most of our developer tooling in OpenStack is machine automation of as many of the repetitive tasks as is humanly possible.
In terms of the problems you’re having:
Email notifications: what would you prefer to receive in email? The email contains the commit message, a git command that can be used to pull the change, and a link to the change in gerrit. Would you prefer the entire diff in the email itself? We have a bug open about allowing responses to email to leave comments on the review: https://bugs.launchpad.net/openstack-ci/+bug/861722 – perhaps file a bug about what you’d rather see in the new patch emails?
Browsing diffs: There is an open bug which is very high on our list to provide a single-page view of the entire patch. Work was done at Nokia on this front, but as far as I know it got held up from contribution by legal, so I’m guessing we’re going to have to redo that work from scratch. In the meantime, you may want to explore the keybindings … I know several of our devs use them extensively.
Support for reviewing patch series is indeed not probably what you’re wanting. Support is there for reviewing dependent patches – but we don’t really recommend people send in massive sets of related patches. Instead, we recommend sending in the patches as they are done, since each one of them is going to be required to be independently testable anyway. That being said – there is work that we’re looking at done by someone in the gerrit community to support reviewing of related patch sets in a sensible way. The biggest hurdle there is modelling and understanding what to do from a testing and patch-landing perspective. If you review the series and approve it, and one of the constituent patches does not pass tests (lets say one of the ones in the middle) – do we land the patches that did pass and stop landing them at the one that didn’t? Or do we reject the whole series as an atomic unit? If it’s the former, then additional UI for expressing a patch series isn’t really providing much additional value. If it’s the later, then we need to provide more context down the event stream so that the testing infrastructure and merge gate understands when to merge something. It’s honestly quite a fun area to spend time thinking about!
Merge commits: we’d love to have patches apply cleanly when possible without a merge commit (and, in fact, we do not merge when we can avoid it). There are two possibilities for this: we can have gerrit reject things that don’t apply cleanly outright and request that the dev rebase and resubmit – but with the volume of changes OpenStack receives and the fact that the resubmission would require a new review (it is, in fact, a new change) – we decided that it was too onerous of a requirement and would make landing changes too hard. Alternately, we can have gerrit cherry pick the changes to apply them. That would get us a cleaner history, but would mean that if a dev was doing an ongoing series of work that built on itself, they’d have to do an unnatural amount of rebasing whenever one of their changes landed. That also seems less than optimal. We do keep our eyes on that though.
Thanks for getting yourself set up to do OpenStack development and sharing your experience! I’m always interested in hearing about peoples needs and experiences both good and bad … we can’t fix peoples pain points if we don’t hear about them.
@monty: For email, yes, I’d like to be sent a full diff of the commit inline to the email. I can imagine some people wouldn’t want that, so it’d probably need a email preference in Gerrit.
A single page view of the entire patch would be really awesome.
This recent work I did on libvirt XML generation is an example of why I’d prefer better patch series support:
https://github.com/berrange/nova/tree/libvirt-xml-config-v1
I delibrately did the conversion in 15 steps, to make it very clear what is being changed in each step, which really simplifies review IMHO. The series is fully bisectable so it should pass tests at each step, and be possible to apply only patches 1->N out of M total. The Gerrit UI really discourages this kind of setup though which is a shame IMHO. I would hate to have to review this series if it was just squashed down into 1 or 2 big patches, which is what I feel tends to happen with many patches in Gerrit.
I opened a bug request for email notifications
https://bugs.launchpad.net/openstack-ci/+bug/955846