Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Eugene Chekanskiy]

> On March 8, 2017, 1:26 p.m., Attila Doroszlai wrote:
> > ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
> > Lines 47 (patched)
> > 
> >
> > Is this eagerly created `Process` used anywhere?  Could it be replaced 
> > with `None`?

done


> On March 8, 2017, 1:26 p.m., Attila Doroszlai wrote:
> > ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
> > Lines 142 (patched)
> > 
> >
> > Won't this set `timedOutEvent` even if out queue loop was terminated 
> > due to `mustDieEvent`?

it possibly can, but it is actually not important


> On March 8, 2017, 1:26 p.m., Attila Doroszlai wrote:
> > ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
> > Lines 236-237 (patched)
> > 
> >
> > Could this be simplified by putting `not self.worker_process` as 2nd 
> > and `not self.worker_process.is_alive()` as 3rd OR condition, this way 
> > omitting `self.worker_process and`?

done


- Eugene


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/#review168289
---


On March 9, 2017, 12:22 p.m., Eugene Chekanskiy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57344/
> ---
> 
> (Updated March 9, 2017, 12:22 p.m.)
> 
> 
> Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
> Robert Levas, and Sebastian Toader.
> 
> 
> Bugs: AMBARI-20323
> https://issues.apache.org/jira/browse/AMBARI-20323
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> All status command execution logic moved under one file.
> 
> * added protection for hardkilling(now process tries to die gracefully)
> * added protection for queue internal logic blocking(underlying pipes hard 
> closed)
> * added lock around queues that will prevent from using queues before new one 
> will be created in case of recreating executor process
> 
> 
> Diffs
> -
> 
>   ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
>   ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
>   ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
>   ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
>   ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
>   ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
>   ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 
> 
> 
> Diff: https://reviews.apache.org/r/57344/diff/5/
> 
> 
> Testing
> ---
> 
> mvn clean test, manual testing
> 
> 
> Thanks,
> 
> Eugene Chekanskiy
> 
>

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Eugene Chekanskiy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/
---

(Updated March 9, 2017, 12:22 p.m.)


Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
Robert Levas, and Sebastian Toader.


Bugs: AMBARI-20323
https://issues.apache.org/jira/browse/AMBARI-20323


Repository: ambari


Description
---

All status command execution logic moved under one file.

* added protection for hardkilling(now process tries to die gracefully)
* added protection for queue internal logic blocking(underlying pipes hard 
closed)
* added lock around queues that will prevent from using queues before new one 
will be created in case of recreating executor process


Diffs (updated)
-

  ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
  ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
  ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
  ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
  ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
  ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
  ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 


Diff: https://reviews.apache.org/r/57344/diff/5/

Changes: https://reviews.apache.org/r/57344/diff/4-5/


Testing
---

mvn clean test, manual testing


Thanks,

Eugene Chekanskiy

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Attila Doroszlai]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/#review168289
---


Fix it, then Ship it!





ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 47 (patched)


Is this eagerly created `Process` used anywhere?  Could it be replaced with 
`None`?



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 142 (patched)


Won't this set `timedOutEvent` even if out queue loop was terminated due to 
`mustDieEvent`?



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 236-237 (patched)


Could this be simplified by putting `not self.worker_process` as 2nd and 
`not self.worker_process.is_alive()` as 3rd OR condition, this way omitting 
`self.worker_process and`?



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 268-272 (patched)


The comment and log message state the same (ie. "died gracefully"), but 
they are on different branches of the `if`.  I guess `process_logs()` and the 
commend belong to the `else` branch instead.


- Attila Doroszlai


On March 7, 2017, 4:41 p.m., Eugene Chekanskiy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57344/
> ---
> 
> (Updated March 7, 2017, 4:41 p.m.)
> 
> 
> Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
> Robert Levas, and Sebastian Toader.
> 
> 
> Bugs: AMBARI-20323
> https://issues.apache.org/jira/browse/AMBARI-20323
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> All status command execution logic moved under one file.
> 
> * added protection for hardkilling(now process tries to die gracefully)
> * added protection for queue internal logic blocking(underlying pipes hard 
> closed)
> * added lock around queues that will prevent from using queues before new one 
> will be created in case of recreating executor process
> 
> 
> Diffs
> -
> 
>   ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
>   ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
>   ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
>   ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
>   ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
>   ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
>   ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 
> 
> 
> Diff: https://reviews.apache.org/r/57344/diff/4/
> 
> 
> Testing
> ---
> 
> mvn clean test, manual testing
> 
> 
> Thanks,
> 
> Eugene Chekanskiy
> 
>

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Sebastian Toader]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/#review168141
---


