You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/09/19 07:42:14 UTC
Fwd: mesos git commit: Fixed a bug in getRootContainerId due to
protobuf copying issue.
Hi Jie,
Do you have more details on what exactly the problem is here? If
protobuf is unable to copy/merge nested messages in general, that
seems like something that might crop up elsewhere.
Perhaps we can (a) file a JIRA (ideally with a self-contained
test-case), and/or (c) report the problem to upstream?
Neil
---------- Forwarded message ----------
From: <ji...@apache.org>
Date: Sat, Sep 17, 2016 at 11:27 PM
Subject: mesos git commit: Fixed a bug in getRootContainerId due to
protobuf copying issue.
To: commits@mesos.apache.org
Repository: mesos
Updated Branches:
refs/heads/master a4fd86bce -> be81a924a
Fixed a bug in getRootContainerId due to protobuf copying issue.
It looks like protobuf is not so great dealing with nesting messages
when doing merge or copy. This patch uses an extra copy to bypass that
issue in the protobuf.
Review: https://reviews.apache.org/r/51992
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/be81a924
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/be81a924
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/be81a924
Branch: refs/heads/master
Commit: be81a924a9e9414ec98f8c9a87a5391dad865146
Parents: a4fd86b
Author: Jie Yu <yu...@gmail.com>
Authored: Sat Sep 17 14:22:31 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Sat Sep 17 14:25:33 2016 -0700
----------------------------------------------------------------------
src/slave/containerizer/mesos/utils.hpp | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/be81a924/src/slave/containerizer/mesos/utils.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/utils.hpp
b/src/slave/containerizer/mesos/utils.hpp
index 2bb55c1..178ebf3 100644
--- a/src/slave/containerizer/mesos/utils.hpp
+++ b/src/slave/containerizer/mesos/utils.hpp
@@ -27,7 +27,12 @@ static ContainerID getRootContainerId(const
ContainerID& containerId)
{
ContainerID rootContainerId = containerId;
while (rootContainerId.has_parent()) {
- rootContainerId = rootContainerId.parent();
+ // NOTE: Looks like protobuf does not handle copying well when
+ // nesting message is involved, because the source and the target
+ // point to the same object. Therefore, we create a temporary
+ // variable and use an extra copy here.
+ ContainerID id = rootContainerId.parent();
+ rootContainerId = id;
}
return rootContainerId;
Re: mesos git commit: Fixed a bug in getRootContainerId due to
protobuf copying issue.
Posted by Kevin Klues <kl...@gmail.com>.
Here is another problematic use case that I ran into a few weeks back:
Running the following ends up enteringa recursive loop and blowing up the
stack:
``` container.id.mutable_parent()->CopyFrom(container.id);
```
If you look at the body of `MergeFrom()`, which is called from
`CopyFrom()`, you can see why:
```
void ContainerID::MergeFrom(const ContainerID& from) {
GOOGLE_CHECK_NE(&from, this);
if (from._has_bits_[0 / 32] & (0xffu << (0 % 32))) {
if (from.has_value()) {
set_value(from.value());
}
if (from.has_parent()) {
mutable_parent()->::mesos::ContainerID::MergeFrom(from.parent());
}
}
mutable_unknown_fields()->MergeFrom(from.unknown_fields());
}
```
When we call `CopyFrom()` we pass it the same object who’s parent we are
trying to modify. However, once we make the original `mutable_parent()`
call, a parent has been created (albeit an empty one) on the original
object. This allows the `from.has_parent()` call in `MergeFrom()` to
succeed. From there we enter a recursive loop in calls to
`MergeFrom()`since we always operate on the same object.
On Mon, Sep 19, 2016 at 1:33 AM haosdent <ha...@gmail.com> wrote:
> From mesos.pb.h/mesos.pb.cc
>
> ```
> inline ContainerID& operator=(const ContainerID& from) {
> CopyFrom(from);
> return *this;
> }
>
> void ContainerID::CopyFrom(const ::google::protobuf::Message& from) {
> if (&from == this) return;
> Clear();
> MergeFrom(from);
> }
> ```
>
> On Mon, Sep 19, 2016 at 4:32 PM, haosdent <ha...@gmail.com> wrote:
>
> > @Neil, when
> >
> > ```
> > rootContainerId = rootContainerId.parent();
> > ```
> >
> > protobuf would try to call `ContaienrID::Clear()` first and then perform
> > `ContainerID::CopyFrom`. Because the parent has been broken after
> > `ContainerID::Clear()`, so the `ContainerID::CopyFrom` would get an empty
> > value. I think this is not a bug of protobuf and we should avoid using
> >
> > ```
> > Message = Message.xx();
> > ```
> >
> > On Mon, Sep 19, 2016 at 3:42 PM, Neil Conway <ne...@gmail.com>
> > wrote:
> >
> >> Hi Jie,
> >>
> >> Do you have more details on what exactly the problem is here? If
> >> protobuf is unable to copy/merge nested messages in general, that
> >> seems like something that might crop up elsewhere.
> >>
> >> Perhaps we can (a) file a JIRA (ideally with a self-contained
> >> test-case), and/or (c) report the problem to upstream?
> >>
> >> Neil
> >>
> >> ---------- Forwarded message ----------
> >> From: <ji...@apache.org>
> >> Date: Sat, Sep 17, 2016 at 11:27 PM
> >> Subject: mesos git commit: Fixed a bug in getRootContainerId due to
> >> protobuf copying issue.
> >> To: commits@mesos.apache.org
> >>
> >>
> >> Repository: mesos
> >> Updated Branches:
> >> refs/heads/master a4fd86bce -> be81a924a
> >>
> >>
> >> Fixed a bug in getRootContainerId due to protobuf copying issue.
> >>
> >> It looks like protobuf is not so great dealing with nesting messages
> >> when doing merge or copy. This patch uses an extra copy to bypass that
> >> issue in the protobuf.
> >>
> >> Review: https://reviews.apache.org/r/51992
> >>
> >>
> >> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> >> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/be81a924
> >> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/be81a924
> >> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/be81a924
> >>
> >> Branch: refs/heads/master
> >> Commit: be81a924a9e9414ec98f8c9a87a5391dad865146
> >> Parents: a4fd86b
> >> Author: Jie Yu <yu...@gmail.com>
> >> Authored: Sat Sep 17 14:22:31 2016 -0700
> >> Committer: Jie Yu <yu...@gmail.com>
> >> Committed: Sat Sep 17 14:25:33 2016 -0700
> >>
> >> ----------------------------------------------------------------------
> >> src/slave/containerizer/mesos/utils.hpp | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >> ----------------------------------------------------------------------
> >>
> >>
> >> http://git-wip-us.apache.org/repos/asf/mesos/blob/be81a924/s
> >> rc/slave/containerizer/mesos/utils.hpp
> >> ----------------------------------------------------------------------
> >> diff --git a/src/slave/containerizer/mesos/utils.hpp
> >> b/src/slave/containerizer/mesos/utils.hpp
> >> index 2bb55c1..178ebf3 100644
> >> --- a/src/slave/containerizer/mesos/utils.hpp
> >> +++ b/src/slave/containerizer/mesos/utils.hpp
> >> @@ -27,7 +27,12 @@ static ContainerID getRootContainerId(const
> >> ContainerID& containerId)
> >> {
> >> ContainerID rootContainerId = containerId;
> >> while (rootContainerId.has_parent()) {
> >> - rootContainerId = rootContainerId.parent();
> >> + // NOTE: Looks like protobuf does not handle copying well when
> >> + // nesting message is involved, because the source and the target
> >> + // point to the same object. Therefore, we create a temporary
> >> + // variable and use an extra copy here.
> >> + ContainerID id = rootContainerId.parent();
> >> + rootContainerId = id;
> >> }
> >>
> >> return rootContainerId;
> >>
> >
> >
> >
> > --
> > Best Regards,
> > Haosdent Huang
> >
>
>
>
> --
> Best Regards,
> Haosdent Huang
>
Re: mesos git commit: Fixed a bug in getRootContainerId due to
protobuf copying issue.
Posted by haosdent <ha...@gmail.com>.
From mesos.pb.h/mesos.pb.cc
```
inline ContainerID& operator=(const ContainerID& from) {
CopyFrom(from);
return *this;
}
void ContainerID::CopyFrom(const ::google::protobuf::Message& from) {
if (&from == this) return;
Clear();
MergeFrom(from);
}
```
On Mon, Sep 19, 2016 at 4:32 PM, haosdent <ha...@gmail.com> wrote:
> @Neil, when
>
> ```
> rootContainerId = rootContainerId.parent();
> ```
>
> protobuf would try to call `ContaienrID::Clear()` first and then perform
> `ContainerID::CopyFrom`. Because the parent has been broken after
> `ContainerID::Clear()`, so the `ContainerID::CopyFrom` would get an empty
> value. I think this is not a bug of protobuf and we should avoid using
>
> ```
> Message = Message.xx();
> ```
>
> On Mon, Sep 19, 2016 at 3:42 PM, Neil Conway <ne...@gmail.com>
> wrote:
>
>> Hi Jie,
>>
>> Do you have more details on what exactly the problem is here? If
>> protobuf is unable to copy/merge nested messages in general, that
>> seems like something that might crop up elsewhere.
>>
>> Perhaps we can (a) file a JIRA (ideally with a self-contained
>> test-case), and/or (c) report the problem to upstream?
>>
>> Neil
>>
>> ---------- Forwarded message ----------
>> From: <ji...@apache.org>
>> Date: Sat, Sep 17, 2016 at 11:27 PM
>> Subject: mesos git commit: Fixed a bug in getRootContainerId due to
>> protobuf copying issue.
>> To: commits@mesos.apache.org
>>
>>
>> Repository: mesos
>> Updated Branches:
>> refs/heads/master a4fd86bce -> be81a924a
>>
>>
>> Fixed a bug in getRootContainerId due to protobuf copying issue.
>>
>> It looks like protobuf is not so great dealing with nesting messages
>> when doing merge or copy. This patch uses an extra copy to bypass that
>> issue in the protobuf.
>>
>> Review: https://reviews.apache.org/r/51992
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/be81a924
>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/be81a924
>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/be81a924
>>
>> Branch: refs/heads/master
>> Commit: be81a924a9e9414ec98f8c9a87a5391dad865146
>> Parents: a4fd86b
>> Author: Jie Yu <yu...@gmail.com>
>> Authored: Sat Sep 17 14:22:31 2016 -0700
>> Committer: Jie Yu <yu...@gmail.com>
>> Committed: Sat Sep 17 14:25:33 2016 -0700
>>
>> ----------------------------------------------------------------------
>> src/slave/containerizer/mesos/utils.hpp | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>> ----------------------------------------------------------------------
>>
>>
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/be81a924/s
>> rc/slave/containerizer/mesos/utils.hpp
>> ----------------------------------------------------------------------
>> diff --git a/src/slave/containerizer/mesos/utils.hpp
>> b/src/slave/containerizer/mesos/utils.hpp
>> index 2bb55c1..178ebf3 100644
>> --- a/src/slave/containerizer/mesos/utils.hpp
>> +++ b/src/slave/containerizer/mesos/utils.hpp
>> @@ -27,7 +27,12 @@ static ContainerID getRootContainerId(const
>> ContainerID& containerId)
>> {
>> ContainerID rootContainerId = containerId;
>> while (rootContainerId.has_parent()) {
>> - rootContainerId = rootContainerId.parent();
>> + // NOTE: Looks like protobuf does not handle copying well when
>> + // nesting message is involved, because the source and the target
>> + // point to the same object. Therefore, we create a temporary
>> + // variable and use an extra copy here.
>> + ContainerID id = rootContainerId.parent();
>> + rootContainerId = id;
>> }
>>
>> return rootContainerId;
>>
>
>
>
> --
> Best Regards,
> Haosdent Huang
>
--
Best Regards,
Haosdent Huang
Re: mesos git commit: Fixed a bug in getRootContainerId due to
protobuf copying issue.
Posted by haosdent <ha...@gmail.com>.
@Neil, when
```
rootContainerId = rootContainerId.parent();
```
protobuf would try to call `ContaienrID::Clear()` first and then perform
`ContainerID::CopyFrom`. Because the parent has been broken after
`ContainerID::Clear()`, so the `ContainerID::CopyFrom` would get an empty
value. I think this is not a bug of protobuf and we should avoid using
```
Message = Message.xx();
```
On Mon, Sep 19, 2016 at 3:42 PM, Neil Conway <ne...@gmail.com> wrote:
> Hi Jie,
>
> Do you have more details on what exactly the problem is here? If
> protobuf is unable to copy/merge nested messages in general, that
> seems like something that might crop up elsewhere.
>
> Perhaps we can (a) file a JIRA (ideally with a self-contained
> test-case), and/or (c) report the problem to upstream?
>
> Neil
>
> ---------- Forwarded message ----------
> From: <ji...@apache.org>
> Date: Sat, Sep 17, 2016 at 11:27 PM
> Subject: mesos git commit: Fixed a bug in getRootContainerId due to
> protobuf copying issue.
> To: commits@mesos.apache.org
>
>
> Repository: mesos
> Updated Branches:
> refs/heads/master a4fd86bce -> be81a924a
>
>
> Fixed a bug in getRootContainerId due to protobuf copying issue.
>
> It looks like protobuf is not so great dealing with nesting messages
> when doing merge or copy. This patch uses an extra copy to bypass that
> issue in the protobuf.
>
> Review: https://reviews.apache.org/r/51992
>
>
> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/be81a924
> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/be81a924
> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/be81a924
>
> Branch: refs/heads/master
> Commit: be81a924a9e9414ec98f8c9a87a5391dad865146
> Parents: a4fd86b
> Author: Jie Yu <yu...@gmail.com>
> Authored: Sat Sep 17 14:22:31 2016 -0700
> Committer: Jie Yu <yu...@gmail.com>
> Committed: Sat Sep 17 14:25:33 2016 -0700
>
> ----------------------------------------------------------------------
> src/slave/containerizer/mesos/utils.hpp | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/be81a924/
> src/slave/containerizer/mesos/utils.hpp
> ----------------------------------------------------------------------
> diff --git a/src/slave/containerizer/mesos/utils.hpp
> b/src/slave/containerizer/mesos/utils.hpp
> index 2bb55c1..178ebf3 100644
> --- a/src/slave/containerizer/mesos/utils.hpp
> +++ b/src/slave/containerizer/mesos/utils.hpp
> @@ -27,7 +27,12 @@ static ContainerID getRootContainerId(const
> ContainerID& containerId)
> {
> ContainerID rootContainerId = containerId;
> while (rootContainerId.has_parent()) {
> - rootContainerId = rootContainerId.parent();
> + // NOTE: Looks like protobuf does not handle copying well when
> + // nesting message is involved, because the source and the target
> + // point to the same object. Therefore, we create a temporary
> + // variable and use an extra copy here.
> + ContainerID id = rootContainerId.parent();
> + rootContainerId = id;
> }
>
> return rootContainerId;
>
--
Best Regards,
Haosdent Huang