Robustify pantsd + watchman integration tests.

Review Request #3912 — Created May 20, 2016 and submitted

kwlzn
pants
kwlzn/pantsd/3377
3377, 3393
pants-reviews
benjyw, jsirois, nhoward_tw, peiyu, stuhood

Fixes #3377.

  • Make failure to successfully launch watchman (when configured) a fatal event.
  • Add an option to override the watchman UNIX socket path to get around the 'path too long' issue with UNIX sockets.
  • Make ProcessManager's .pids dir relocatable - and isolate that in the integration tests.
  • More strictly check the daemon state between integration test runs.
  • Misc cleanups, tests.

CI is green @

  • https://travis-ci.org/pantsbuild/pants/builds/132437726
  • http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/branch/PR-3393/30

I submitted multiple CI-triggering commits last night to attempt to repro the initial issue. Other than what appear to be spurious failures in Jenkins, that seems to be successful - and is green-lit across 4+ Travis runs (including several not-accounted-for rebuilds).

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. Should mention the "global options proxy" thing in the description.


    Comment on the strategy: rather than using a global directory for pids throughout the unit tests, I'd love to see handling in BaseTest for creating and then destroying a unique daemons dir during setup/teardown. Currently, I'm worried that different tests will end up using one another's daemons and cross-polluting test results.

    1. good call - fixed this aspect up and killed the 'global options proxy' bit.

  2. type=str is the default

  3. Should explain why probably.

  4. A little blurb indicating why this is necessary would be good.

    1. moved away from this approach.

  5. src/python/pants/option/subsystem/global_options.py (Diff revision 1)
     
     
     
     
     
     

    Frankly, for this usecase the Factory seems like overkill.

    1. just trying to be consistent with the pattern for newer subsystems being implemented nowadays, which afaict seems to be strategically setting things up for eventual elimination of the "subsystems as magic singletons" paradigm.

      if you feel strongly tho, I'm happy to change this to a vanilla-form subsystem.

  6. This scope is likely to be very, very confusing.

    Maybe we can coerce Benjy into giving his opinion on how we can convert global options parsing into a subsystem without this proxy?

    1. yeah, fwiw - I tried '' (the real scope for global options) and 'GLOBAL' - both threw deprecation warnings due to validation of lowercase alpha characters + dashes, so just went with an allowable name that worked initially given that this scope wasn't actually utilized.

      migrated this to the 'subprocess' scope in its new form.

  7. tests/python/pants_test/base_test.py (Diff revision 1)
     
     

    Should this be on BaseTest itself, or does it intentionally not use BaseTest.build_root? If it's intentionally not using the build root from BaseTest, I wonder whether it should be here or elsewhere.

    1. killed this in favor of logic in BaseTest.setUp().

  8. Since this extends BaseTest and runs in it's own build_root, I think it will be unlikely for it to find the correct thing here?

    1. well, in this case ProcessGroup et al doesn't currently rely on ProcessManager metadata to function - just it's API wrapped around an already enumerated pid - so the value here gets ignored in favor of keying against process names/args + property keys connecting this to the buildroot. however, given this bit runs in the tearDownClass() context and new buildroots are assigned every setUp() I would expect these buildroots to have never lined up in the first place.

      but you're totally right - I didn't realize BaseTest was implicitly changing the buildroot and had mistakenly assumed these were all keying to the same static location like they did in the non-BaseTest tests.

      fixed both issues across the board.

  9. 
      
  1. TBH I don't really like the GlobalOptions approach - it's confusing and hacky.

    A) Instead of this weird, roundabout way to get your hands on the global Options instance, we can make a more straightforward way.
    B) Why not make a Subprocess subsystem that contains all options related to spawning subprocesses, and put that one option there instead
    of on the global scope, and get rid of the problem entirely?

    1. well, for one: having this option on the global scope allows us to plumb it directly to the pantsd thin client just after OptionsBootstrapper (and before the heavier OptionsInitializer), the same place we determine if pantsd is enabled and attempt to use it (via the metadata read from this pid path).

      however, this aspect really only affects which subsystem the option is placed on. for the subsystem itself, I'd be completely fine with renaming that to Subprocess for the ProcessManager access - but it'd still just be a ~proxy to the global options.

      fwiw, the GlobalOptions approach seemed like a reasonable way to establish a stake in the ground toward "global options as a subsystem" so that tasks/subsystems could then formally depend on those (and consider dependency in the task fingerprinting etc).

      very open to further ideas on this tho.

    2. The trouble with stakes in the ground is that people tend to trip over them...

      This is something that needs to be thought through carefully, and not casually dropped in. There are several potential issues with making global options into a subsystem. For example, the implications on the semantics of the naming hierarchy are unclear. So I am far from convinced that this is even a direction we should go in.

      I'm actually not even sure how your change can work. We only set options on subsystems in OptionsInitializer. They should be None before OptionsInitializer runs.

    3. roger that. changed the approach here.

    4. Why not just factor the responsibility of loading plugins out of OptionsInitializer so it can be a lightweight means of initializing Optionables (Tasks, Subsystems) given a collection of them from elsewhere.  The thin client can pass its limited set of subsystems - possibly 1 - no plugin loading needed - to the refactored OptionsInitializer.  The fat client can use the factored out plugin-loading code to form its fat set of Subsytems and Tasks to pass in to its OptionInitializer.
    5. maybe I'm missing something - but isn't this precisely the point of OptionsBootstrapper and the bootstrap options, sans the Subsystem dependency bit?

      afaict, we'll always need bootstrap options - thus I'm not sure I see the value of dragging in an extra OptionsInitializer call at this stage of the run purely to rely on Subsystem options vs global options. imho, --enable-pantsd and --pants-subprocessdir seem well aligned with the nature of bootstrap options. esp as we achieve deeper daemon integration they will be more or less fundamental to how pants runs at its core.

      fwiw, the relevant bootstrapping and options usage for the thin client are laid out here: https://github.com/twitter/pants/blob/3bc6763bbbba2b9e08a1e2eff05e7c92d0bf544d/src/python/pants/bin/pants_runner.py#L48-L57

      but please let me know if this all sounds off - I'm very interested in understanding what folks think is wrong in the latest changeset so that I can adjust my understanding accordingly.

    6. My bad. I missed diff 2 - my help was directed at diff 1. Sorry for the noise, looks like you deleted the new way of doing global options that was the focus of this discussion.
    7. ah, gotcha. no worries!

  2. 
      
  1. 
      
  2. src/python/pants/option/global_options.py (Diff revision 2)
     
     
     
     
     

    I wonder whether it would be easier in the long run to move from a "rm -Rf .pants.d" approach, to a "remove everything in .pants.d except pantsd" approach. Then this (and anything else that is not "cleanable") could continue to live within the workdir. Would love to be consolidating mutable directories rather than adding more...

    1. sure, could definitely make sense longer term to consolidate this and others (~/.ivy2, ~/.cache, ~/.pex, etc) under the workdir and apply a more selective cleaning policy.

  3. Can you add a comment explaining why it needs to be global (I think I know, but).

  4. 
      
  1. 
      
  2. src/python/pants/pantsd/process_manager.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     

    Not sure the significance between metadata_base_dir and Subprocess.Factory.global_instance().create().get_subprocess_dir(), but if self._metadata_base_dir has to be assigned with some value, would it make sense to use the default --pants-subprocessdir?

    1. not sure I follow your question?

    2. non blocking. More of a newbie question, I was hoping you could explain why _metadata_base_dir has to be optional and why sometimes we need to use Subprocess.Factory.global_instance().create().get_subprocess_dir()

    3. ah. Subprocess is a Subsystem, which doesn't work without the subsystem being initialized. so the two main cases you'd override metadata_base_dir would be 1) in early pants run bootstrapping before Subsystems are initialized and 2) in unit tests, where it's not desirable to have to initialize subsystems.

    4. Makes sense. Thanks!

  3. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

thanks folks. submitted @ 1f554c8ca1fc56b4830091d7c75aac3056785e9e

Loading...