Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.

[Greg Mann]

> On Nov. 8, 2019, 12:33 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp
> > Lines 24-28 (patched)
> > 
> >
> > What do you think about making `SSLSocketWrapper` a true wrapper of the 
> > underlying socket? In other words, let's make the wrapper a subclass of 
> > `SocketImpl` and have it hold a `SocketImpl` member which is the underlying 
> > socket that it's using for communication. We could add a `create()` method 
> > and a constructor like this:
> > ```
> > Try> SSLSocketWrapper::create(const 
> > SocketImpl& socketImpl_)
> > {
> >   openssl::initialize();
> > 
> >   if (!openssl::flags().enabled) {
> > return Error("SSL is disabled");
> >   }
> > 
> >   return std::make_shared(socketImpl_);
> > }
> > 
> > 
> > SSLSocketWrapper::SSLSocketWrapper(const SocketImpl& socketImpl_)
> >   : socketImpl(socketImpl_),
> > ssl(nullptr) {}
> > ```
> > 
> > then we could call methods like `connect()` and `accept()` directly on 
> > the held socket within the wrapper code.
> > 
> > It's a small structural change, but I think it would help to 
> > communicate to readers how the underlying socket and the SSL wrapper are 
> > related. Further, it makes it more obvious how we could evolve the wrapper 
> > class into some kind of `Server` and `Client` objects which are initialized 
> > with a socket.
> > 
> > WDYT?

The `SocketWrapperBIOData` could also hold a `SocketImpl` member instead of an 
`int_fd`, right?


- Greg


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