Ship it!




Ship It!

- Sebastian Toader


On March 7, 2017, 4:41 p.m., Eugene Chekanskiy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57344/
> ---
> 
> (Updated March 7, 2017, 4:41 p.m.)
> 
> 
> Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
> Robert Levas, and Sebastian Toader.
> 
> 
> Bugs: AMBARI-20323
> https://issues.apache.org/jira/browse/AMBARI-20323
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> All status command execution logic moved under one file.
> 
> * added protection for hardkilling(now process tries to die gracefully)
> * added protection for queue internal logic blocking(underlying pipes hard 
> closed)
> * added lock around queues that will prevent from using queues before new one 
> will be created in case of recreating executor process
> 
> 
> Diffs
> -
> 
>   ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
>   ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
>   ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
>   ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
>   ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
>   ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
>   ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 
> 
> 
> Diff: https://reviews.apache.org/r/57344/diff/4/
> 
> 
> Testing
> ---
> 
> mvn clean test, manual testing
> 
> 
> Thanks,
> 
> Eugene Chekanskiy
> 
>

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Eugene Chekanskiy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/
---

(Updated March 7, 2017, 3:41 p.m.)


Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
Robert Levas, and Sebastian Toader.


Bugs: AMBARI-20323
https://issues.apache.org/jira/browse/AMBARI-20323


Repository: ambari


Description
---

All status command execution logic moved under one file.

* added protection for hardkilling(now process tries to die gracefully)
* added protection for queue internal logic blocking(underlying pipes hard 
closed)
* added lock around queues that will prevent from using queues before new one 
will be created in case of recreating executor process


Diffs (updated)
-

  ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
  ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
  ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
  ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
  ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
  ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
  ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 


Diff: https://reviews.apache.org/r/57344/diff/4/

Changes: https://reviews.apache.org/r/57344/diff/3-4/


Testing
---

mvn clean test, manual testing


Thanks,

Eugene Chekanskiy

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Eugene Chekanskiy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/
---

(Updated March 7, 2017, 3:32 p.m.)


Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
Robert Levas, and Sebastian Toader.


Bugs: AMBARI-20323
https://issues.apache.org/jira/browse/AMBARI-20323


Repository: ambari


Description
---

All status command execution logic moved under one file.

* added protection for hardkilling(now process tries to die gracefully)
* added protection for queue internal logic blocking(underlying pipes hard 
closed)
* added lock around queues that will prevent from using queues before new one 
will be created in case of recreating executor process


Diffs (updated)
-

  ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
  ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
  ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
  ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
  ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
  ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
  ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 


Diff: https://reviews.apache.org/r/57344/diff/3/

Changes: https://reviews.apache.org/r/57344/diff/2-3/


Testing
---

mvn clean test, manual testing


Thanks,

Eugene Chekanskiy

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Eugene Chekanskiy]

> On March 7, 2017, 11:53 a.m., Sebastian Toader wrote:
> > ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
> > Lines 158 (patched)
> > 
> >
> > Could this deadlock in case the worker process is killed from outside 
> > thus being shut down without properly closing the mp_result_queue?

No. Queues is closed on process relaunch and every place that blocked will be 
unblocked by IOError.


- Eugene


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/#review168101
---


On March 7, 2017, 3:28 p.m., Eugene Chekanskiy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57344/
> ---
> 
> (Updated March 7, 2017, 3:28 p.m.)
> 
> 
> Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
> Robert Levas, and Sebastian Toader.
> 
> 
> Bugs: AMBARI-20323
> https://issues.apache.org/jira/browse/AMBARI-20323
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> All status command execution logic moved under one file.
> 
> * added protection for hardkilling(now process tries to die gracefully)
> * added protection for queue internal logic blocking(underlying pipes hard 
> closed)
> * added lock around queues that will prevent from using queues before new one 
> will be created in case of recreating executor process
> 
> 
> Diffs
> -
> 
>   ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
>   ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
>   ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
>   ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
>   ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
>   ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
>   ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 
> 
> 
> Diff: https://reviews.apache.org/r/57344/diff/2/
> 
> 
> Testing
> ---
> 
> mvn clean test, manual testing
> 
> 
> Thanks,
> 
> Eugene Chekanskiy
> 
>

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Eugene Chekanskiy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/
---

(Updated March 7, 2017, 3:28 p.m.)


Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
Robert Levas, and Sebastian Toader.


Bugs: AMBARI-20323
https://issues.apache.org/jira/browse/AMBARI-20323


Repository: ambari


Description
---

All status command execution logic moved under one file.

