Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172362547
This PR has already been merged by @remibergsma . For same weird reason it
remained open here and now says it has conflicts. Closing it.
Thanks,
Github user wilderrodrigues closed the pull request at:
https://github.com/apache/cloudstack/pull/1277
---
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172359841
Run the tests again, all fine.
```
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/1277#issuecomment-172361223
PR is merged but not properly picked up by Github or so. Checked all
commits, they are in 4.7 just fine. @wilderrodrigues please close this PR,
thanks!
---
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172176953
Ping @remibergsma
If you get a second LGTM, feel free to merge.
Cheers,
Wilder
---
If your project is set up for it, you can reply
GitHub user wilderrodrigues reopened a pull request:
https://github.com/apache/cloudstack/pull/1277
[4.7] Critical VPCVR issues fixed: CLOUDSTACK-9154; CLOUDSTACK-9187; and
CLOUDSTACK-9188
This PR applies the same fixes as in the PR #1259, but against branch 4.7.
Please
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172246077
@koushik-das @DaanHoogland You both reviewed this PR. What do we need to do
to get it in? Those are critical fixes I want in the next releases. Thanks!
---
If
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172250734
@remibergsma @DaanHoogland
This PR doesn't contain the commit that @koushik-das voted -1. So, it's
good to go.
I just double checked in
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172250789
Thanks @wilderrodrigues happy we can move on now!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172260184
looked at the code. python LGTM. The reformat seems unneeded, it seems the
java code does not contain any change but reformat and making stuff final.
---
If
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-172057412
@wilderrodrigues Please reopen this PR, it needs to get in as those are
important fixes. Thanks! :-)
---
If your project is set up for it, you can reply to
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-171572729
If those changes are needed by the community in the futures, please let me
know and I will reopen the PR. One can also feel free to create a PR on top of
Github user wilderrodrigues closed the pull request at:
https://github.com/apache/cloudstack/pull/1277
---
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
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-171597154
:( indeed, @remibergsma. We have a policy against having our fork on github
(tell you over a beer someday) Otherwise I think it would be merged in our
private
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-171591896
To bad this is taking to much effort. We or someone will run into this and
need it. hopefully we get it in then or someday, @wilderrodrigues. Thanks for
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-171593083
@DaanHoogland This PR addresses serious issues. Without it we couldn't run
a reliable production services. It's a shame we can't get it merged. :-(
---
If
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170508275
Hi @koushik-das
Your questions were rhetorical and hence not valid because I did not
disagree with you in terms of refactor. I asked you to help
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170483420
@wilderrodrigues My questions and comments are very valid. If you don't see
any value in them too bad. Looks like you don't want to understand the point I
am
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170535918
Let's all move on, @koushik-das. If we don't, we will be talking about this
for weeks. We have been saying the same thing all the time. I got your points
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170558660
@DaanHoogland @remibergsma @koushik-das
It worked here without the commit mentioned by @koushik-das .
```
[root@cs1 integration]# less
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170520102
@wilderrodrigues My questions were not in the context of refactor at all.
The reason for asking the questions was to highlight the point that for testing
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170542781
I got two failures. egress and an exception VPC_nics_after_destroy. No time
to investigate today.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170340809
* Results
```
[root@cs1 integration]# less
/tmp//MarvinLogs/test_vpc_redundant_DIBS05/results.txt
Create a redundant VPC with two networks
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170343091
@wilderrodrigues I will start this in my old bubble and do some review.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170352604
never mind, should have tried with -b 4.7
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170352576
got an error during the install:
```
/data/git/cs1/cloudstack
Done.
HEAD is now at 2a9927a Merge pull request #1315 from pavanb018/master
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170360300
@DaanHoogland Indeed, that should work :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170240228
@wilderrodrigues
- Do you agree that gc thread should run every gc.interval seconds whether
it has to cleanup any network or not?
- Do you agree that
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170236373
@koushik-das
You will have to run the tests on your environment. How did you check that
the Network GC was running every 10 seconds? After
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170237820
Hi @koushik-das ,
Do you have real hardware to test?
Well I have 2 tests running agains real hardware. @remibergsma also
tested it and
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170238553
@wilderrodrigues
The GC thread execution frequency shouldn't depend on real hardware or
simulator. It will always execute at gc.interval seconds as soon as
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170239651
Are you not sure what it should do once real machines are gone and a
network is still present?
The tests do:
1. Deploy a DC
2. Create
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170237613
@wilderrodrigues I simply ran the steps mentioned against latest master in
a simulator setup and attached a debugger. The network GC thread was running
every
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170242223
Hi @koushik-das
It's not only with specific VPC tests, @koushik-das. That's why I asked you
to do it manually, but unfortunately you don't have a
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170218395
@remibergsma @koushik-das @DaanHoogland
I removed the commit you mentioned. But now we have to retest it. Do you
have a test environment? I need
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-169928856
Hi @DaanHoogland and @koushik-das
Now I see your point. Thanks for the screenshot, @DaanHoogland. I checked
the Github diff but did not notice
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-169939093
@DaanHoogland and @koushik-das
I split the commits and pushed the changes. You now should be able to
review the actual changes I have applied to
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170098946
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,required_hardware=true \
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170202865
On latest master (without any fixes) I see that the config values
(network.gc.interval and network.gc.wait) are read correctly. Did the following:
- Started
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170017549
Try something, @koushik-das:
```
Do not use this code for the steps below, but Master instead.
```
1. Deploy a DC
2. Create a
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-169979032
As a general cleanup, all configs defined in Config.java should be
gradually moved to the appropriate java classes from where they are used. We
should do away
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-169996476
Do you agree that it is a completely different issue you are trying to
address?
ACS needs refactor in each corner of its code, but we cannot work
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-170011469
I have clearly mentioned that the refactoring needs to be done gradually.
But in the changes you have reverted to using the config defined in Config.java
when
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-169609249
@wilderrodrigues as @koushik-das says, the diff for
engine/orchestration/src/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
does not show
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-169304485
Ping @remibergsma @DaanHoogland @koushik-das @bhaisaab
Is there anyone caring about getting those fixes in? If not, I will close
the PR.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-166881123
@koushik-das Github cannot display large diffs. This is how you can show it
on your local checkout:
```
prId=1277
git checkout master
git
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-166884395
@remibergsma What you are suggesting is to pull the commit locally and then
view it using git show. But then we are loosing the benefit of viewing and
in-place
GitHub user wilderrodrigues opened a pull request:
https://github.com/apache/cloudstack/pull/1277
[4.7] Critical VPCVR issues fixed: CLOUDSTACK-9154; CLOUDSTACK-9187; and
CLOUDSTACK-9188
This PR applies the same fixes as in the PR #1259, but against branch 4.7.
Please
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-13159
Ping @remibergsma @miguelaferreira @borisroman @michaelandersen
PR created against 4.7. Please check all tests I ran and results in the
previous
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1277#issuecomment-166820236
Github is not showing the diff for the below file as there are too many
changes. Is there any way to display it?
50 matches
Mail list logo