On Oct. 24, 2019, 1:41 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> ---
> 
> (Updated Oct. 24, 2019, 1:41 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
> 
> The new class is used whenever SSL is enabled, but Libevent is not.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 40c8ef935104bab4ea2f8b2b5919450b55165f60 
>   3rdparty/libprocess/src/openssl.cpp 
> bd05866950e1043d9585a7c5fdc7b2147a233fd3 
>   3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135 
>   3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71660/diff/2/
> 
> 
> Testing
> ---
> 
> cmake .. -ENABLE_SSL=1   # No Libevent here!
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71660: SSL Wrapper: Stubbed out a SSL socket class.

[Greg Mann]

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




3rdparty/libprocess/src/CMakeLists.txt
Lines 101-103 (patched)


Need to update autotools build also? Or is this Windows-only for now?



3rdparty/libprocess/src/ssl/socket_wrapper.hpp
Lines 24-28 (patched)


What do you think about making `SSLSocketWrapper` a true wrapper of the 
underlying socket? In other words, let's make the wrapper a subclass of 
`SocketImpl` and have it hold a `SocketImpl` member which is the underlying 
socket that it's using for communication. We could add a `create()` method and 
a constructor like this:
```
Try> SSLSocketWrapper::create(const SocketImpl& 
socketImpl_)
{
  openssl::initialize();

  if (!openssl::flags().enabled) {
return Error("SSL is disabled");
  }

  return std::make_shared(socketImpl_);
}

SSLSocketWrapper::SSLSocketWrapper(const SocketImpl& socketImpl_)
  : socketImpl(socketImpl_),
ssl(nullptr) {}
```

then we could call methods like `connect()` and `accept()` directly on the 
held socket within the wrapper code.

It's a small structural change, but I think it would help to communicate to 
readers how the underlying socket and the SSL wrapper are related. Further, it 
makes it more obvious how we could evolve the wrapper class into some kind of 
`Server` and `Client` objects which are initialized with a socket.

WDYT?


- Greg Mann


On Oct. 24, 2019, 1:41 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71660/
> ---
> 
> (Updated Oct. 24, 2019, 1:41 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This creates some new files implementing the SocketImpl class,
> in preparation for implementing generic SSL sockets.
> 
> The new class is used whenever SSL is enabled, but Libevent is not.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 40c8ef935104bab4ea2f8b2b5919450b55165f60 
>   3rdparty/libprocess/src/openssl.cpp 
> bd05866950e1043d9585a7c5fdc7b2147a233fd3 
>   3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135 
>   3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION 
>   3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71660/diff/2/
> 
> 
> Testing
> ---
> 
> cmake .. -ENABLE_SSL=1   # No Libevent here!
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71659: SSL Wrapper: Allowed SSL without libevent.

[Greg Mann]

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




cmake/CompilationConfigure.cmake
Lines 263-268 (original)


We need to update the autotools build to allow this as well, right?



docs/ssl.md
Line 7 (original), 7 (patched)


The 'enable-ssl' section of 'docs/configuration/autotools.md' needs to be 
updated as well.



docs/ssl.md
Line 15 (original), 18 (patched)


s/only// ?



docs/ssl.md
Lines 17-19 (original), 33-35 (patched)


Let's be more explicit here that users can use `enable-ssl` both with or 
without `enable-libevent`.


- Greg Mann


On Oct. 23, 2019, 7:37 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71659/
> ---
> 
> (Updated Oct. 23, 2019, 7:37 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-10009
> https://issues.apache.org/jira/browse/MESOS-10009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes the configure-time check on having both
> ENABLE_SSL and ENABLE_LIBEVENT set to true in order to have
> SSL sockets.  The subsequent commits will add SSL support
> to the poll socket class as a wrapper.
> 
> This also updates the related documentation for SSL,
> including on Windows.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 62cb23e81abb2c7e4e8e13c83b45afb98708bd30 
>   docs/ssl.md f6beb4211f0c17fdaa4b52425cec405c9b6a7d56 
>   docs/windows.md 35b12ddedec6f85b75adefa80d38fa439cd6b2d3 
> 
> 
> Diff: https://reviews.apache.org/r/71659/diff/1/
> 
> 
> Testing
> ---
> 
> Requires a libprocess build change too (next patch).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 71735: Added 'nodiscard' attribute to some member functions of Resources.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [71734, 71735]

Failed command: ['bash', '-c', "set -o pipefail; export OS='ubuntu:14.04' 
BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose 
--disable-libtool-wrappers --disable-parallel-test-execution' 
ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh 2>&1 | tee 
build_71735"]

Error:
..
chical.cpp:1625] Framework d2c0f3d0-d637-4928-a83d-e6f1f803763a- filtered 
agent d2c0f3d0-d637-4928-a83d-e6f1f803763a-S0 for 5secs
I1107 17:15:32.903017 18799 master.cpp:12589] Sending operation '' (uuid: 
f6b41846-a77b-4008-8c14-c5311bc1bf09) to agent 
d2c0f3d0-d637-4928-a83d-e6f1f803763a-S0 at slave(1247)@172.17.0.4:39947 
(791d85515a04)
I1107 17:15:32.903573 18804 slave.cpp:4352] Ignoring new checkpointed resources 
and operations identical to the current version
I1107 17:15:32.906338 18802 provider.cpp:498] Received APPLY_OPERATION event
I1107 17:15:32.906379 18802 provider.cpp:1351] Received RESERVE operation '' 
(uuid: f6b41846-a77b-4008-8c14-c5311bc1bf09)
I1107 17:15:32.920514 18799 status_update_manager_process.hpp:152] Received 
operation status update OPERATION_FINISHED (Status UUID: 
7cd79696-a316-4286-a4e6-f45fc42c62bf) for operation UUID 
f6b41846-a77b-4008-8c14-c5311bc1bf09 on agent 
d2c0f3d0-d637-4928-a83d-e6f1f803763a-S0
I1107 17:15:32.920583 18799 status_update_manager_process.hpp:414] Creating 
operation status update stream f6b41846-a77b-4008-8c14-c5311bc1bf09 
checkpoint=true
I1107 17:15:32.920883 18799 status_update_manager_process.hpp:929] 
Checkpointing UPDATE for operation status update OPERATION_FINISHED (Status 
UUID: 7cd79696-a316-4286-a4e6-f45fc42c62bf) for operation UUID 
f6b41846-a77b-4008-8c14-c5311bc1bf09 on agent 
d2c0f3d0-d637-4928-a83d-e6f1f803763a-S0
I1107 17:15:32.937435 18799 status_update_manager_process.hpp:528] Forwarding 
operation status update OPERATION_FINISHED (Status UUID: 
7cd79696-a316-4286-a4e6-f45fc42c62bf) for operation UUID 
f6b41846-a77b-4008-8c14-c5311bc1bf09 on agent 
d2c0f3d0-d637-4928-a83d-e6f1f803763a-S0
I1107 17:15:32.938262 18803 http_connection.hpp:131] Sending 
UPDATE_OPERATION_STATUS call to 
http://172.17.0.4:39947/slave(1247)/api/v1/resource_provider
I1107 17:15:32.939275 18793 process.cpp:3671] Handling HTTP event for process 
'slave(1247)' with path: '/slave(1247)/api/v1/resource_provider'
I1107 17:15:32.942592 18792 hierarchical.cpp:1853] Performed allocation for 1 
agents in 1.249097ms
I1107 17:15:32.943356 18794 master.cpp:10427] Sending offers [ 
d2c0f3d0-d637-4928-a83d-e6f1f803763a-O3 ] to framework 
d2c0f3d0-d637-4928-a83d-e6f1f803763a- (default) at 
scheduler-d84d8700-40bf-481e-afbe-e5ec4fce662d@172.17.0.4:39947
I1107 17:15:32.943956 18807 sched.cpp:934] Scheduler::resourceOffers took 
66154ns
I1107 17:15:32.946669 18798 process.cpp:3671] Handling HTTP event for process 
'master' with path: '/master/api/v1'
I1107 17:15:32.948333 18795 http.cpp:1115] HTTP POST for /master/api/v1 from 
172.17.0.4:48928
I1107 17:15:32.948576 18795 http.cpp:263] Processing call CREATE_VOLUMES
I1107 17:15:32.949432 18795 master.cpp:3938] Authorizing principal 
'test-principal' to create volumes 
'[{"disk":{"persistence":{"id":"09e86a6c-aee9-4868-8f78-bc38b51e91bf","principal":"test-principal"},"source":{"id":"/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_xFSogm/2GB-2d79ba2f-8efc-4ebc-89d1-d942fde9fb29","mount":{"root":"./csi/org.apache.mesos.csi.test/local/mounts"},"profile":"test","type":"MOUNT","vendor":"org.apache.mesos.csi.test.local"},"volume":{"container_path":"volume","mode":"RW"}},"name":"disk","provider_id":{"value":"cee03e8d-a980-4744-b7e9-9d40c2b2df65"},"reservations":[{"role":"storage","type":"DYNAMIC"},{"principal":"test-principal","role":"storage/default-role","type":"DYNAMIC"}],"scalar":{"value":2048.0},"type":"SCALAR"}]'
I1107 17:15:32.950933 18807 sched.cpp:960] Rescinded offer 
d2c0f3d0-d637-4928-a83d-e6f1f803763a-O3
I1107 17:15:32.951045 18807 sched.cpp:971] Scheduler::offerRescinded took 
56333ns
I1107 17:15:32.951463 18796 hierarchical.cpp:1576] Recovered ports(allocated: 
storage/default-role):[31000-32000]; disk(allocated: 
storage/default-role)(reservations: 
[(DYNAMIC,storage),(DYNAMIC,storage/default-role,test-principal)])[MOUNT(org.apache.mesos.csi.test.local,/tmp/CSIVersion_StorageLocalResourceProviderTest_OperatorOperationsWithResourceProviderResources_v1_xFSogm/2GB-2d79ba2f-8efc-4ebc-89d1-d942fde9fb29,test)]:2048;
 cpus(allocated: storage/default-role):2; mem(allocated: 
storage/default-role):1024; disk(allocated: storage/default-role):1024 (total: 
cpus:2; mem:1024; disk:1024; ports:[31000-32000]; disk(reservations: 

Re: Review Request 71735: Added 'nodiscard' attribute to some member functions of Resources.

[Benno Evers]

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

(Updated Nov. 7, 2019, 2:37 p.m.)


Review request for mesos, Benjamin Bannier and Greg Mann.


Changes
---

Reformatted line breaks.


Summary (updated)
-

Added 'nodiscard' attribute to some member functions of Resources.


Repository: mesos


Description
---

Added the `[[nodiscard]]` attribute to `Resources::pushReservation()`
and `Resources::popReservation()`, in order to prevent mistakes by
authors incorrectly assuming that these functions would modify the
resources objects in place.


Diffs (updated)
-

  include/mesos/resources.hpp b8aef28e08f85c87bb78f25a64b0d7318f2727cc 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 71735: Added 'nodiscard' attribute to some Resources member functions.

[Benjamin Bannier]

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


Ship it!




Thanks for this.

As for the formatting, personally I'd prefer whatever formatting our 
`clang-format` does, if only to remove the human element from this as much as 
possible.

- Benjamin Bannier


On Nov. 6, 2019, 3:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71735/
> ---
> 
> (Updated Nov. 6, 2019, 3:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the `[[nodiscard]]` attribute to `Resources::pushReservation()`
> and `Resources::popReservation()`, in order to prevent mistakes by
> authors incorrectly assuming that these functions would modify the
> resources objects in place.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp b8aef28e08f85c87bb78f25a64b0d7318f2727cc 
> 
> 
> Diff: https://reviews.apache.org/r/71735/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71722: Improved error reporting in 'Resources::pushReservation()'.

[Benjamin Bannier]

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


Ship it!




Thanks for this, I remember having patched that in for development repeatedly.

- Benjamin Bannier


On Nov. 5, 2019, 6:05 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71722/
> ---
> 
> (Updated Nov. 5, 2019, 6:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure to print the actual error message after an assertion
> failure in `Resources::pushReservations()`.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp bfa9f3ea7e8c3e2dc9b4c4f7c86ad29b0de81f24 
> 
> 
> Diff: https://reviews.apache.org/r/71722/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71734: Revamped attribute handling in stout.

[Benno Evers]

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

(Updated Nov. 7, 2019, 2:23 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.


Changes
---

Incorporated feedback.


Repository: mesos


Description
---

This makes several changes to the attribute compatibility layer
provided by stout:

  * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler
support for a given attribute.
  * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
  * Preferred the use of the standardized `[[noreturn]]` syntax
if supported by the compiler.
  * Fixed previous usages of `NORETURN` in the stout codebase.
  * Added support for the `[[nodiscard]]` attribute.


Diffs (updated)
-

  3rdparty/stout/include/stout/abort.hpp 
43ed5ce2830c493e4c801cc81f8dde0922c99a8d 
  3rdparty/stout/include/stout/attributes.hpp 
aa377db82e1dbdb8727b1128780e2409accc8ae9 
  3rdparty/stout/include/stout/exit.hpp 
34585a005063b17d0c7754c8e8c13f0641383bc4 
  3rdparty/stout/include/stout/unimplemented.hpp 
ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7 
  3rdparty/stout/include/stout/unreachable.hpp 
d4b3bb0582eb9e64e6f150735d1e9f2956edbca6 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 71729: Added authorization handling for reservations with `source`.

[Benno Evers]

> On Nov. 6, 2019, 4:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > 
> >
> > It doesn't seem ideal to have recursively nested calls to 
> > `collectauthorizations()`, even if the logic is sound it seems hard to 
> > reason about.
> > 
> > Would it be possible to eliminate the branching by setting `source` to 
> > be `resources.popReservation()` if `source` is empty?
> 
> Benjamin Bannier wrote:
> > Would it be possible to eliminate the branching by setting source to be 
> resources.popReservation() if source is empty?
> 
> This would only work if we know that all resources passed to `RESERVE` 
> are indeed reserved. Unfortunately that is not the case in the current 
> implementation (e.g., `cpus(A):1;mem:256` would reserve only `cpus`). We need 
> to keep support for that behavior as it is part of the APII.
> 
> In the patch I put up we go from the narrower extended API (e.g., all 
> resources passed to `RESERVE` must have identical reservations) to the wider 
> existing API so we are good. Going from wider to narrower doesn't work, 
> though.
> 
> What I could do for the sake of readibility would be to introduce a 
> dedicated function for the legacy behavior to avoid the self-recursion. I am 
> not sure that would help (and might it even make harder to follow the code).
> 
> WDYT?

Intuitively introducing a dedicated function sounds cleaner to me, but the 
self-recursion should be fine as well if there's no easy way to avoid it. Maybe 
it would be good to add some of the reasoning above to the comment, though.


- Benno


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


On Nov. 7, 2019, 11 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> ---
> 
> (Updated Nov. 7, 2019, 11 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56 
>   src/tests/master_authorization_tests.cpp 
> 06471aa7779d399f4474ed40db3fbcc60b8298b2 
> 
> 
> Diff: https://reviews.apache.org/r/71729/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

[GitHub] [mesos] ArmandGrillet commented on a change in pull request #345: Allow `mesos task exec/attach` for any task_id

[GitBox]

ArmandGrillet commented on a change in pull request #345: Allow `mesos task 
exec/attach` for any task_id
URL: https://github.com/apache/mesos/pull/345#discussion_r343623804
 
 

 ##
 File path: src/python/cli_new/lib/cli/http.py
 ##
 @@ -41,6 +41,8 @@ def read_endpoint(addr, endpoint):
 
 try:
 url = "{addr}/{endpoint}".format(addr=addr, endpoint=endpoint)
+if query is not None:
+  url += "?{query}".format(query=urllib.parse.urlencode(query))
 
 Review comment:
   Checking again, our timing was quite unfortunate today .


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] brugidou commented on a change in pull request #345: Allow `mesos task exec/attach` for any task_id

[GitBox]

brugidou commented on a change in pull request #345: Allow `mesos task 
exec/attach` for any task_id
URL: https://github.com/apache/mesos/pull/345#discussion_r343619740
 
 

 ##
 File path: src/python/cli_new/lib/cli/http.py
 ##
 @@ -41,6 +41,8 @@ def read_endpoint(addr, endpoint):
 
 try:
 url = "{addr}/{endpoint}".format(addr=addr, endpoint=endpoint)
+if query is not None:
+  url += "?{query}".format(query=urllib.parse.urlencode(query))
 
 Review comment:
   I fixed all the linting, and added a commit to fix an optional parameter 
ordering issue.
   
   However I was not able to pass tests before my changes.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [mesos] ArmandGrillet commented on a change in pull request #345: Allow `mesos task exec/attach` for any task_id

[GitBox]

ArmandGrillet commented on a change in pull request #345: Allow `mesos task 
exec/attach` for any task_id
URL: https://github.com/apache/mesos/pull/345#discussion_r343617448
 
 

 ##
 File path: src/python/cli_new/lib/cli/http.py
 ##
 @@ -41,6 +41,8 @@ def read_endpoint(addr, endpoint):
 
 try:
 url = "{addr}/{endpoint}".format(addr=addr, endpoint=endpoint)
+if query is not None:
+  url += "?{query}".format(query=urllib.parse.urlencode(query))
 
 Review comment:
   lib/cli/http.py:45:0: W0311: Bad indentation. Found 10 spaces, expected 12 
(bad-indentation)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Review Request 71725: Added end-to-end test for operator API reservation updates.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [71725, 71729]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On Nov. 6, 2019, 3:54 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71725/
> ---
> 
> (Updated Nov. 6, 2019, 3:54 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9992
> https://issues.apache.org/jira/browse/MESOS-9992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new test to verify that reservations can be updated
> using the operator API.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp bd207eaebc8fc14de16f7af633d943b315328e8a 
> 
> 
> Diff: https://reviews.apache.org/r/71725/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 71729: Added authorization handling for reservations with `source`.

[Benjamin Bannier]

> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3810 (patched)
> > 
> >
> > It doesn't seem ideal to have recursively nested calls to 
> > `collectauthorizations()`, even if the logic is sound it seems hard to 
> > reason about.
> > 
> > Would it be possible to eliminate the branching by setting `source` to 
> > be `resources.popReservation()` if `source` is empty?

> Would it be possible to eliminate the branching by setting source to be 
> resources.popReservation() if source is empty?

This would only work if we know that all resources passed to `RESERVE` are 
indeed reserved. Unfortunately that is not the case in the current 
implementation (e.g., `cpus(A):1;mem:256` would reserve only `cpus`). We need 
to keep support for that behavior as it is part of the APII.

In the patch I put up we go from the narrower extended API (e.g., all resources 
passed to `RESERVE` must have identical reservations) to the wider existing API 
so we are good. Going from wider to narrower doesn't work, though.

What I could do for the sake of readibility would be to introduce a dedicated 
function for the legacy behavior to avoid the self-recursion. I am not sure 
that would help (and might it even make harder to follow the code).

WDYT?


> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3820 (patched)
> > 
> >
> > Shouldn't the first `Unreserve` operation contain the original `source`?

Of course.


> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3828 (patched)
> > 
> >
> > Is this the same as `reserve.resources().reservations()`?

Good point, this is indeed the same as `reserve.resources(0).reservations`. 
Using that seems to be a better idea than going strictly with symmetric 
approaches for `targetReservations` and `ancestorReservations` as it can help 
avoid a number of temporaries.


> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Lines 3834 (patched)
> > 
> >
> > That look more like debug-statements rather than `INFO`-level logging?

Indeed, even explicitly marked up as such with my magic string, yet still 
missed.


- Benjamin


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


On Nov. 7, 2019, noon, Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71729/
> ---
> 
> (Updated Nov. 7, 2019, noon)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9991
> https://issues.apache.org/jira/browse/MESOS-9991
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds authorization handling for `RESERVE` operations
> containing `source` fields. In order to stay backwards-compatible we add
> a dedicated authorization branch for such operations which under the
> hood translates each removed reservation to an `UNRESERVE` operation and
> every added reservation as a `RESERVE` operation where we fall back to
> existing authorization code for authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56 
>   src/tests/master_authorization_tests.cpp 
> 06471aa7779d399f4474ed40db3fbcc60b8298b2 
> 
> 
> Diff: https://reviews.apache.org/r/71729/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71729: Added authorization handling for reservations with `source`.

[Benjamin Bannier]

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

(Updated Nov. 7, 2019, noon)


Review request for mesos and Benno Evers.


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


Repository: mesos


Description
---

This patch adds authorization handling for `RESERVE` operations
containing `source` fields. In order to stay backwards-compatible we add
a dedicated authorization branch for such operations which under the
hood translates each removed reservation to an `UNRESERVE` operation and
every added reservation as a `RESERVE` operation where we fall back to
existing authorization code for authorization.


Diffs (updated)
-

  src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56 
  src/tests/master_authorization_tests.cpp 
06471aa7779d399f4474ed40db3fbcc60b8298b2 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier