logviewer support for configurable storm.log.dir

[erikdw]

but fall back to hardcoded mesos-sandbox path.

Notably, storm.log.dir is the configuration parameter in storm for choosing the log directory to be used (it's set in the logback (storm-0.9.x) & log4j 2 (storm-0.10.0+) config files).

[erikdw]

@dsKarthick @JessicaLHartog @DarinJ : please review at your convenience

[dsKarthick]

@erikdw MESOS_SANDBOX is set by Mesos correct? Wondering if we should override System.getenv("MESOS_SANDBOX") with logdir only if storm.log.dir is configured?

[dsKarthick]

Ok. I spoke too soon. I see that you have done exactly what I wanted in the LogViewerController function above :)

[erikdw]

yup yup! ;)

[erikdw]

@DarinJ & @dsKarthick : thank you for the review, I just pushed a change that addresses your comments.

NOTE: I also noticed some setting of storm.log.dir to the mesos sandbox in the Docker command line shim methods, and in the marathon config file, and in the README:

% git grep storm.log.dir
bin/run-with-marathon.sh:    ./bin/storm ui -c ui.port=$PORT0 -c nimbus.thrift.port=$PORT1 -c storm.log.dir=$MESOS_SANDBOX/logs ${STORM_UI_OPTS:-}
bin/run-with-marathon.sh:  ./bin/storm-mesos nimbus -c mesos.master.url=zk://${MESOS_MASTER_ZK}:2181/mesos -c storm.zookeeper.servers="[\"${MESOS_MASTER_ZK}\"]" -c nimbus.thrift.port=$PORT1  -c storm.log.dir=$MESOS_SANDBOX/logs  ${STORM_NIMBUS_OPTS:-}
storm-shim-10x/src/main/java/storm/mesos/shims/DockerCommandLineShim.java:        "-c storm.log.dir=$MESOS_SANDBOX/logs%s",
storm-shim-9x/src/main/java/storm/mesos/shims/DockerCommandLineShim.java:        "-c storm.log.dir=$MESOS_SANDBOX/logs%s",
storm/src/main/storm/mesos/logviewer/LogViewerController.java:        "storm.log.dir=" + logDir,
storm/src/main/storm/mesos/util/MesosCommon.java:  public static final String STORM_LOG_DIR_CONF = "storm.log.dir";

For now I'm going to ignore this, but it probably needs to be readdressed later.

[erikdw]

So I said I'm going to ignore the Docker command line references, but now I'm thinking that I'll handle it by changing the MesosCommon.getStormLogDir() method to take a String which will be the default logDir to return. And then I'll pass the appropriate default from the Docker shims as well as the LogViewerController.

NOTE: as an aside, I wonder why the docker command lines have log.dir set explicitly like this, but the non-docker ones do not.

[erikdw]

And that latest plan is hitting a roadblock: MesosCommon is not reference-able from within the storm-shim-9x/storm-shim-10x source code. :_(

[DarinJ]

Looked a bit at the docker-shim code. I think the goal was to keep the logs from being written to the container itself. This has a few advantages. One it keeps the container size from growing, I've seen that fill /var/lib/docker when logs are written in the container. Two it keeps people from having to exec in to look at logs, this approach lets them simply look at the sandbox. I think this might be desired behavior in all real cases.

I'll look at the shim though to see what changes we could make to allow the majority of the code be visible from the shims. Likely this will involve refactoring anything not calling the shims to a common sub-project. Another possibility is to replace LocalState with a Map of some type and go back to one build. I think it'd be easy enough to adjust the command lines once at run-time.

[erikdw]

@DarinJ

1. Thanks for the explanation about docker -- I don't have any experience with really running docker containers under mesos. What you said makes sense (I think!).
2. Regarding "replacing LocalState with a Map": the only thing we use the LocalState mechanism for today (after Unique TaskIDs #106) is simply:
   • storage of the framework ID onto disk, so that when we restart MesosNimbus it can look up that value and appear as the same instance of the framework when MesosNimbus reconnects with Mesos.
   • So perhaps instead of a memory-based Map, we should just write the ID to a file.
     • The file path can just be: the localDir + /storm-mesos_framework-id, or some such ish.
       • NOTE: From my observations, localDir is the cwd of the MesosNimbus process on startup, followed by storm-local/nimbus/inimbus/. Here is how storm constructs localDir.
3. If we do the above, it's probably ok to rip out all the hard work you did for the shims / modules, since as you said the command line construction is probably relatively easy to do at runtime somehow (just swapping storm for storm.py afterall!).

So: I propose for now that I put this logviewer fix on the backburner, and I'll try to do the refactor proposed here. Probably won't start on it until late tomorrow or maybe Tuesday, since I need to put work in on #117 and #120, so please let me know if you have any concerns with the proposal. Thanks!

[DarinJ]

@erikdw sounds fine, if you want me to look at anything let me know. A bit swamped right now, but should be able to look things over and provide comments.

[erikdw]

@DarinJ : great, I'm also swamped (as always 😉 ), but I'll "at" you on the PR when I send it for a cursory review.

[erikdw]

Sooo.... I'm planning to just close this without fixing it.

Reasoning:

1. The auto-launching of logviewer is totally broken in storm-mesos even with this fix. See this comment in #6, this other comment in #6, and STORM-1342 for more info.
2. The work proposed above is a lot for minimal gain given the previous explanation about logviewer being totally broken anyways.
3. It would be a PITA to upgrade from the storm-mesos version prior to the proposed fix above to the one with the proposed fix. i.e., if people wanna keep their framework running, then we'd have to tell them how to extract their framework ID out of the serialized LocalState file and put it into whatever other file we are using.