> I'm surprised we haven't run into issues like this before on Linux. Indeed!
> I'm wondering if this warrants a general solution that could take care of all > filesystem-disallowed characters. I think yes and no. Yes: it'd be easy to just `process::http::encode(executorId/taskId)` (though it might look funny) right where I'm currently applying the same but only to `:`. No: As the only character we've run into a problem with is `:` (MESOS-9109), it might not be worth it to generalize this to solve a bunch of problems that we haven't encountered. I'm somewhat comfortable doing so only for Windows, as we don't really need to worry about the recovery scenario; but very uncomfortable about doing so for Linux etc., for precisely that reason. So expanding this is definitely up for debate; but we must fix the bug with `:`. Thanks for the feedback, Andy On 08/22/2018 5:11 pm, Greg Mann wrote: > Thanks for addressing this Andy!! AFAIK we allow all characters in executor > and task IDs; I'm surprised we haven't run into issues like this before on > Linux. > > The percent-encoding approach seems fine to me. As long as the percent > character isn't an issue on any filesystems that we're interested in? As a > starting point, Wikipedia seems to have a decent survey of restrictions on > different filesystems here [5]. Looks like the percent character may be fine. > > I wonder if there are other characters we should be concerned about? I'm > guessing we should worry about slashes and backslashes as well? Seems like a > more general solution might help us avoid similar pitfalls in the future. > Perhaps we could just percent-encode executor and task IDs before we write to > disk? If we did this, we would have issues during recovery to consider, where > we need to look for "old" paths when recovering state from an "old" agent. > > In any case, I'm wondering if this warrants a general solution that could > take care of all filesystem-disallowed characters. WDYT? > > Cheers, > Greg > > On Tue, Aug 21, 2018 at 2:02 PM, Andrew Schwartzmeyer > <and...@schwartzmeyer.com> wrote: > >> Hey all, >> >> I have a set of patches up for MESOS-9109 that I need reviewed, starting >> here: https://reviews.apache.org/r/68297/ [1]. >> >> Eduard here was trying to use Chronos to schedule a task on a Windows agent, >> and found an error due to the fact that Chronos uses colons (as in `:`) in >> its generated framework (and task) IDs. Now, to maintain backward >> compatibility, we obviously can't disallow the use of `:` as there are >> frameworks already using it. However, this is a reserved character on >> Windows for file system paths >> (https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file [2]), >> so it cannot be in the path. >> >> My first implementation simply applied `s/:/_COLON_` to `frameworkId` and >> `taskId` in the functions in `paths.cpp` which generate Mesos's filesystem >> paths. While this worked, it's kind of a kludge. Or that is to say, it would >> nicer to use the ASCII representation of `%3A` instead. Doing so, however, >> revealed a bug in libprocess (MESOS-9168) that I have also fixed and need >> reviewed, starting here: https://reviews.apache.org/r/68420/ [3] >> >> So combining the two fixes, the chain maps `:` in `frameworkId` and `taskId` >> to `%3A` (and back when appropriate). This obviously doesn't fix any >> third-party tooling, but being Windows, I don't think there is any yet to >> worry about. >> >> I wanted to get this in for 1.7, but due to a miscommunication, we were not >> able to land it in time. If you can, please review! Or if you have a better >> way of doing this, let me know! >> >> Thanks, >> >> Andy >> >> P.S. Original discussion here: >> https://mesos.slack.com/archives/C1LPTK50T/p1533324650000396 [4] (our Slack >> archives seem to be down, so this is only available until Slack cycles out >> sadly). Links: ------ [1] https://reviews.apache.org/r/68297/ [2] https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file [3] https://reviews.apache.org/r/68420/ [4] https://mesos.slack.com/archives/C1LPTK50T/p1533324650000396 [5] https://en.wikipedia.org/wiki/Filename#Comparison_of_filename_limitations
