Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1176#issuecomment-162231372
@anshul1886 Please address comments made above. After that we can merge.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/872#discussion_r46761289
--- Diff: systemvm/patches/debian/vpn/etc/ipsec.d/l2tp.conf ---
@@ -30,4 +33,6 @@ conn L2TP-PSK
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/977#issuecomment-162232240
@ustcweizhou Can you please rebase and resolve the conflict? I can then run
the tests again.
@wido @DaanHoogland Do we want this in 4.7?
---
If your
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1171#issuecomment-161920716
@bhaisaab How can I help testing this? Also, your branch mentions 4.6 but
the PR is made against master branch. Is that correct? Just checking ;-)
---
If your
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1163#issuecomment-161916986
@DaanHoogland I will count that as a LGTM and will ask @wilderrodrigues to
also review.
---
If your project is set up for it, you can reply to this email
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1013#issuecomment-161919315
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1125#issuecomment-161919700
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1163#issuecomment-161915494
@DaanHoogland Yes, that one fails quite regularly and I think that is
related. Although without this fix it also passes, just not always (can be
explained
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1169#issuecomment-161916597
Thanks @bhaisaab I will merge this now, as I count the LGTMs in PR #1149
(in this case running the tests again is not needed).
---
If your project is set up
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1157#issuecomment-161918736
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1177#issuecomment-162107478
Pinging @snuf to have a look.
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-162107170
@bhaisaab @abhinandanprateek Just tested again. It's cool, the UI tweaks
are very nice. Much better, I like it!
I can also confirm the menu option
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1172#issuecomment-162043429
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1168#issuecomment-162044783
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1167#issuecomment-162047910
Example logs from this branch:
```
2015-12-04 18:37:14,864 DEBUG [c.c.s.StatsCollector]
(StatsCollector-2:ctx-601c739d) (logid:52f7b0de
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1167#issuecomment-162045526
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1166#issuecomment-162048942
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1161#issuecomment-162051744
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1086#issuecomment-162053643
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1142#issuecomment-162054305
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1098#issuecomment-162057398
@milamberspace Do you want to update this PR before we merge it?
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1176#issuecomment-162050985
FYI, this integration test failed:
```
Create a VPC with two networks with one VM in each network and test nics
after destroy ... === TestName
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1094#issuecomment-162054941
@nvazquez @miguelaferreira Hi Guys, do you think you can finish this PR on
Monday so we can include it in 4.7? Would be nice :-) Let me know if you need
help
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1149#issuecomment-161885184
@bhaisaab Check! This I cannot merge to 4.6 with this PR so if you make one
against 4.6 that would be easy. Mention this one so it can go through faster
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1164#issuecomment-161893553
@anshul1886 Thanks, you can close this PR as the other one will go to both
4.6 and master.
---
If your project is set up for it, you can reply to this email
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1164#issuecomment-161889769
@anshul1886 Thanks for the fix! Could you please make this PR against 4.6?
Once merged in 4.6, it will be fwd-merged to master. Thanks!
---
If your project
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/841#issuecomment-161554065
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1155#issuecomment-161569317
Will run some tests to double check.
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1107#issuecomment-161553363
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1151#issuecomment-161557568
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1134#issuecomment-161560287
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/943#issuecomment-161551497
LGTM as these integration tests still pass:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/943#issuecomment-161552200
@DaanHoogland @rafaelweingartner The failed expunge test was a false alarm
due to our testing environment. It passes again now. Please ping me when
there's
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/944#issuecomment-161558537
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1151#issuecomment-161557066
The test passes fine here, also on a 2 node cluster:
test_01_vpc_privategw_acl
(integration.smoke.test_privategw_acl.TestPrivateGwACL
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-161570202
@bhaisaab That sounds like a good solution. I will now test this PR again
and let you know.
---
If your project is set up for it, you can reply to this email
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1158#issuecomment-161641914
-1
I don't think it is a good idea to reboot routers when out-of-band
operation is detected. We discussed this before, let's not do it again. Please
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-161645283
@abhinandanprateek I will test it soon, thanks. Personally, I think a black
& white icon is better in line with the other icons.
---
If your project is se
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-161653555
@bhaisaab The SQL issue is indeed resolved. I still see the icon when the
plugin is disabled (and it of course gives errors then). Tried clearing the
cache
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1013#issuecomment-161751305
@jharshman I will run some tests and report back. Can you please squash the
commits?
---
If your project is set up for it, you can reply to this email
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1155#issuecomment-161745983
LGTM, run some tests and they are all fine:
```
Create a redundant VPC with two networks with two VMs in each network ...
=== TestName
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1149#issuecomment-161748134
@bhaisaab Should I merge this to master, or should it be merged to 4.6?
---
If your project is set up for it, you can reply to this email and have your
reply
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1134#issuecomment-161750205
@pdube indeed, I waited because of the comments but that has been addressed
now :-)
---
If your project is set up for it, you can reply to this email and have
GitHub user remibergsma opened a pull request:
https://github.com/apache/cloudstack/pull/1163
Send arping to the gateway instead of our own address
We need to send an Unsolicited ARP to the gateway, instead of our own
address. We now encounter problems when people deploy/destroy
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1145#issuecomment-161430026
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1134#issuecomment-161431929
@pdube will run some integration tests against this branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1151#issuecomment-161428530
@wilderrodrigues Will start some tests!
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1029#issuecomment-161431453
@ustcweizhou PR #1145 is merged to 4.6, please close this PR. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1149#issuecomment-161429236
Makes sense, LGTM also based on the successful job above.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1145#issuecomment-161429404
@ustcweizhou Took me a while, but the issue is not in this PR. Will post
test results soon.
---
If your project is set up for it, you can reply to this email
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-161449429
Had a first look at the feature and have some questions / remarks:
The quota icon is the only one in color and doesn't really fit the rest. Is
that done
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/942#issuecomment-161433986
@DaanHoogland That expunge issue is now resolved in the integration tests,
my bad :-s
---
If your project is set up for it, you can reply to this email
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-161451821
@bhaisaab The SQL changes required are implemented in
`schema-461to470.sql`. When I build from scratch, I hit `schema-460to470.sql`
and end up with a db without
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-161436020
Sounds great guys!
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1149#issuecomment-161437830
Ping @borisroman to have a look
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-161530260
Let's ask @bhaisaab, we probably need to have the sql in both files to
support both upgrade paths. Is there a better way?
---
If your project is set up
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/872#issuecomment-161191190
@bhaisaab time is running out, if we want this in 4.7 (and that would be
great!) we need to merge on Dec 7 the latest. Can you give this a spin and see
if you
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1145#issuecomment-160928383
The integration tests show this test failing (part of
test_vm_life_cycle.py):
```
Test destroy(expunge) Virtual Machine ... === TestName
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1146#issuecomment-160764367
The build script now makes a branch that makes sense:
`4.6.1-RC20151130T2206` instead of `4.6-RC20151130T2206`. Also the versions are
updated properly
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1146#issuecomment-160766514
@rafaelweingartner I agree and I think this needs some TLC indeed.
Investigate why it doesn't work with mvn and then do one global search/replace
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1146#discussion_r46205478
--- Diff: tools/build/build_asf.sh ---
@@ -123,11 +128,7 @@ git clean -f
#create a RC branch
RELEASE_BRANCH="RC"`date +%
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1146#discussion_r46205215
--- Diff: tools/build/build_asf.sh ---
@@ -101,6 +101,11 @@ perl -pi -e "s/-SNAPSHOT//" build/replace.properties
perl -pi -e &q
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1138#issuecomment-160721455
Thanks for the reviews, I'll merge it to 4.6 like this. The improvement
should go via Transiflex website and ends up in a new PR when @milamberspace
submits
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1122#issuecomment-160711475
LGTM, the test runs fine here:
```
Testcreation, adding and removing tag on public IP address ... ===
TestName: test_24_public_IP_tag | Status
GitHub user remibergsma opened a pull request:
https://github.com/apache/cloudstack/pull/1146
make sure all files are updates with new version
During the previous release I had to edit these manually. This will fix
that so all versions get updated.
You can merge this pull request
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1144#issuecomment-160756847
@ustcweizhou This is a mistake I guess, PR against the wrong branch?
---
If your project is set up for it, you can reply to this email and have your
reply
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-160780363
@jburwell Should we squash the commits here? Guess that would make sense
because it'd be an atomic commit then, right?
---
If your project is set up
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1085#issuecomment-160442363
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1085#issuecomment-160442447
Functionality also works:
```
(local) � > list vlanipranges keyword=50
count = 1
vlaniprange:
id = abdc0be9-6869-438b-9
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1116#issuecomment-160442050
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1087#issuecomment-160431621
LGTM, tested on CentOS7:
Logrotate is there:
```
[root@cs1 x86_64]# cat /etc/logrotate.d/cloudstack-catalina
/var/log/cloudstack
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1087#issuecomment-160431637
@PaulAngus please respond to the comment by @jburwell. After that we can
merge.
---
If your project is set up for it, you can reply to this email and have
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1132#issuecomment-160443626
@bhaisaab Let's find someone else who can review this..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1121#issuecomment-160390678
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1134#discussion_r46088753
--- Diff: test/integration/component/test_affinity_groups_projects.py ---
@@ -0,0 +1,1083 @@
+#!/usr/bin/env python
+# Licensed
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1136#issuecomment-160390613
@bhaisaab The problem with your approach, to be honest, is that the commit
ID changed. As this was a bit of a special case it may not be a problem.
General
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1121#issuecomment-160390932
Pinging @DaanHoogland @borisroman to do second review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1118#issuecomment-160390779
LGTM based on these tests (although I didn't test the actual change).
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1094#issuecomment-160391354
@miguelaferreira @wilderrodrigues Can one of you review this? Once done, I
can run the integration tests again.Thanks!
---
If your project is set up
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1007#discussion_r46089078
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java
---
@@ -3310,13 +3331,19 @@ public Vlan dedicatePublicIpRange(final
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1135#issuecomment-160393076
FYI: @pdube I set all timers to 5 sec but that didn't result in a faster
integration test run for me.
---
If your project is set up for it, you can reply
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1132#issuecomment-160392997
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
component
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1137#issuecomment-160390871
LGTM, just looked at the code.
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1136#issuecomment-160391149
@bhaisaab The commit ID didn't change from the PR and your merge, that was
fine. It was just different in 4.6 branch because the change was done combined
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1122#discussion_r46095649
--- Diff: test/integration/component/test_tags.py ---
@@ -2401,3 +2403,143 @@ def
test_23_list_untagged_host_for_vm_migration(self
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1106#issuecomment-160288024
LGTM, works fine and I was able to sort columns (example global settings).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user remibergsma commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1134#discussion_r46079415
--- Diff: test/integration/component/test_affinity_groups_projects.py ---
@@ -0,0 +1,1083 @@
+#!/usr/bin/env python
+# Licensed
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1134#issuecomment-160288372
@pdube Awesome you added all the tests! Thanks! Will have a run some tests
myself soon.
---
If your project is set up for it, you can reply to this email
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1135#issuecomment-160287896
@pdube Had a look and you may also want to tweak these settings:
# Network garbage collection
network.gc.wait
network.gc.interval
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1081#issuecomment-160296533
@DaanHoogland This is already much better, when you have 30 port forwarding
rules the UI is unusable without this fix. When we want to make more
improvements
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1101#issuecomment-160296390
@ustcweizhou LGTM, tested it and works as expected.
Minor comment: the first item in the list is empty (it will then keep the
same template), rest
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1115#issuecomment-160298327
If we want this in 4.6, please don't merge this. When #1116 is merged in
4.6, it will be fwd merged to master as well.
---
If your project is set up
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1133#issuecomment-160298443
@pdube Any news on your testing? :-)
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/768#issuecomment-160282940
All integration tests pass, so that's good! Didn't look at the Quota
functionality itself yet.
