strands-project / mongodb_store

MongoDB tools for storing and analysing runs of ROS systems.
BSD 3-Clause "New" or "Revised" License
49 stars 72 forks source link

[mongodb_store] apply machine tag for each nodes #148

Closed furushchev closed 8 years ago

strands-jenkins commented 8 years ago

Can one of the STRANDS admins verify this patch? Post a comment containing "ok to test" to enable Jenkins builds for this pull request.

marc-hanheide commented 8 years ago

Can you explain why you believe this to be required?

furushchev commented 8 years ago

We use this launch file giving use_machine:=false but with other machine tags like https://github.com/PR2/pr2_common/blob/indigo-devel/pr2_machine/pr2.machine

I must specify machine to execute mongodb_store server on the same machine on which mongodb daemon is running, because connecting pymongo to localhost mongodb daemon is useful for decreasing data traffic.

marc-hanheide commented 8 years ago

thanks. @jailander or @hawesie I don't fully grasp the implication this might have in our setup. Can you comment?

Jailander commented 8 years ago

@furushchev I am not sure this is the way around your problem or maybe is that I don't understand your problem :/
What this PR would do is to basically add the machine argument to the nodes but for that to work the machine label must have been created somewhere in the launch file, for example, in this case this machine is the default machine for all the nodes in this launch file

https://github.com/furushchev/mongodb_store/blob/add-machine-tag/mongodb_store/launch/mongodb_store.launch#L16

if you notice in that line the default parameter is set to true which means that all the nodes in the launch file will be launched in $(arg machine) so in that sense this PR has no effect when use_machine:=true

However if you launch it with use_machine:=false it might fail as the machine tag might not have been defined.

I think the best solution for your problem is to ensure that this is launched in the machine you choose so for example following your machine tags you could launch it doing:

roslaunch mongodb_store mongodb_store.launch machine:="c1"

and making sure ROS_ENV_LOADER is equally defined on all the machines/users you launch nodes running with the same machine tag "c1" (in this example)

If what you want for it is to run on localhost you can just launch the launch file without any arguments (that are machine realted), this should work unless another launch file with the machine tag localhost has been launched on which the address field or the ROS_ENV_LOADER are different from this one.

I hope I am understanding your problem correctly

furushchev commented 8 years ago

@Jailander Thank you for taking time to review and advice. I understand theoretically it works on machine c1 by: roslaunch mongodb_store mongodb_store.launch machine:="c1" The problem is that if you use machine tag of same name or address, you must write strictly the same as other machine tags on other included launch files. For example, if we create a launch file like below:

<launch>
  <include file="$(find pr2_machine)/pr2.machine" />
  <include file="$(find mongodb_store)/launch/mongodb_store.launch">
    <arg name="machine" value="c2" />
  </include>
</launch>

it does not work.

$ roslaunch test.launch
... logging to /home/applications/.ros/log/98a08062-74b5-11e5-8832-001517c3b779/roslaunch-pr1012-29578.log
Checking log directory for disk usage. This may take awhile.
Press Ctrl-C to interrupt
WARNING: disk usage in log directory [/home/applications/.ros/log] is over 1GB.
It's recommended that you use the 'rosclean' command.

Machine [c2] already added and does not match duplicate entry

This causes because machine entry is not strictly equal to the others:

in pr2_machine/pr2.machine: <machine name="c2" address="c2" env-loader="$(env ROS_ENV_LOADER)" />

in mongodb_store/mongodb_store.launch (variables are assigned as machine:=c2) <machine name="c2" address="c2" env-loader="$(optenv ROS_ENV_LOADER )" user="" default="true" />

which there are difference (env and optenv, user/default attribute) This is the specification of roslaunch.

(If you use machine tags which is strictly the same, it works without any problem. e.g. sample_a.launch

<launch>
  <include file="$(find pr2_machine)/pr2.machine" />
  <node name="" ... />
</launch>

sample_b.launch

<launch>
  <include file="$(find pr2_machine)/pr2.machine" />
  <include file="sample_b.launch" />
</launch>

roslaunch sample_b.launch -> it works. )

We use this mongodb_store.launch file with multi including like: pr2_bringup.launch -> mongodb.launch -> mongodb_store/mongodb_store.launch In such a situation, there is a problem.

Jailander commented 8 years ago