* added protection for hardkilling(now process tries to die gracefully)
* added protection for queue internal logic blocking(underlying pipes hard 
closed)
* added lock around queues that will prevent from using queues before new one 
will be created in case of recreating executor process


Diffs (updated)
-

  ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
  ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
  ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
  ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
  ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
  ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
  ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 


Diff: https://reviews.apache.org/r/57344/diff/2/

Changes: https://reviews.apache.org/r/57344/diff/1-2/


Testing
---

mvn clean test, manual testing


Thanks,

Eugene Chekanskiy

Re: Review Request 57344: Commands timed-out on ambari host without any error logs

[Sebastian Toader]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/#review168101
---




ambari-agent/src/main/python/ambari_agent/Controller.py
Line 208 (original), 205 (patched)


"INITIAL_START" might be confusing as the server may request the agent to 
re-register. Eg. the server was restarted, or agent lost heartbeat etc.



ambari-agent/src/main/python/ambari_agent/Controller.py
Lines 479-480 (patched)


Isn't this the same as 
```self.statusCommandsExecutor.relaunch("INITIAL_START")``` in ```def 
registerWithServer(self):```? 

Would it make sense to create ```StatusCommandsExecutor``` in ```def 
registerWithServer(self):``` just before calling 
```self.statusCommandsExecutor.relaunch("INITIAL_START")``` if hasn't been 
instantiated yet?



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Line 43 (original), 40 (patched)


Comment what this lock is used for.



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 51 (patched)


This doesn't seem to be used?



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 60 (patched)


Messages needs to be logged to ambari-agent.log as there are environments 
logs are collected from ambari-agent.log and published into a central log 
collector.

Perhaps the parent process could read the out file and channel the content 
into ambari-agent.log.



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 94 (patched)


You could use the timed_out property.



ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py
Lines 158 (patched)


Could this deadlock in case the worker process is killed from outside thus 
being shut down without properly closing the mp_result_queue?



ambari-agent/src/main/python/ambari_agent/main.py
Lines 317-318 (original), 318-319 (patched)


How this ensures that status command executor is re-launched in case is 
killedd from oputside?


- Sebastian Toader


On March 6, 2017, 6:45 p.m., Eugene Chekanskiy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57344/
> ---
> 
> (Updated March 6, 2017, 6:45 p.m.)
> 
> 
> Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
> Robert Levas, and Sebastian Toader.
> 
> 
> Bugs: AMBARI-20323
> https://issues.apache.org/jira/browse/AMBARI-20323
> 
> 
> Repository: ambari
> 
> 
> Description
> ---
> 
> All status command execution logic moved under one file.
> 
> * added protection for hardkilling(now process tries to die gracefully)
> * added protection for queue internal logic blocking(underlying pipes hard 
> closed)
> * added lock around queues that will prevent from using queues before new one 
> will be created in case of recreating executor process
> 
> 
> Diffs
> -
> 
>   ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
>   ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
>   ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
>   ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
>   ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
>   ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
>   ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 
> 
> 
> Diff: https://reviews.apache.org/r/57344/diff/1/
> 
> 
> Testing
> ---
> 
> mvn clean test, manual testing
> 
> 
> Thanks,
> 
> Eugene Chekanskiy
> 
>

Review Request 57344: Commands timed-out on ambari host without any error logs

[Eugene Chekanskiy]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57344/
---

Review request for Ambari, Attila Doroszlai, Andrew Onischuk, Dmytro Sen, 
Robert Levas, and Sebastian Toader.


Bugs: AMBARI-20323
https://issues.apache.org/jira/browse/AMBARI-20323


Repository: ambari


Description
---

All status command execution logic moved under one file.

* added protection for hardkilling(now process tries to die gracefully)
* added protection for queue internal logic blocking(underlying pipes hard 
closed)
* added lock around queues that will prevent from using queues before new one 
will be created in case of recreating executor process


Diffs
-

  ambari-agent/src/main/python/ambari_agent/ActionQueue.py 5300b52 
  ambari-agent/src/main/python/ambari_agent/Controller.py 61a74e6 
  ambari-agent/src/main/python/ambari_agent/StatusCommandsExecutor.py 2f15770 
  ambari-agent/src/main/python/ambari_agent/main.py 3f333c4 
  ambari-agent/src/test/python/ambari_agent/TestActionQueue.py 9fefefb 
  ambari-agent/src/test/python/ambari_agent/TestController.py 663e215 
  ambari-agent/src/test/python/ambari_agent/TestMain.py 97c448b 


Diff: https://reviews.apache.org/r/57344/diff/1/


Testing
---

mvn clean test, manual testing


Thanks,

Eugene Chekanskiy