You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Armand Grillet <ag...@mesosphere.io> on 2018/02/04 19:31:38 UTC

Re: Review Request 65246: Added download button for master and agent logs in Web UI.

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

(Updated Feb. 4, 2018, 7:31 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Changes
-------

Resolved issues.


Bugs: MESOS-8454
    https://issues.apache.org/jira/browse/MESOS-8454


Repository: mesos


Description
-------

See summary.


Diffs (updated)
-----

  src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
  src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
  src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 


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

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


Testing (updated)
-------

Tested using Google Chrome 64.0.3282.119.

Created an High Availability Mode Mesos cluster locally:
```
$ zkServer start
$ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
$ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
$ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
```
Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.

New UI (masters):
![New logs button master](https://i.imgur.com/Uz8aj1H.png)

New UI (agents):
![New logs button](https://i.imgur.com/tmGavCL.png)


Thanks,

Armand Grillet


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Armand Grillet <ag...@mesosphere.io>.

> On Feb. 27, 2018, 11:06 p.m., Benjamin Mahler wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 562 (patched)
> > <https://reviews.apache.org/r/65246/diff/5/?file=1966031#file1966031line569>
> >
> >     Why did you need the additional query into the /flags here but not in agent? These fields should still be in $scope.state, no?

I should have added more information because it seems to be a bug in Mesos. Getting the `state` of a master returns the state of the leader master, even if the hostname used is not the one of the leader master. Parsing the `flags` endpoint correctly returns the flags of the master we want to reach. This problem did not appear when communicating with agents.


> On Feb. 27, 2018, 11:06 p.m., Benjamin Mahler wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 571-572 (patched)
> > <https://reviews.apache.org/r/65246/diff/5/?file=1966031#file1966031line578>
> >
> >     Why the `$('#alert').show()` here instead of the `$dialog.messageBox(...).open()` as was done before? I'm not familiar with the difference, should the UI use a consistent approach?

Indeed, we used two different approaches in the past. `$('#alert').show();` is used many times to display alerts, while we were using a `$dialog` before this patch only in one case: if a user wanted to get unaccessible logs. The difference is that an alert is displayed when there is an error that was not expected (e.g. when reaching an endpoint) while a dialog was displayed when a user tried to do something he cannot do due to a configuration choice. 

I have removed the two uses of `$dialog` as they were not constructive, displaying buttons to access something that is not accessible and showing an alert being a strange user experience. `$dialog` is thus not used anymore following this patch, I have not removed the code supporting that feature as I think it makes sense for many use cases that are just not there yet.


- Armand


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


On Feb. 27, 2018, 11:40 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2018, 11:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds a new button to download the logs in addition to the ability to
> view them. Does not display the buttons if the master or agent does not
> have an external log file or a log directory. Displays the correct logs,
> i.e. if you are on a non-leading master you will get the logs of that
> master and not the leading master.
> 
> Regarding the architectural choices, `HomeCtrl` is now similar to the
> other controllers with an update function called when the state changes.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/6/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Benjamin Mahler <bm...@apache.org>.

> On Feb. 27, 2018, 11:06 p.m., Benjamin Mahler wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 562 (patched)
> > <https://reviews.apache.org/r/65246/diff/5/?file=1966031#file1966031line569>
> >
> >     Why did you need the additional query into the /flags here but not in agent? These fields should still be in $scope.state, no?
> 
> Armand Grillet wrote:
>     I should have added more information because it seems to be a bug in Mesos. Getting the `state` of a master returns the state of the leader master, even if the hostname used is not the one of the leader master. Parsing the `flags` endpoint correctly returns the flags of the master we want to reach. This problem did not appear when communicating with agents.

Ok, I'll add a comment that clarifies this and will get this committed.


- Benjamin


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


On Feb. 27, 2018, 11:40 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2018, 11:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds a new button to download the logs in addition to the ability to
> view them. Does not display the buttons if the master or agent does not
> have an external log file or a log directory. Displays the correct logs,
> i.e. if you are on a non-leading master you will get the logs of that
> master and not the leading master.
> 
> Regarding the architectural choices, `HomeCtrl` is now similar to the
> other controllers with an update function called when the state changes.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/6/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/#review198345
-----------------------------------------------------------


Fix it, then Ship it!




Thanks Armand! Looks pretty good now, just a few comments below to get through then I should be able to get this committed for you.


src/webui/master/static/js/controllers.js
Lines 562 (patched)
<https://reviews.apache.org/r/65246/#comment278493>

    



src/webui/master/static/js/controllers.js
Line 565 (original), 562-573 (patched)
<https://reviews.apache.org/r/65246/#comment278494>

    



src/webui/master/static/js/controllers.js
Line 565 (original), 562-573 (patched)
<https://reviews.apache.org/r/65246/#comment278495>

    



src/webui/master/static/js/controllers.js
Lines 562 (patched)
<https://reviews.apache.org/r/65246/#comment278496>

    Why did you need the additional query into the /flags here but not in agent? These fields should still be in $scope.state, no?



src/webui/master/static/js/controllers.js
Line 565 (original), 564-568 (patched)
<https://reviews.apache.org/r/65246/#comment278483>

    Ditto here.



src/webui/master/static/js/controllers.js
Lines 571-572 (patched)
<https://reviews.apache.org/r/65246/#comment278491>

    Why the `$('#alert').show()` here instead of the `$dialog.messageBox(...).open()` as was done before? I'm not familiar with the difference, should the UI use a consistent approach?



src/webui/master/static/js/controllers.js
Lines 696-702 (patched)
<https://reviews.apache.org/r/65246/#comment278482>

    Logging enabled is a little misleading of a name here since we're still logging when these conditions are not held. How about something like `log_file_attached`?
    
    Can we avoid the if condition?
    
    ```
    // The agent attaches a "/slave/log" file when either
    // of these flags are set.
    $scope.agent.log_file_attached = $scope.state.external_log_file || $scope.state.log_dir;
    ```


- Benjamin Mahler


On Feb. 27, 2018, 3:31 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2018, 3:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds a new button to download the logs in addition to the ability to
> view them. Does not display the buttons if the master or agent does not
> have an external log file or a log directory. Displays the correct logs,
> i.e. if you are on a non-leading master you will get the logs of that
> master and not the leading master.
> 
> Regarding the architectural choices, `HomeCtrl` is now similar to the
> other controllers with an update function called when the state changes.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/5/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/#review198332
-----------------------------------------------------------



PASS: Mesos patch 65246 was successfully built and tested.

Reviews applied: `['65246']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65246

- Mesos Reviewbot Windows


On Feb. 27, 2018, 4:31 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2018, 4:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds a new button to download the logs in addition to the ability to
> view them. Does not display the buttons if the master or agent does not
> have an external log file or a log directory. Displays the correct logs,
> i.e. if you are on a non-leading master you will get the logs of that
> master and not the leading master.
> 
> Regarding the architectural choices, `HomeCtrl` is now similar to the
> other controllers with an update function called when the state changes.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/5/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/#review198363
-----------------------------------------------------------



PASS: Mesos patch 65246 was successfully built and tested.

Reviews applied: `['65246']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65246

- Mesos Reviewbot Windows


On Feb. 27, 2018, 11:40 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2018, 11:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds a new button to download the logs in addition to the ability to
> view them. Does not display the buttons if the master or agent does not
> have an external log file or a log directory. Displays the correct logs,
> i.e. if you are on a non-leading master you will get the logs of that
> master and not the leading master.
> 
> Regarding the architectural choices, `HomeCtrl` is now similar to the
> other controllers with an update function called when the state changes.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/6/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/
-----------------------------------------------------------

(Updated Feb. 27, 2018, 11:40 p.m.)


Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.


Changes
-------

Rebased and issue fixed.


Bugs: MESOS-8454
    https://issues.apache.org/jira/browse/MESOS-8454


Repository: mesos


Description
-------

Adds a new button to download the logs in addition to the ability to
view them. Does not display the buttons if the master or agent does not
have an external log file or a log directory. Displays the correct logs,
i.e. if you are on a non-leading master you will get the logs of that
master and not the leading master.

Regarding the architectural choices, `HomeCtrl` is now similar to the
other controllers with an update function called when the state changes.


Diffs (updated)
-----

  src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
  src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
  src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 


Diff: https://reviews.apache.org/r/65246/diff/6/

Changes: https://reviews.apache.org/r/65246/diff/5-6/


Testing
-------

Tested using Google Chrome 64.0.3282.119.

Created an High Availability Mode Mesos cluster locally:
```
$ zkServer start
$ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
$ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
$ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
```
Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.

New UI (masters):
![New logs button master](https://i.imgur.com/Uz8aj1H.png)

New UI (agents):
![New logs button](https://i.imgur.com/tmGavCL.png)


Thanks,

Armand Grillet


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/
-----------------------------------------------------------

(Updated Feb. 27, 2018, 3:31 p.m.)


Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.


Changes
-------

Updated architecture and patch description to have a simpler logic similar to the one in the other controllers.


Bugs: MESOS-8454
    https://issues.apache.org/jira/browse/MESOS-8454


Repository: mesos


Description (updated)
-------

Adds a new button to download the logs in addition to the ability to
view them. Does not display the buttons if the master or agent does not
have an external log file or a log directory. Displays the correct logs,
i.e. if you are on a non-leading master you will get the logs of that
master and not the leading master.

Regarding the architectural choices, `HomeCtrl` is now similar to the
other controllers with an update function called when the state changes.


Diffs (updated)
-----

  src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
  src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
  src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 


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

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


Testing
-------

Tested using Google Chrome 64.0.3282.119.

Created an High Availability Mode Mesos cluster locally:
```
$ zkServer start
$ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
$ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
$ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
```
Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.

New UI (masters):
![New logs button master](https://i.imgur.com/Uz8aj1H.png)

New UI (agents):
![New logs button](https://i.imgur.com/tmGavCL.png)


Thanks,

Armand Grillet


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/#review196919
-----------------------------------------------------------



PASS: Mesos patch 65246 was successfully built and tested.

Reviews applied: `['65246']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65246

- Mesos Reviewbot Windows


On Feb. 6, 2018, 6:43 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2018, 6:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/4/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Armand Grillet <ag...@mesosphere.io>.

> On Feb. 15, 2018, 7:23 p.m., Benjamin Mahler wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 578-585 (patched)
> > <https://reviews.apache.org/r/65246/diff/4/?file=1953485#file1953485line578>
> >
> >     I'm puzzled about what's going on here, why do we need to stream the data through javascript and create a file hyperlink and click it? Why didn't we have to do this for the sandbox download links?

Because I wanted to set the name of the file downloaded which is not necessary with the sandbox download links. We cannot use the HTML attribute `download` as browsers do not honor the download attribute suggested filename for cross origin requests. After the last revision, the name of the files will be something like `mesos-master.Mac.Armand.log.INFO.20180226-105655` which is not optimal (no file extension, too much information in the name).


- Armand


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


On Feb. 27, 2018, 3:31 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2018, 3:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds a new button to download the logs in addition to the ability to
> view them. Does not display the buttons if the master or agent does not
> have an external log file or a log directory. Displays the correct logs,
> i.e. if you are on a non-leading master you will get the logs of that
> master and not the leading master.
> 
> Regarding the architectural choices, `HomeCtrl` is now similar to the
> other controllers with an update function called when the state changes.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/5/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/#review197631
-----------------------------------------------------------




src/webui/master/static/js/controllers.js
Lines 578-585 (patched)
<https://reviews.apache.org/r/65246/#comment277852>

    I'm puzzled about what's going on here, why do we need to stream the data through javascript and create a file hyperlink and click it? Why didn't we have to do this for the sandbox download links?



src/webui/master/static/js/controllers.js
Lines 707 (patched)
<https://reviews.apache.org/r/65246/#comment277853>

    Why does this one use read instead of download, is it due to cross origin resource sharing? Why didn't we have to do this for the sandbox download links?


- Benjamin Mahler


On Feb. 6, 2018, 6:43 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2018, 6:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/4/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/
-----------------------------------------------------------

(Updated Feb. 6, 2018, 6:43 p.m.)


Review request for mesos, Benjamin Mahler and Vinod Kone.


Changes
-------

Fixed last issue.


Bugs: MESOS-8454
    https://issues.apache.org/jira/browse/MESOS-8454


Repository: mesos


Description
-------

See summary.


Diffs (updated)
-----

  src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
  src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
  src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 


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

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


Testing
-------

Tested using Google Chrome 64.0.3282.119.

Created an High Availability Mode Mesos cluster locally:
```
$ zkServer start
$ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
$ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
$ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
```
Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.

New UI (masters):
![New logs button master](https://i.imgur.com/Uz8aj1H.png)

New UI (agents):
![New logs button](https://i.imgur.com/tmGavCL.png)


Thanks,

Armand Grillet


Re: Review Request 65246: Added download button for master and agent logs in Web UI.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65246/#review196795
-----------------------------------------------------------



PASS: Mesos patch 65246 was successfully built and tested.

Reviews applied: `['65246']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65246

- Mesos Reviewbot Windows


On Feb. 4, 2018, 7:31 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65246/
> -----------------------------------------------------------
> 
> (Updated Feb. 4, 2018, 7:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-8454
>     https://issues.apache.org/jira/browse/MESOS-8454
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/webui/master/static/agent.html 908d01d1d28f2a4fc595b581fa16515c5be1410a 
>   src/webui/master/static/home.html df3eef61eb9564dbdfc3d138ecfd18e85f747f11 
>   src/webui/master/static/js/controllers.js 59665869dbea77c00740e42f2590473181dfe2fe 
> 
> 
> Diff: https://reviews.apache.org/r/65246/diff/3/
> 
> 
> Testing
> -------
> 
> Tested using Google Chrome 64.0.3282.119.
> 
> Created an High Availability Mode Mesos cluster locally:
> ```
> $ zkServer start
> $ bash mesos-master.sh --port='5061' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master1' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master1-log'
> $ bash mesos-master.sh --port='5062' --zk='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/master2' --quorum=1 --webui_dir='/Users/Armand/Code/apache-mesos/src/webui' --log_dir='/tmp/master2-log'
> $ bash mesos-agent.sh --port='5051' --master='zk://127.0.0.1:2181/mesos' --work_dir='/tmp/agent1' --log_dir='/tmp/agent1-log'
> ```
> Tested the download and streaming features on the home and agents Web UI endpoints, from both `http://localhost:5061/` and `http://localhost:5062/`.
> 
> New UI (masters):
> ![New logs button master](https://i.imgur.com/Uz8aj1H.png)
> 
> New UI (agents):
> ![New logs button](https://i.imgur.com/tmGavCL.png)
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>