cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From lsimons <...@git.apache.org>
Subject [GitHub] cloudstack pull request: Feature cenik123 vpcvrr 1.1.1
Date Tue, 02 Dec 2014 08:50:05 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message