Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/909
---
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
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/885
---
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
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145436050
I will merge this once travis is done.
---
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 proje
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145434199
commited, squashed and force-pushed the change requested by Rajani
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145423925
one minor comment. otherwise, LGTM (only did code review)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/909#discussion_r41109279
--- Diff:
engine/orchestration/src/com/cloud/vm/VirtualMachinePowerStateSyncImpl.java ---
@@ -111,7 +111,19 @@ private void processReport(long hostId, Ma
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145382427
@karuturi @koushik-das You reviewed PR #885, @dahn has added one commit and
made this PR ontop of the previous one. Would you be so kind to review it? Need
one m
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145381656
Tested everything I tested in PR #885 again in this PR. @dahn already run
the tests, I verified the functionality manually by destoying VMs out-of-band
and see h
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145369924
```bash
nosetests --with-marvin
--marvin-config=/data/shared/marvin/mct-zone1-kvm1.cfg -s -a \
tags=advanced,required_hardware=false smoke/test_routers.p
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145324612
one test skipped, that requires two hosts.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If yo
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145324409
Test router internal advanced zone ... === TestName:
test_02_router_internal_adv | Status : SUCCESS ===
ok
Test restart network ... === TestName: test_03
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145154565
Hi @DaanHoogland can you please run the same tests as we did on PR #885 and
post output? Let me know if you need help.
---
If your project is set up for it, you
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/909#issuecomment-145037606
@DaanHoogland Could you please add a bit more descriptive title?
---
If your project is set up for it, you can reply to this email and have your
reply appear on
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/909
CLOUDSTACK-8848
added a null guard to @resmo's #885 A unit test or two would be nice as
well but as this is a blocker I want to get it to review asap.
@koushik-das @wilderrodrigues @ans
Let's merge this as-is for 4.6 and solve a blocker. The improvement can be done
later in a separate PR.
Sent from my iPhone
> On 02 Oct 2015, at 10:29, DaanHoogland wrote:
>
> Github user DaanHoogland commented on the pull request:
>
>https://github.com/apache/cloudstack/pull/885#issueco
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144958879
Had a look, the NPE won't occur within the scope of this fix. I would
suggest some changes anyway to prevent future issues as the method
isPowerStateUpToDate()
Github user DaanHoogland closed the pull request at:
https://github.com/apache/cloudstack/pull/829
---
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 featu
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-144949845
This change will have to go in a more solid and complete refactor, probably
in smaller steps. The fix it is intended for will go in #885.
---
If your project
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144936216
Talked to @DaanHoogland he will have a look soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144935223
@resmo can you respond to the comment about the possible NP please?
@dahn can you close PR #829 as we'll be using PR #885 (or am I mistaken?)
Please conf
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-16693
The functionality of this PR also seems to work:
```
WARN [c.c.v.VirtualMachinePowerStateSyncImpl]
(AgentManager-Handler-5:null) Detected missing VM
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144430354
@resmo I tested a series of BVT tests (not all of them) and the result is
fine.
First test run:
```
marvinCfg=/data/shared/marvin/mct-zone1-kvm1.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144377723
@resmo Just a heads-up that I am testing this as we speak. Running the BVT
tests against this branch to verify it all works. Once everything is done I'll
post re
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144361517
ok, no further comment (please see the possible null pointer one)
---
If your project is set up for it, you can reply to this email and have your
reply appear o
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144346880
well I need it in 4.5.3. I would suggest we take this fix for now for 4.6
as well and make a proper refactor for 4.7/5.0.
---
If your project is set up for it, you ca
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144334476
I think this will work but it is a fix on a broken state machine. The
statemachine expects a power report every so and so time-interval and when it
doesn't come
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/885#discussion_r40772428
--- Diff: engine/schema/src/com/cloud/vm/dao/VMInstanceDaoImpl.java ---
@@ -805,6 +805,12 @@ public Boolean doInTransaction(TransactionStatus
status)
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-144030748
@resmo The changes LGTM.
About testing on XS, the following should work:
- Deploy a VM
- Destroy the VM outside of CS (for e.g. from XenCenter)
- I
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143685748
@resmo Thanks! I'll run some tests today.
---
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 anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143639563
LGTM on the basis of 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 does n
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143591694
@remibergsma done
---
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 fe
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143589352
@resmo Thanks for the fix! Could you rebase against current master please?
Then we'll be able to run some tests against this branch.
---
If your project is set
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143549250
@karuturi @anshul1886
So, I 'll checked the handling again and what I see is, it will not work
with this one line change only:
~~~
Date vmStateUpdateTime
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143544132
@karuturi I understand what @anshul1886 trying to point out: the powerState
should already be updated to the new state `powerReportMissing`.
And because it ch
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143543503
@anshul1886 without the new method, how do you ensure that
PowerStateUpdateTime is up to date for the vm? It may be a very old value and
can be a false alarm(same i
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143542706
code LGTM. I am facing some issues with my devcloud4 setup and hence
couldnt run tests.
---
If your project is set up for it, you can reply to this email and have
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143442228
We don't need new method isPowerStateUpToDate(long instanceId) as we are
already performing the same operations in
_instanceDao.updatePowerState(entry.getKey(), h
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-143227931
I successfully tested the patch on 4.5.2:
~~~
2015-09-25 15:38:11,953 WARN [c.c.v.VirtualMachinePowerStateSyncImpl]
(DirectAgentCronJob-7:ctx-65344d8a) Detecte
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-142950081
It is actually not easy to test because you must hit a race condition.
What I do is on vCenter make a few migrations of a VR to another hosts and
look on the
@resmo I will find some time tomorrow to test this. Can you add steps to
test please? Or are they already available on the other pr?
On Thu, Sep 24, 2015 at 19:39 PM, resmo wrote:
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-14
GitHub user resmo opened a pull request:
https://github.com/apache/cloudstack/pull/885
CLOUDSTACK-8848: ensure power state is up to date when handling missiâ¦
â¦ng VMs in powerReport
There 2 things which has been changed.
* We look on power_state_update_time inst
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/885#issuecomment-142941464
/cc @DaanHoogland @koushik-das @anshul1886
---
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 proj
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142940617
So I made a new PR #885 as a replacement for new discussion how this issue
can be solved.
IMHO this PR can be closed
---
If your project is set up for it, yo
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142920071
I have followed your explanation but what I am trying to say is that it may
get updated due to side effect of some other code as from my experience this
was worki
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142912573
Cloud you follow my explanations? If this would help, I must set the ping
interval to a year or somewhat identical. Are you REALLY followed my
explanations? What did I
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142909650
Yes, increasing time should help.
---
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 resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142906721
This is perfectly clear to me. The problem is we are relaying on the
`update_time` (not to mix up with `power_state_update_time`).
~~~
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142901959
Increasing the grace period for a few minutes won't help. If you look at
these update_time values I queried from 10 running routers, you see these are
days, weeks behi
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142903425
This simply means that there power_state has not changed for that period of
time. As I pointed out in my previous comment that if state is consecutively
updated w
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142899514
From code it seems to be getting updated and DB also suggests that. It will
not be updated if there is no power change for
MAX_CONSECUTIVE_SAME_STATE_UPDATE_COUNT
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-142882541
@anshul1886 @koushik-das
@DaanHoogland and I had a debug session last friday, and since he is off
for the next couple of days I can give you more details about w
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141613687
@DaanHoogland what update_time are you talking about?
I think as a work around increasing ping interval to some higher value
like 90 sec or 120 seconds in glo
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141482058
@koushik-das @anshul1886 the grace period is not going to work. updat_time
is not getting updated. We really need to fix the statemachine
---
If your project i
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141421211
@koushik-das about the bug probably not being in 4.6 i agree.
@resmo I understood that the router was rebooted while you set the reboot
on out of band flag t
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141418450
@DaanHoogland As per logs attached in bug VR reboot happened due to
out-of-band movement from one host to another and not due to power report
missing.
H
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141407138
@koushik-das @DaanHoogland unless we have a "better" solution I would like
to go with @anshul1886 suggested fix. It seems the best way not breaking
anything else.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141403050
@koushik-das I am perfectly fine with not putting this code in but I don't
understand that you don't follow how it is related to the bug reported. The
logs supp
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141377920
-1 to this PR as I am not able to follow how the change is related to the
bug. Also this is going to impact any HV having VM changes outside of CS like
Vmware an
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-141050957
If @remibergsma is right then the design is really broken. two events come
in during migration:
# PowerOn (or Off or what ever is actual)
# PowerReportMi
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-140983520
@resmo You're right and I wasn't clear enough I guess. Because I think the
'PowerReportMissing' comes from the VM on the hypervisor that you are migrating
_to_,
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-140879864
@remibergsma does not look related to me, the keyword seems to be
PowerReportMissing.
---
If your project is set up for it, you can reply to this email and have your
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-140867296
@DaanHoogland @resmo Not sure if this is related, but in 4.4 (and running
XenServer/KVM probably any hypervisor) I see sometimes alerts when a router is
live-mig
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-140791892
@koushik-das could you comment, please. Regarding power off and no power
state equal in some case might make sense but in general it certainly doesn't
as we hav
Github user anshul1886 commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-140665483
Currently we report only PowerOn VMs and do not report PowerOff VMs that's
why we consider Missing and PowerOff as same And that's how whole code is
written and e
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-140643483
@bhaisaab it should be for 4.5 as well but It is not complete yet, unless
we decide that we don't take any action on a missing report. I will make a PR
for 4.5
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/829#issuecomment-140641376
@DaanHoogland this is for just master, or also applies for 4.5; if so
please send a PR for 4.5 as well. Thanks.
---
If your project is set up for it, you can reply
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/829
CLOUDSTACK-8848: extra state to handle; no power report from host
there no no real handling code now so this can be regarded as a DISCUSS
item. It may well be an improvement over existing c
67 matches
Mail list logo