hi @furushchev you are completely right if all the machine tags are not defined exactly the same this will fail. @hawesie @marc-hanheide should we go back to the idea of having a strands specific launch file with our machine tags and other generical with no machine tags whatsoever for other users? @furushchev would that work for you?

furushchev commented 8 years ago

@Jailander Yes, I think it is very great for me.

furushchev commented 8 years ago

So should I remove machine tag in launch file in this PR?

Jailander commented 8 years ago

Hi let me ping @hawesie and @marc-hanheide again, because if we change the name the starting scripts for the STRANDS system have to be modified, we could create a new file without machine tags but maybe is confusing.

I think removing all machine tags from mongodb_store.launch and moving the current version to mongodb_strands.launch is the way to go but I would like to hear from them. @furushchev If they haven't replied in a while I'll do it today.

marc-hanheide commented 8 years ago

@Jailander I seriously don't have the full grasp of the machine tag problem... I would, however, at this point suggest to create a new launch file without machine tags, rather than changing the one we use (It's used in ways too many places). I understand the version @furushchev is suggesting wouldn't work for us? In that case, I really suggest to - for now - create a launch file store.launch or similar without machine tags at all (if that solves @furushchev's problem). We can at some later point rename mongodb_store.launch to something more STRANDS specific.

Jailander commented 8 years ago

That was my doubt exactly, I'll do so, this should solve the problem.

hawesie commented 8 years ago

Give me a day or so to try to get my head around this and then I’ll come back to you.

aginika commented 8 years ago

ping

hawesie commented 8 years ago

Here's my attempt to summarise things.

  1. By default our existing mongodb_store launch file defines a machine tag based on input arguments
  2. Standard practice is to define machine tags in a separate file and then use them in the machine argument to a node.
  3. When these two approaches are used together our existing mongodb_store launch file causes a failure by defining a second machine tag with the same name as the first one in the separate file and which may not exactly match the first definition, preventing it from being used.
  4. We would like a mongodb_store launch file that can be both launched with no machine arguments (therefore defaulting to localhost) and can be launched with a machine argument. Currently this only works if the machine argument hasn't been previously defined (for reasons above).
  5. The sticking point is that currently in STRANDS we assume that the machine argument should cause the creation of a machine tag, but this is what is breaking stuff.
  6. I don't want two different launch files ;)

What I'd like to have is the solution where machine tags can be defined elsewhere and used in this launch file (as we will get better uptake by matching the standards in use on the PR2). I think I see a way ahead, but I want to check one thing first: what happens if a node gets the machine argument localhost but we haven't defined a machine tag called localhost? Is this created somewhere by default anyway, or would it cause a failure?

hawesie commented 8 years ago

My inclination is that we should accept this PR, but with the addition that use_machine is renamed to define_machine_tag and is set to false by default. If I'm correct then this should make @furushchev's use case work, but would require STRANDS users to set define_machine_tag to true in their launch files if they want to. The problem is that if no argument is set, and the localhost tag is not predefined, then this will cause a failure, won't it?

furushchev commented 8 years ago

@hawesie You're completely correct. It is bad spec of roslaunch, I think. Practical solution we can select will be:

hawesie commented 8 years ago

Yay!

Do we need to define all nodes twice? Can't we just use the machine="XXX" versions as we will make it so that all versions have a defined machine tag: either its done externally, or we do it internally as either localhost or as the value given as input.

hawesie commented 8 years ago

@furushchev what do you think about the last comment? Once we have resolved this I can create new releases with all recent updates.

furushchev commented 8 years ago

If you don't want to write define any machine tag on this launch file, the only way, I think, is to define all nodes twice: with machine attribute and w/o, and switch them by argument. Or if you allow to define some default machine tag, following are possible.:

<launch>
<arg name="use_external_machine_tag" default="false" />
<arg name="machine" default="localhost" />

<machine name="localhost" address="localhost" unless="$(use_external_machine_tag)" />

<node name="~" ... machine="$(arg machine)" />
...
</launch>
hawesie commented 8 years ago

The more I look at this, the more I don't think I can achieve the design I want. Basically I want to say: "if machine is not set, then define a machine tag with localhost and use it, else use the machine tag indicated by machine" but roslaunch doesn't support argument testing as far as I can tell.

hawesie commented 8 years ago

See #151.