Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On Sept. 26, 2020, 4:26 a.m., Andrei Sekretenko wrote: > > src/common/http.hpp > > Lines 422-426 (patched) > > <https://reviews.apache.org/r/72448/diff/3/?file=2236142#file2236142line423> > > > > Just `return approved(...)` ? I got totally confused here, I have no idea what you mean. Can you develop a patch and post here that I can make a reference ? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review221963 --- On July 23, 2020, 5:03 p.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated July 23, 2020, 5:03 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/3/ > > > Testing > --- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On July 18, 2020, 1:10 a.m., Andrei Sekretenko wrote: > > Thanks! Now this patch addresses the main issue. > > Dong Zhu wrote: > I updated the patch, this time I do not handle > `approvers->approved(resource)` in `void > Master::Subscribers::Subscriber::send()` specifically since I do not find any > `Error()` return from > https://github.com/apache/mesos/blob/master/src/authorizer/local/authorizer.cpp#L601-L797 > Could you please help reviewing it again ? Hi, Could you help taking a look at it ? It's been a long time since the last update. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review221252 --- On July 23, 2020, 5:03 p.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated July 23, 2020, 5:03 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/3/ > > > Testing > --- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72709: Fixed the tests warning messages.
> On July 28, 2020, 1:12 a.m., Benjamin Mahler wrote:
> > Hm.. it seems this was done to support running the tests from an
> > installation?
> >
> > https://github.com/apache/mesos/commit/6cca8c8071e4b863d951a1631141f861c47826d1
>
> Dong Zhu wrote:
> Yes. But this issue exists even from an installation without this fix
> unless specified option `--build_dir=/none` explicitly:
>
> ```
> $PREFIX/libexec/mesos/tests/mesos-tests/mesos-tests
> --gtest_filter="ContainerizerTest.ROOT_CGROUPS_BalloonFramework" --verbose
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 8: /root/mesos-development/mesos/build/src/colors.sh: No such file or
> directory
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 9: /root/mesos-development/mesos/build/src/atexit.sh: No such file or
> directory
> ```
>
> If user leave the `--source_dir=/nowhere` options as default or specify
> an valid path this patch works with the both circumstances.
> If use specify both `--source_dir=/nowhere` **(invalid path)** and
> `--build_dir=/none` explicitly, there will be following error show up:
>
> ```
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 6: /nowhere/support/colors.sh: No such file or directory
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 7: /nowhere/support/atexit.sh: No such file or directory
> ```
>
> I suppose it is confusing users therefore it is necessary to fix it and
> there are two methods here:
> 1. use the original commit I submitted
> 2. add checks for the source `${MESOS_SOURCE_DIR}/support/colors.sh` to
> prevent the `No such file or directory` showing up:
> ```
> FILE="${MESOS_SOURCE_DIR}/support/colors.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_SOURCE_DIR}/support/atexit.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_HELPER_DIR}/colors.sh" && test -f $FILE && source $FILE
>
> FILE="${MESOS_HELPER_DIR}/atexit.sh" && test -f $FILE && source $FILE
> ```
>
> What's your opinon here and which method will you prefer ?
>
> Dong Zhu wrote:
> Hi Ben, any comments here ?
>
> Benjamin Mahler wrote:
> Hm.. I suppose option 2 with an explanation of why we check both
> locations would be ok.
>
> Dong Zhu wrote:
> I have updated the patch, please review it again, thanks !
>
> Dong Zhu wrote:
> Any comments here ?
@bmahler, could you please take a look at it ?
- Dong
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72709/#review221383
---
On Aug. 4, 2020, 12:30 p.m., Dong Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72709/
> ---
>
> (Updated Aug. 4, 2020, 12:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Remove unnecessary codes which lead to the following warnning
> messages while performing tests:
>
> ...mesos/build/src/colors.sh: No such file or directory
> ...mesos/build/src/atexit.sh: No such file or directory
>
>
> Diffs
> -
>
> src/tests/balloon_framework_test.sh
> 7d0a4fee200577a5f7b2edbb3b310a07678879dd
> src/tests/disk_full_framework_test.sh
> 62e62cb90ffb5c05f0db1517409179d5b38518e8
> src/tests/dynamic_reservation_framework_test.sh
> c094ed1464c61b6acf647549f1fd83117b135358
> src/tests/no_executor_framework_test.sh
> df282cab6e91645d19d0aa09867e6b45adaf2508
> src/tests/persistent_volume_framework_test.sh
> 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0
> src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b
> src/tests/test_http_framework_test.sh
> 256655dfd7697071547f755e8941822b6d51f8a8
>
>
> Diff: https://reviews.apache.org/r/72709/diff/3/
>
>
> Testing
> ---
>
> make tests
>
>
> Thanks,
>
> Dong Zhu
>
>
Re: Review Request 72709: Fixed the tests warning messages.
> On July 28, 2020, 1:12 a.m., Benjamin Mahler wrote:
> > Hm.. it seems this was done to support running the tests from an
> > installation?
> >
> > https://github.com/apache/mesos/commit/6cca8c8071e4b863d951a1631141f861c47826d1
>
> Dong Zhu wrote:
> Yes. But this issue exists even from an installation without this fix
> unless specified option `--build_dir=/none` explicitly:
>
> ```
> $PREFIX/libexec/mesos/tests/mesos-tests/mesos-tests
> --gtest_filter="ContainerizerTest.ROOT_CGROUPS_BalloonFramework" --verbose
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 8: /root/mesos-development/mesos/build/src/colors.sh: No such file or
> directory
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 9: /root/mesos-development/mesos/build/src/atexit.sh: No such file or
> directory
> ```
>
> If user leave the `--source_dir=/nowhere` options as default or specify
> an valid path this patch works with the both circumstances.
> If use specify both `--source_dir=/nowhere` **(invalid path)** and
> `--build_dir=/none` explicitly, there will be following error show up:
>
> ```
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 6: /nowhere/support/colors.sh: No such file or directory
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 7: /nowhere/support/atexit.sh: No such file or directory
> ```
>
> I suppose it is confusing users therefore it is necessary to fix it and
> there are two methods here:
> 1. use the original commit I submitted
> 2. add checks for the source `${MESOS_SOURCE_DIR}/support/colors.sh` to
> prevent the `No such file or directory` showing up:
> ```
> FILE="${MESOS_SOURCE_DIR}/support/colors.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_SOURCE_DIR}/support/atexit.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_HELPER_DIR}/colors.sh" && test -f $FILE && source $FILE
>
> FILE="${MESOS_HELPER_DIR}/atexit.sh" && test -f $FILE && source $FILE
> ```
>
> What's your opinon here and which method will you prefer ?
>
> Dong Zhu wrote:
> Hi Ben, any comments here ?
>
> Benjamin Mahler wrote:
> Hm.. I suppose option 2 with an explanation of why we check both
> locations would be ok.
>
> Dong Zhu wrote:
> I have updated the patch, please review it again, thanks !
Any comments here ?
- Dong
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72709/#review221383
---
On Aug. 4, 2020, 12:30 p.m., Dong Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72709/
> ---
>
> (Updated Aug. 4, 2020, 12:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Remove unnecessary codes which lead to the following warnning
> messages while performing tests:
>
> ...mesos/build/src/colors.sh: No such file or directory
> ...mesos/build/src/atexit.sh: No such file or directory
>
>
> Diffs
> -
>
> src/tests/balloon_framework_test.sh
> 7d0a4fee200577a5f7b2edbb3b310a07678879dd
> src/tests/disk_full_framework_test.sh
> 62e62cb90ffb5c05f0db1517409179d5b38518e8
> src/tests/dynamic_reservation_framework_test.sh
> c094ed1464c61b6acf647549f1fd83117b135358
> src/tests/no_executor_framework_test.sh
> df282cab6e91645d19d0aa09867e6b45adaf2508
> src/tests/persistent_volume_framework_test.sh
> 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0
> src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b
> src/tests/test_http_framework_test.sh
> 256655dfd7697071547f755e8941822b6d51f8a8
>
>
> Diff: https://reviews.apache.org/r/72709/diff/3/
>
>
> Testing
> ---
>
> make tests
>
>
> Thanks,
>
> Dong Zhu
>
>
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On July 18, 2020, 1:10 a.m., Andrei Sekretenko wrote: > > Thanks! Now this patch addresses the main issue. I updated the patch, this time I do not handle `approvers->approved(resource)` in `void Master::Subscribers::Subscriber::send()` specifically since I do not find any `Error()` return from https://github.com/apache/mesos/blob/master/src/authorizer/local/authorizer.cpp#L601-L797 Could you please help reviewing it again ? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review221252 --- On July 23, 2020, 5:03 p.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated July 23, 2020, 5:03 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/3/ > > > Testing > --- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72709: Fixed the tests warning messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72709/ --- (Updated Aug. 4, 2020, 12:30 p.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Remove unnecessary codes which lead to the following warnning messages while performing tests: ...mesos/build/src/colors.sh: No such file or directory ...mesos/build/src/atexit.sh: No such file or directory Diffs (updated) - src/tests/balloon_framework_test.sh 7d0a4fee200577a5f7b2edbb3b310a07678879dd src/tests/disk_full_framework_test.sh 62e62cb90ffb5c05f0db1517409179d5b38518e8 src/tests/dynamic_reservation_framework_test.sh c094ed1464c61b6acf647549f1fd83117b135358 src/tests/no_executor_framework_test.sh df282cab6e91645d19d0aa09867e6b45adaf2508 src/tests/persistent_volume_framework_test.sh 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0 src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b src/tests/test_http_framework_test.sh 256655dfd7697071547f755e8941822b6d51f8a8 Diff: https://reviews.apache.org/r/72709/diff/3/ Changes: https://reviews.apache.org/r/72709/diff/2-3/ Testing --- make tests Thanks, Dong Zhu
Re: Review Request 72709: Fixed the tests warning messages.
> On July 28, 2020, 1:12 a.m., Benjamin Mahler wrote:
> > Hm.. it seems this was done to support running the tests from an
> > installation?
> >
> > https://github.com/apache/mesos/commit/6cca8c8071e4b863d951a1631141f861c47826d1
>
> Dong Zhu wrote:
> Yes. But this issue exists even from an installation without this fix
> unless specified option `--build_dir=/none` explicitly:
>
> ```
> $PREFIX/libexec/mesos/tests/mesos-tests/mesos-tests
> --gtest_filter="ContainerizerTest.ROOT_CGROUPS_BalloonFramework" --verbose
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 8: /root/mesos-development/mesos/build/src/colors.sh: No such file or
> directory
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 9: /root/mesos-development/mesos/build/src/atexit.sh: No such file or
> directory
> ```
>
> If user leave the `--source_dir=/nowhere` options as default or specify
> an valid path this patch works with the both circumstances.
> If use specify both `--source_dir=/nowhere` **(invalid path)** and
> `--build_dir=/none` explicitly, there will be following error show up:
>
> ```
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 6: /nowhere/support/colors.sh: No such file or directory
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 7: /nowhere/support/atexit.sh: No such file or directory
> ```
>
> I suppose it is confusing users therefore it is necessary to fix it and
> there are two methods here:
> 1. use the original commit I submitted
> 2. add checks for the source `${MESOS_SOURCE_DIR}/support/colors.sh` to
> prevent the `No such file or directory` showing up:
> ```
> FILE="${MESOS_SOURCE_DIR}/support/colors.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_SOURCE_DIR}/support/atexit.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_HELPER_DIR}/colors.sh" && test -f $FILE && source $FILE
>
> FILE="${MESOS_HELPER_DIR}/atexit.sh" && test -f $FILE && source $FILE
> ```
>
> What's your opinon here and which method will you prefer ?
>
> Dong Zhu wrote:
> Hi Ben, any comments here ?
>
> Benjamin Mahler wrote:
> Hm.. I suppose option 2 with an explanation of why we check both
> locations would be ok.
I have updated the patch, please review it again, thanks !
- Dong
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72709/#review221383
---
On Aug. 4, 2020, 12:24 p.m., Dong Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72709/
> ---
>
> (Updated Aug. 4, 2020, 12:24 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Remove unnecessary codes which lead to the following warnning
> messages while performing tests:
>
> ...mesos/build/src/colors.sh: No such file or directory
> ...mesos/build/src/atexit.sh: No such file or directory
>
>
> Diffs
> -
>
> src/tests/balloon_framework_test.sh
> 7d0a4fee200577a5f7b2edbb3b310a07678879dd
> src/tests/disk_full_framework_test.sh
> 62e62cb90ffb5c05f0db1517409179d5b38518e8
> src/tests/dynamic_reservation_framework_test.sh
> c094ed1464c61b6acf647549f1fd83117b135358
> src/tests/no_executor_framework_test.sh
> df282cab6e91645d19d0aa09867e6b45adaf2508
> src/tests/persistent_volume_framework_test.sh
> 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0
> src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b
> src/tests/test_http_framework_test.sh
> 256655dfd7697071547f755e8941822b6d51f8a8
>
>
> Diff: https://reviews.apache.org/r/72709/diff/2/
>
>
> Testing
> ---
>
> make tests
>
>
> Thanks,
>
> Dong Zhu
>
>
Re: Review Request 72709: Fixed the tests warning messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72709/ --- (Updated Aug. 4, 2020, 12:24 p.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Remove unnecessary codes which lead to the following warnning messages while performing tests: ...mesos/build/src/colors.sh: No such file or directory ...mesos/build/src/atexit.sh: No such file or directory Diffs (updated) - src/tests/balloon_framework_test.sh 7d0a4fee200577a5f7b2edbb3b310a07678879dd src/tests/disk_full_framework_test.sh 62e62cb90ffb5c05f0db1517409179d5b38518e8 src/tests/dynamic_reservation_framework_test.sh c094ed1464c61b6acf647549f1fd83117b135358 src/tests/no_executor_framework_test.sh df282cab6e91645d19d0aa09867e6b45adaf2508 src/tests/persistent_volume_framework_test.sh 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0 src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b src/tests/test_http_framework_test.sh 256655dfd7697071547f755e8941822b6d51f8a8 Diff: https://reviews.apache.org/r/72709/diff/2/ Changes: https://reviews.apache.org/r/72709/diff/1-2/ Testing --- make tests Thanks, Dong Zhu
Re: Review Request 72709: Fixed the tests warning messages.
> On July 28, 2020, 1:12 a.m., Benjamin Mahler wrote:
> > Hm.. it seems this was done to support running the tests from an
> > installation?
> >
> > https://github.com/apache/mesos/commit/6cca8c8071e4b863d951a1631141f861c47826d1
>
> Dong Zhu wrote:
> Yes. But this issue exists even from an installation without this fix
> unless specified option `--build_dir=/none` explicitly:
>
> ```
> $PREFIX/libexec/mesos/tests/mesos-tests/mesos-tests
> --gtest_filter="ContainerizerTest.ROOT_CGROUPS_BalloonFramework" --verbose
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 8: /root/mesos-development/mesos/build/src/colors.sh: No such file or
> directory
> /root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line
> 9: /root/mesos-development/mesos/build/src/atexit.sh: No such file or
> directory
> ```
>
> If user leave the `--source_dir=/nowhere` options as default or specify
> an valid path this patch works with the both circumstances.
> If use specify both `--source_dir=/nowhere` **(invalid path)** and
> `--build_dir=/none` explicitly, there will be following error show up:
>
> ```
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 6: /nowhere/support/colors.sh: No such file or directory
> /usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh:
> line 7: /nowhere/support/atexit.sh: No such file or directory
> ```
>
> I suppose it is confusing users therefore it is necessary to fix it and
> there are two methods here:
> 1. use the original commit I submitted
> 2. add checks for the source `${MESOS_SOURCE_DIR}/support/colors.sh` to
> prevent the `No such file or directory` showing up:
> ```
> FILE="${MESOS_SOURCE_DIR}/support/colors.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_SOURCE_DIR}/support/atexit.sh" && test -f $FILE &&
> source $FILE
> FILE="${MESOS_HELPER_DIR}/colors.sh" && test -f $FILE && source $FILE
>
> FILE="${MESOS_HELPER_DIR}/atexit.sh" && test -f $FILE && source $FILE
> ```
>
> What's your opinon here and which method will you prefer ?
Hi Ben, any comments here ?
- Dong
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72709/#review221383
---
On July 27, 2020, 5:34 p.m., Dong Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72709/
> ---
>
> (Updated July 27, 2020, 5:34 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Remove unnecessary codes which lead to the following warnning
> messages while performing tests:
>
> ...mesos/build/src/colors.sh: No such file or directory
> ...mesos/build/src/atexit.sh: No such file or directory
>
>
> Diffs
> -
>
> src/tests/balloon_framework_test.sh
> 7d0a4fee200577a5f7b2edbb3b310a07678879dd
> src/tests/disk_full_framework_test.sh
> 62e62cb90ffb5c05f0db1517409179d5b38518e8
> src/tests/dynamic_reservation_framework_test.sh
> c094ed1464c61b6acf647549f1fd83117b135358
> src/tests/no_executor_framework_test.sh
> df282cab6e91645d19d0aa09867e6b45adaf2508
> src/tests/persistent_volume_framework_test.sh
> 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0
> src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b
> src/tests/test_http_framework_test.sh
> 256655dfd7697071547f755e8941822b6d51f8a8
>
>
> Diff: https://reviews.apache.org/r/72709/diff/1/
>
>
> Testing
> ---
>
> make tests
>
>
> Thanks,
>
> Dong Zhu
>
>
Re: Review Request 72709: Fixed the tests warning messages.
> On July 28, 2020, 1:12 a.m., Benjamin Mahler wrote:
> > Hm.. it seems this was done to support running the tests from an
> > installation?
> >
> > https://github.com/apache/mesos/commit/6cca8c8071e4b863d951a1631141f861c47826d1
Yes. But this issue exists even from an installation without this fix unless
specified option `--build_dir=/none` explicitly:
```
$PREFIX/libexec/mesos/tests/mesos-tests/mesos-tests
--gtest_filter="ContainerizerTest.ROOT_CGROUPS_BalloonFramework" --verbose
/root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line 8:
/root/mesos-development/mesos/build/src/colors.sh: No such file or directory
/root/mesos-development/mesos/src/tests/balloon_framework_test.sh: line 9:
/root/mesos-development/mesos/build/src/atexit.sh: No such file or directory
```
If user leave the `--source_dir=/nowhere` options as default or specify an
valid path this patch works with the both circumstances.
If use specify both `--source_dir=/nowhere` **(invalid path)** and
`--build_dir=/none` explicitly, there will be following error show up:
```
/usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh: line 6:
/nowhere/support/colors.sh: No such file or directory
/usr/local/mesos-develop/libexec/mesos/tests/balloon_framework_test.sh: line 7:
/nowhere/support/atexit.sh: No such file or directory
```
I suppose it is confusing users therefore it is necessary to fix it and there
are two methods here:
1. use the original commit I submitted
2. add checks for the source `${MESOS_SOURCE_DIR}/support/colors.sh` to
prevent the `No such file or directory` showing up:
```
FILE="${MESOS_SOURCE_DIR}/support/colors.sh" && test -f $FILE && source
$FILE
FILE="${MESOS_SOURCE_DIR}/support/atexit.sh" && test -f $FILE && source
$FILE
FILE="${MESOS_HELPER_DIR}/colors.sh" && test -f $FILE && source $FILE
FILE="${MESOS_HELPER_DIR}/atexit.sh" && test -f $FILE && source $FILE
```
What's your opinon here and which method will you prefer ?
- Dong
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72709/#review221383
---
On July 27, 2020, 5:34 p.m., Dong Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72709/
> ---
>
> (Updated July 27, 2020, 5:34 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Remove unnecessary codes which lead to the following warnning
> messages while performing tests:
>
> ...mesos/build/src/colors.sh: No such file or directory
> ...mesos/build/src/atexit.sh: No such file or directory
>
>
> Diffs
> -
>
> src/tests/balloon_framework_test.sh
> 7d0a4fee200577a5f7b2edbb3b310a07678879dd
> src/tests/disk_full_framework_test.sh
> 62e62cb90ffb5c05f0db1517409179d5b38518e8
> src/tests/dynamic_reservation_framework_test.sh
> c094ed1464c61b6acf647549f1fd83117b135358
> src/tests/no_executor_framework_test.sh
> df282cab6e91645d19d0aa09867e6b45adaf2508
> src/tests/persistent_volume_framework_test.sh
> 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0
> src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b
> src/tests/test_http_framework_test.sh
> 256655dfd7697071547f755e8941822b6d51f8a8
>
>
> Diff: https://reviews.apache.org/r/72709/diff/1/
>
>
> Testing
> ---
>
> make tests
>
>
> Thanks,
>
> Dong Zhu
>
>
Review Request 72709: Fixed the tests warning messages.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72709/ --- Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Remove unnecessary codes which lead to the following warnning messages while performing tests: ...mesos/build/src/colors.sh: No such file or directory ...mesos/build/src/atexit.sh: No such file or directory Diffs - src/tests/balloon_framework_test.sh 7d0a4fee200577a5f7b2edbb3b310a07678879dd src/tests/disk_full_framework_test.sh 62e62cb90ffb5c05f0db1517409179d5b38518e8 src/tests/dynamic_reservation_framework_test.sh c094ed1464c61b6acf647549f1fd83117b135358 src/tests/no_executor_framework_test.sh df282cab6e91645d19d0aa09867e6b45adaf2508 src/tests/persistent_volume_framework_test.sh 6488656c4bd8ad0c6f25a367cf7de2f926dbecf0 src/tests/test_framework_test.sh 8782e01edc5be3833760fc45e2b545fce100547b src/tests/test_http_framework_test.sh 256655dfd7697071547f755e8941822b6d51f8a8 Diff: https://reviews.apache.org/r/72709/diff/1/ Testing --- make tests Thanks, Dong Zhu
Re: Review Request 72600: Remove credential text format support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72600/ --- (Updated July 24, 2020, 9:46 a.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Since text format for credential is no longer supported, remove it. Diffs (updated) - CHANGELOG 11c5c9983b9c77b5ca4d3ff11232b4d4a66e3615 src/credentials/credentials.hpp 6e9f4aabfa810a4569acdcbeb9491f928e29bb9d Diff: https://reviews.apache.org/r/72600/diff/4/ Changes: https://reviews.apache.org/r/72600/diff/3-4/ Testing --- Manually tested make check Thanks, Dong Zhu
Re: Review Request 72618: Deprecated tests plain text credential format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72618/ --- (Updated July 24, 2020, 9:34 a.m.) Review request for mesos. Repository: mesos Description --- Deprecated tests plain text credential format. Testing --- Thanks, Dong Zhu
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/ --- (Updated July 23, 2020, 9:03 a.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- This patch intends to fix issue MESOS-10085. When the authorization failed happens master return nothing to the subscriber, subscriber isn't aware of what is happening, this issue can lead to inconsistencies in Event stream. Diffs (updated) - include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c src/tests/master/mock_master_api_subscriber.cpp 893d3e366164ccebd2847ed4c2874ab00e0e5b7b Diff: https://reviews.apache.org/r/72448/diff/3/ Changes: https://reviews.apache.org/r/72448/diff/2-3/ Testing --- - Manually tested - make check Thanks, Dong Zhu
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote: > > src/common/http.hpp > > Lines 437 (patched) > > <https://reviews.apache.org/r/72448/diff/2/?file=2235829#file2235829line437> > > > > Is it necessary to use an intermediate `ostringstream` instead of > > directly writing into the glog's stream returned by `LOG(WARNING)`? > > Dong Zhu wrote: > I need those logs to inform subscriber about the exact fail reason. Is it necessary to notify user regarding the fail reason? If so what sort of message would you prefer ? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review221253 ------- On July 14, 2020, 9:43 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated July 14, 2020, 9:43 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/2/ > > > Testing > --- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote: > > src/common/http.hpp > > Lines 436-441 (patched) > > <https://reviews.apache.org/r/72448/diff/2/?file=2235829#file2235829line436> > > > > Thanks for taking care about logging! > > > > We definitely don't want to write a log line per every authorization > > error, as the errors might (and likely will) come in large bunches. > > > > This should happen at most once per disconnecting the subscriber (one > > more reason to move this into `Subscribers::send()`). > > > > Also, it probably would be beneficial to log some other details > > allowing the persons reading the log to identify the subscriber (at the > > very least, IP + principal, and probably something else - honestly, I > > don't remember what else Mesos master knows about a subscriber). So you mean there is not need to print those errors to logs anymore ? > On July 17, 2020, 5:40 p.m., Andrei Sekretenko wrote: > > src/common/http.hpp > > Lines 437 (patched) > > <https://reviews.apache.org/r/72448/diff/2/?file=2235829#file2235829line437> > > > > Is it necessary to use an intermediate `ostringstream` instead of > > directly writing into the glog's stream returned by `LOG(WARNING)`? I need those logs to inform subscriber about the exact fail reason. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review221253 --- On July 14, 2020, 9:43 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated July 14, 2020, 9:43 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/2/ > > > Testing > --- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On July 17, 2020, 5:10 p.m., Andrei Sekretenko wrote:
> > src/common/http.hpp
> > Lines 428 (patched)
> > <https://reviews.apache.org/r/72448/diff/2/?file=2235829#file2235829line428>
> >
> > It would be highly beneficial to keep the error sending logic separate
> > from `ObjectApprovers` and move it closer to `Subscribers`.
> >
> > `ObjectApprovers` by design are oblivious of the exact purpose for
> > which they are called, and it is preferable to keep them decoupled from
> > implementation details of the external APIs.
> > Also, we have a related API issue
> > https://issues.apache.org/jira/browse/MESOS-10099 which will requre
> > handling authorization errors in a totally different way.
> >
> > If you want to avoid changing return type of the of the existing
> > `approved(..)` method, I would suggest adding a general-purpose
> > `ObjectApprovers` method that would return `Try`(something like
> > `template<..> Try ObjectApprovers::tryApprove<>(..)`)
> > **and to make the old method `bool ObjectApprovers::approved<..>(..)`
> > into a thin wrapper around this method**, so that we don't need to care
> > about the duplicated logic (especially the one in
> > `approved(..)`) in the future.
> >
> >
> > To use the returned value of this method in `Subscriber::send()` you
> > will probably want to wrap the calls to it into some callable that will
> > return `bool` and write down the error.
> > Something like
> > ```
> > Option error;
> > auto splitError = [](Try&& approved) {
> > if (approved.isError()) {
> > error = std::move(approved.error());
> > return false;
> > }
> > return *approved;
> > }
> > // Code that checks authorizations and composes the message to be sent
> > ...
> > if (splitError(approvers->approved(
> > event.framework_added().framework().framework_info({
> > ...
> > }
> > ...
> > // At the end of send(), after all the authorizations have been checked
> > if (error.isSome()) {
> > // Send error event
> > ...
> > // Close connection
> > ...
> > return;
> > }
> >
> > // All is OK, send the event
> > ...
> >
> > ```
> >
> > Note that my choice of names `tryApproved` and `splitError` is
> > questionable, would be great if you come up with something better.
I plan to add another method as below and extract the error in
`Subscriber::send()` as you introduced, eg
`splitError(approvers->tryApproved(event.task_added().task(),
*frameworkInfo))`:
```
template
Try tryApproved(const Args&... args) const
{
const Try approval =
approved(action, ObjectApprover::Object(args...));
if (approval.isError()) {
LOG(WARNING) << "Failed to authorize principal "
<< " '" << (principal.isSome() ? stringify(*principal) : "")
<< "' for action " << authorization::Action_Name(action)
<< ": " << approval.error();
}
return approval;
}
```
For `approvers->approved(resource)` I think it is a specific case, I
have to add another method as below:
```
template <>
inline Try ObjectApprovers::tryApproved(
const Resource& resource) const
{
Try result = true;
// Necessary because recovered agents are presented in old format.
if (resource.has_role() && resource.role() != "*") {
result = tryApproved(resource.role());
if (result.isError() || !result.get())
return result;
}
// Reservations follow a path model where each entry is a child of the
// previous one. Therefore, to accept the resource the acceptor has to
// accept all entries.
foreach (Resource::ReservationInfo reservation, resource.reservations()) {
result = tryApproved(reservation.role());
if (result.isError() || !result.get())
return result;
}
if (resource.has_allocation_info()) {
result =
tryApproved(resource.allocation_info().role());
if (result.isError() || !result.get())
return result;
}
return result;
}
```
I do not place original method `bool approved(const Args&... args) const` into
`Try tryApproved(const Args&... args) const` since I need to get the
error message.
Please let me know whether it make sense.
- Dong
---
This is an automatically generated e-m
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote: > > src/master/master.cpp > > Lines 12264 (patched) > > <https://reviews.apache.org/r/72448/diff/1/?file=2229246#file2229246line12264> > > > > As implemented now, `false` returned by `ObjectApprovers::approved()` > > is not necessarily an error case. This might just be a declined > > authorization, i.e. the subscriber is not even allowed to see the object > > in question due to the missing permissions. Events for such objects should > > just be not sent to the subscriber. > > > > To distinguish between **declined authorization** and **authorization > > error** here, you will have to change/extend the interface provided by > > `ObjectApprovers` (plural) so that it doesn't hide `Error`s returned by > > `approved()` method of `ObjectApprover`(singular). > > Dong Zhu wrote: > Why will we need to distinguish those two circumstances ? > `ObjectApprovers::approved()` returned with `false` when **declined > authorization** or **authorization error** happens. The purpose of this patch > just handling the `false` part by sending a notification to the subscriber. > > Andrei Sekretenko wrote: > The `false` part (i.e. declined authorization) is not an issue. > Subscriber does not need to be informed about the fact that somewhere on > the cluster there are some objects (tasks/frameworks/whatever) that the > subscriber is not allowed to access. > It is frequently the case that there are, and there is nothing wrong with > it. > > Declined authorization is pretty much expected to occur, and existing > subscribers (such as DC/OS UI) are designed not to expect any information > about the objects their principal is not authorized to access. > > Probably https://issues.apache.org/jira/browse/MESOS-7785 points to more > detailed explanations why this decision was made. > > > On the other hand, subscribers expect to see all events for all objects > they are authorized to see. > Here lies the issue described in MESOS-10085: in case of an authorization > failure, the event will be silently dropped, but the subscriber will have no > knowledge of that and will continue working as if there was no event. > > Dong Zhu wrote: > In summary: > - for declined authorization circumstance: > There is no need to inform the subscriber just drop the message. eg, > for `FRAMEWORK_ADDED` event, if there is no proper permission like > `VIEW_FRAMEWORK` configured just drop the message. > - for authorization error circumstance: > Send an event to subscriber to inform the error. It fixed the issue > intorducted in MESOS-10085 **"in case of an authorization failure, the event > will be silently dropped, but the subscriber will have no knowledge of that > and will continue working as if there was no event."** > > Am I correct ? > > Andrei Sekretenko wrote: > Yes, exactly. > > Note that to accomplish this, you will have to change the return type of > `ObjectApprovers::approve()`, as it currently does not allow the caller to > distinguish between a declined authorization and an error. > > Dong Zhu wrote: > I came up with two ways for solving this issue: > > 1. Change/Extend the interface provided by > `ObjectApprovers::approved()`. eg, change the return type from `bool` to > `Try` > https://github.com/apache/mesos/blob/master/src/common/http.hpp#L395-L418 > then distinguish the **declined authorization** and **authorization error** > in the caller side `void Master::Subscribers::Subscriber::send()`. Since > there are many callers in mesos it will result in tremendous code changes and > introduce code duplication. > 2. Add a new element to `class ObjectApprovers` eg, > `StreamingHttpConnection http;`. Consturct the return > message in `ObjectApprovers::approved()` and send to the subscriber if > **authorization error** occurs. I am not sure whether this way broke any > mesos developing principals though. > > What's your option here ? or do you have any other good way ? Thanks ! Hi Andrei, I have updated the patch, could you please help reviewing it again ? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review220548 --- On July 14, 2020, 9:43 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https:/
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/ --- (Updated July 14, 2020, 9:43 a.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- This patch intends to fix issue MESOS-10085. When the authorization failed happens master return nothing to the subscriber, subscriber isn't aware of what is happening, this issue can lead to inconsistencies in Event stream. Diffs (updated) - include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c src/tests/master/mock_master_api_subscriber.cpp 893d3e366164ccebd2847ed4c2874ab00e0e5b7b Diff: https://reviews.apache.org/r/72448/diff/2/ Changes: https://reviews.apache.org/r/72448/diff/1-2/ Testing --- - Manually tested - make check Thanks, Dong Zhu
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote: > > src/master/master.cpp > > Lines 12264 (patched) > > <https://reviews.apache.org/r/72448/diff/1/?file=2229246#file2229246line12264> > > > > As implemented now, `false` returned by `ObjectApprovers::approved()` > > is not necessarily an error case. This might just be a declined > > authorization, i.e. the subscriber is not even allowed to see the object > > in question due to the missing permissions. Events for such objects should > > just be not sent to the subscriber. > > > > To distinguish between **declined authorization** and **authorization > > error** here, you will have to change/extend the interface provided by > > `ObjectApprovers` (plural) so that it doesn't hide `Error`s returned by > > `approved()` method of `ObjectApprover`(singular). > > Dong Zhu wrote: > Why will we need to distinguish those two circumstances ? > `ObjectApprovers::approved()` returned with `false` when **declined > authorization** or **authorization error** happens. The purpose of this patch > just handling the `false` part by sending a notification to the subscriber. > > Andrei Sekretenko wrote: > The `false` part (i.e. declined authorization) is not an issue. > Subscriber does not need to be informed about the fact that somewhere on > the cluster there are some objects (tasks/frameworks/whatever) that the > subscriber is not allowed to access. > It is frequently the case that there are, and there is nothing wrong with > it. > > Declined authorization is pretty much expected to occur, and existing > subscribers (such as DC/OS UI) are designed not to expect any information > about the objects their principal is not authorized to access. > > Probably https://issues.apache.org/jira/browse/MESOS-7785 points to more > detailed explanations why this decision was made. > > > On the other hand, subscribers expect to see all events for all objects > they are authorized to see. > Here lies the issue described in MESOS-10085: in case of an authorization > failure, the event will be silently dropped, but the subscriber will have no > knowledge of that and will continue working as if there was no event. > > Dong Zhu wrote: > In summary: > - for declined authorization circumstance: > There is no need to inform the subscriber just drop the message. eg, > for `FRAMEWORK_ADDED` event, if there is no proper permission like > `VIEW_FRAMEWORK` configured just drop the message. > - for authorization error circumstance: > Send an event to subscriber to inform the error. It fixed the issue > intorducted in MESOS-10085 **"in case of an authorization failure, the event > will be silently dropped, but the subscriber will have no knowledge of that > and will continue working as if there was no event."** > > Am I correct ? > > Andrei Sekretenko wrote: > Yes, exactly. > > Note that to accomplish this, you will have to change the return type of > `ObjectApprovers::approve()`, as it currently does not allow the caller to > distinguish between a declined authorization and an error. I came up with two ways for solving this issue: 1. Change/Extend the interface provided by `ObjectApprovers::approved()`. eg, change the return type from `bool` to `Try` https://github.com/apache/mesos/blob/master/src/common/http.hpp#L395-L418 then distinguish the **declined authorization** and **authorization error** in the caller side `void Master::Subscribers::Subscriber::send()`. Since there are many callers in mesos it will result in tremendous code changes and introduce code duplication. 2. Add a new element to `class ObjectApprovers` eg, `StreamingHttpConnection http;`. Consturct the return message in `ObjectApprovers::approved()` and send to the subscriber if **authorization error** occurs. I am not sure whether this way broke any mesos developing principals though. What's your option here ? or do you have any other good way ? Thanks ! - Dong ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review220548 --- On April 29, 2020, 6:18 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated April 29, 2020, 6:18 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. >
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote: > > src/master/master.cpp > > Lines 12264 (patched) > > <https://reviews.apache.org/r/72448/diff/1/?file=2229246#file2229246line12264> > > > > As implemented now, `false` returned by `ObjectApprovers::approved()` > > is not necessarily an error case. This might just be a declined > > authorization, i.e. the subscriber is not even allowed to see the object > > in question due to the missing permissions. Events for such objects should > > just be not sent to the subscriber. > > > > To distinguish between **declined authorization** and **authorization > > error** here, you will have to change/extend the interface provided by > > `ObjectApprovers` (plural) so that it doesn't hide `Error`s returned by > > `approved()` method of `ObjectApprover`(singular). > > Dong Zhu wrote: > Why will we need to distinguish those two circumstances ? > `ObjectApprovers::approved()` returned with `false` when **declined > authorization** or **authorization error** happens. The purpose of this patch > just handling the `false` part by sending a notification to the subscriber. > > Andrei Sekretenko wrote: > The `false` part (i.e. declined authorization) is not an issue. > Subscriber does not need to be informed about the fact that somewhere on > the cluster there are some objects (tasks/frameworks/whatever) that the > subscriber is not allowed to access. > It is frequently the case that there are, and there is nothing wrong with > it. > > Declined authorization is pretty much expected to occur, and existing > subscribers (such as DC/OS UI) are designed not to expect any information > about the objects their principal is not authorized to access. > > Probably https://issues.apache.org/jira/browse/MESOS-7785 points to more > detailed explanations why this decision was made. > > > On the other hand, subscribers expect to see all events for all objects > they are authorized to see. > Here lies the issue described in MESOS-10085: in case of an authorization > failure, the event will be silently dropped, but the subscriber will have no > knowledge of that and will continue working as if there was no event. In summary: - for declined authorization circumstance: There is no need to inform the subscriber just drop the message. eg, for `FRAMEWORK_ADDED` event, if there is no proper permission like `VIEW_FRAMEWORK` configured just drop the message. - for authorization error circumstance: Send an event to subscriber to inform the error. It fixed the issue intorducted in MESOS-10085 **"in case of an authorization failure, the event will be silently dropped, but the subscriber will have no knowledge of that and will continue working as if there was no event."** Am I correct ? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review220548 --- On April 29, 2020, 6:18 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated April 29, 2020, 6:18 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/1/ > > > Testing > --- > > - Manually tested > - make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72620: Deprecated tests plain text credential format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72620/ --- (Updated June 27, 2020, 11:32 a.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Deprecated tests plain text credential format. Diffs - src/tests/credentials_tests.cpp 8fa26dfc564a417cf09ddb904f0d34acd448b811 src/tests/script.cpp 038519c92787824098ab503e0ac79ec311e057b0 Diff: https://reviews.apache.org/r/72620/diff/1/ Testing (updated) --- Manually tested Thanks, Dong Zhu
Review Request 72620: Deprecated tests plain text credential format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72620/ --- Review request for mesos. Repository: mesos Description --- Deprecated tests plain text credential format. Diffs - src/tests/credentials_tests.cpp 8fa26dfc564a417cf09ddb904f0d34acd448b811 src/tests/script.cpp 038519c92787824098ab503e0ac79ec311e057b0 Diff: https://reviews.apache.org/r/72620/diff/1/ Testing --- Thanks, Dong Zhu
Re: Review Request 72600: Remove credential text format support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72600/ --- (Updated June 26, 2020, 5:47 a.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Since text format for credential is no longer supported, remove it. Diffs (updated) - CHANGELOG 11c5c9983b9c77b5ca4d3ff11232b4d4a66e3615 src/credentials/credentials.hpp 6e9f4aabfa810a4569acdcbeb9491f928e29bb9d Diff: https://reviews.apache.org/r/72600/diff/3/ Changes: https://reviews.apache.org/r/72600/diff/2-3/ Testing --- Manually tested make check Thanks, Dong Zhu
Review Request 72618: Deprecated tests plain text credential format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72618/ --- Review request for mesos. Repository: mesos Description --- Deprecated tests plain text credential format. Testing --- Thanks, Dong Zhu
Re: Review Request 72600: Remove credential text format support.
> On June 19, 2020, 6:28 p.m., Benjamin Mahler wrote: > > Can you mark down this breaking change in the changelog? > > > > I think we'll need to send an email to users about this change, since it > > may break some users. You mean this file https://github.com/apache/mesos/blob/master/CHANGELOG ? And which section should it be added to ? > On June 19, 2020, 6:28 p.m., Benjamin Mahler wrote: > > src/credentials/credentials.hpp > > Line 67 (original), 65 (patched) > > <https://reviews.apache.org/r/72600/diff/1/?file=2234791#file2234791line67> > > > > Can you re-work the logic here so that errors are handled first and the > > error messages from the Trys are included? Hi Ben, Sure. I have updated the patch per your suggestion. - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72600/#review221037 ------- On June 23, 2020, 3:40 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72600/ > --- > > (Updated June 23, 2020, 3:40 a.m.) > > > Review request for mesos, Adam Berry, Benjamin Bannier, and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Since text format for credential is no longer supported, remove it. > > > Diffs > - > > src/credentials/credentials.hpp 6e9f4aabfa810a4569acdcbeb9491f928e29bb9d > > > Diff: https://reviews.apache.org/r/72600/diff/2/ > > > Testing > --- > > Manually tested > make check > > > Thanks, > > Dong Zhu > >
Re: Review Request 72600: Remove credential text format support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72600/ --- (Updated June 23, 2020, 3:40 a.m.) Review request for mesos, Adam Berry, Benjamin Bannier, and Benjamin Mahler. Repository: mesos Description --- Since text format for credential is no longer supported, remove it. Diffs (updated) - src/credentials/credentials.hpp 6e9f4aabfa810a4569acdcbeb9491f928e29bb9d Diff: https://reviews.apache.org/r/72600/diff/2/ Changes: https://reviews.apache.org/r/72600/diff/1-2/ Testing --- Manually tested make check Thanks, Dong Zhu
Review Request 72600: Remove credential text format support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72600/ --- Review request for mesos, Benjamin Bannier and Benjamin Mahler. Repository: mesos Description --- Since text format for credential is no longer supported, remove it. Diffs - src/credentials/credentials.hpp 6e9f4aabfa810a4569acdcbeb9491f928e29bb9d Diff: https://reviews.apache.org/r/72600/diff/1/ Testing --- Manually tested make check Thanks, Dong Zhu
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
> On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote:
> > Thanks for handling this bug!
> >
> > At this point, the main issue with this patch is that it doesn't discern
> > between declined authorization and authorization failure (see below).
> > To address this, you'll likely have to extend/change the interface provided
> > by ObjectApprover**s** (plural) so that they don't disguise authorization
> > errors as declined authorization.
> >
> > Also, please link MESOS-10085 to this review ("Bugs:" on the right or
> > `support/post-reviews.py --bugs-closed MESOS-10085`) and don't forget the
> > "testing done" section.
>
> Dong Zhu wrote:
> Thanks for your info, I replied them one by one as below.
@Andrei Sekretenko Could you take a look at the following comments ?
- Dong
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72448/#review220548
---
On April 29, 2020, 6:18 a.m., Dong Zhu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72448/
> ---
>
> (Updated April 29, 2020, 6:18 a.m.)
>
>
> Review request for mesos, Andrei Sekretenko and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch intends to fix issue MESOS-10085.
>
> When the authorization failed happens master return nothing to the
> subscriber, subscriber isn't aware of what is happening, this issue
> can lead to inconsistencies in Event stream.
>
>
> Diffs
> -
>
> include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29
> include/mesos/v1/master/master.proto
> 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d
> src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c
> src/tests/master/mock_master_api_subscriber.cpp
> 893d3e366164ccebd2847ed4c2874ab00e0e5b7b
>
>
> Diff: https://reviews.apache.org/r/72448/diff/1/
>
>
> Testing
> ---
>
> - Manually tested
> - make check
>
>
> Thanks,
>
> Dong Zhu
>
>
Re: Review Request 72448: Fixed operator api event inconsistencies issue.
is form of message is better and this is one thing that I want to confirm with. I will make change to the patch. > On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote: > > src/master/master.cpp > > Lines 12268 (patched) > > <https://reviews.apache.org/r/72448/diff/1/?file=2229246#file2229246line12268> > > > > Given that now we are sending `Error` event, the subscribers that are > > aware of this event can do whatever they want (proceed as if nothing > > happened; close connection and subscribe again, etc.) > > > > Are we aiming to fix inconsistency created by dropped events for > > existing subscribers, that do not handle `Error` event? If yes, then one > > (and probably the only) option for fixing this for existing subscribers is > > to close connection and drop the subscriber after sending `Error`. > > > > Probably, at this point we could just always close after Error. In a > > distant future, when the subsrcibers become aware of `Error`, we will > > probably want to add a flag to `SUBSCRIBE` so that, if the flag is set, > > disconnection doesn't happen, and the subscriber resubscribes on its own if > > it wants to. OK, close this http connection after sending the `Error` Event. > On April 29, 2020, 2:58 p.m., Andrei Sekretenko wrote: > > src/tests/master/mock_master_api_subscriber.cpp > > Lines 242 (patched) > > <https://reviews.apache.org/r/72448/diff/1/?file=2229247#file2229247line242> > > > > Given that we will need to add a test for this fix, doesn't it make > > sense to add one more callback, for `ERROR`? > > This way, tests will be able to use `EXPECT_CALL` when they need to > > check that the error was sent. Can I send subsequent patch for adding test ? - Dong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/#review220548 --- On April 29, 2020, 6:18 a.m., Dong Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72448/ > --- > > (Updated April 29, 2020, 6:18 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > This patch intends to fix issue MESOS-10085. > > When the authorization failed happens master return nothing to the > subscriber, subscriber isn't aware of what is happening, this issue > can lead to inconsistencies in Event stream. > > > Diffs > - > > include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 > include/mesos/v1/master/master.proto > 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d > src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c > src/tests/master/mock_master_api_subscriber.cpp > 893d3e366164ccebd2847ed4c2874ab00e0e5b7b > > > Diff: https://reviews.apache.org/r/72448/diff/1/ > > > Testing > --- > > > Thanks, > > Dong Zhu > >
Review Request 72448: Fixed operator api event inconsistencies issue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72448/ --- Review request for mesos. Repository: mesos Description --- This patch intends to fix issue MESOS-10085. When the authorization failed happens master return nothing to the subscriber, subscriber isn't aware of what is happening, this issue can lead to inconsistencies in Event stream. Diffs - include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c src/tests/master/mock_master_api_subscriber.cpp 893d3e366164ccebd2847ed4c2874ab00e0e5b7b Diff: https://reviews.apache.org/r/72448/diff/1/ Testing --- Thanks, Dong Zhu
Re: Review Request 72445: Fixed operator api event inconsistencies issue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72445/ --- (Updated April 28, 2020, 9:13 p.m.) Review request for mesos and Andrei Sekretenko. Bugs: MESOS-10085 https://issues.apache.org/jira/browse/MESOS-10085 Repository: mesos Description --- This patch intends to fix issue MESOS-10085. When the authorization failed happens master return nothing to the subscriber, subscriber isn't aware of what is happening, this issue can lead to inconsistencies in Event stream. Signed-off-by: Dong Zhu Diffs - include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c Diff: https://reviews.apache.org/r/72445/diff/1/ Testing --- Thanks, Dong Zhu
Review Request 72445: Fixed operator api event inconsistencies issue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72445/ --- Review request for mesos and Andrei Sekretenko. Repository: mesos Description --- This patch intends to fix issue MESOS-10085. When the authorization failed happens master return nothing to the subscriber, subscriber isn't aware of what is happening, this issue can lead to inconsistencies in Event stream. Signed-off-by: Dong Zhu Diffs - include/mesos/master/master.proto 021dadcea026da41347b3aaee5ddd12f4f14fa29 include/mesos/v1/master/master.proto 488fe294e8bfe8e0c6fc23c88f06c0d41169b96d src/master/master.cpp a8cca622ff0bd172300b9a2717b4860ed06b620c Diff: https://reviews.apache.org/r/72445/diff/1/ Testing --- Thanks, Dong Zhu
Re: Review Request 72275: Added Dong Zhu to contributors list.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72275/ --- (Updated March 27, 2020, 1:33 a.m.) Review request for mesos and Benjamin Mahler. Repository: mesos Description (updated) --- Added Dong Zhu to contributors list. Diffs - docs/contributors.yaml 038013bb5bdfbbda109a7db630bcd068c2268aaa Diff: https://reviews.apache.org/r/72275/diff/1/ Testing --- Thanks, Dong Zhu
Review Request 72275: Added Dong Zhu to contributors list.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72275/ --- Review request for mesos and Benjamin Mahler. Repository: mesos Description --- Signed-off-by: Dong Zhu Diffs - docs/contributors.yaml 038013bb5bdfbbda109a7db630bcd068c2268aaa Diff: https://reviews.apache.org/r/72275/diff/1/ Testing --- Thanks, Dong Zhu
