Github user lsimons commented on the pull request:
https://github.com/apache/cloudstack/pull/33#issuecomment-65199170
Hey folks,
Karl thanks for contributing. Looks like quite a bit of work!
Rohit thanks for CC-ing me in :)
It's a bit hard to read the pull request right now and provide review
comments:
* Github says there's nearly 7000 changed files so at a guess something
went wrong there? I had a look, and it looks like
KarlHarrisSungardAS/cloudstack is quite a ways behind master, which may explain
that...? It's probably a good idea to make a copy of the feature branch, rebase
it against current master, squash some of the commits together, and then
re-send the pull request. Or, start another branch, and cherry-pick over just
the changes that _should_ be in the change set.
* git hasn't picked up on the relation between veewee and packer, it's
probably good idea to give it some more hints. For example it would be good to
see
https://github.com/KarlHarrisSungardAS/cloudstack/commit/892b11ec0230ce87d14370fd1055ac223096cc77#diff-67bdab8b08f1e110ceabe1a23c3d4011R1
compared to the veewee equivalent.
* there's a lot of 'shuffling' and/or adding comments intermixed with
functional changes. For example
https://github.com/KarlHarrisSungardAS/cloudstack/commit/7f55a1ef032c29cb955e02c90da43c91739108d7#diff-95748c84d8b6be2da502091e69b748d9L30
you might think the 'set -x' is getting removed here, but it just moved down a
bit amidst a few new comments.
* there's a lot of commits that are seemingly unrelated to the actual
change (probably cherry-picked from master onto the feature branch?), those
will need to be filtered out I think.
* there's a lot of 'archive' commits which have unfinished 'checkpoint'
code. I imagine all that work gets finished 'later on' (like renaming
_gitignore to .gitignore is one that I tracked) but it's not so easy to see.
It'd be great if those things could be squashed together.
As far as content goes...
1) Like sebastien says, veewee is in maintenance mode and moving to packer
(replacing veewee) is definitely a good idea. This looks like a straightforward
port, which if it works ok seems like the way to go. We _should_ make sure to
do it in a way that's somewhat compatible with the jenkins.buildacloud.org
setup. I don't think we could sustain maintenance of both packer and veewee
builds for long, it'll be a pain to keep changes in sync. I'd want to drop
veewee use completely.
2) Testing the systemvm using vagrant is also obviously a good idea (we're
doing much the same, over at
https://github.com/schubergphilis/cloudstack/tree/feature/systemvm-persistent-config/tools/vagrant/systemvm
see https://github.com/schubergphilis/cloudstack/commits/feature/systemvm-test
for an abandoned branch having just test-related commits), though I don't
understand why we'd need to use vagrant templates? The .box file that we get
out of the veewee-based systemvm build imports
(https://github.com/apache/cloudstack/blob/master/tools/appliance/build.sh#L466)
just fine...
3) For the stuff that seems to be planned _beyond_ these changes, it might
be a good idea to pop by on the development mailing list and discuss plans. A
few people at Schuberg Philis (my employer, where there's a few people working
on cloudstack) have been working on redundancy for the virtual router for a few
months now. We've found that making it work also involves a lot of changes
outside of the systemvm, too, i.e. changing the orchestration logic in the java
code to match changed behavior of the vpc router (see
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Refactor+for+Redundant+Virtual+Router+Implementation
/ https://www.slideshare.net/LeoSimons1/toolkit-posterccceu14-v2 ). @spark404
please do have a look at Karl's approach :)
cheers,
Leo
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
