[pantsd] Daemon lifecycle for options changes. #5045

kwlzn · 2017-11-02T20:38:48Z

Problem

Currently, if options (from cli args, pants.ini, env vars, rc files et al) affecting the daemon change between runs the daemon does not detect this and will happily run with trapped, stale state.

Solution

Fingerprint the relevant subset of bootstrap options at startup time in the thin client and compare that fingerprint to the one used at launch time for the current pantsd process to determine if the daemon needs to be restarted in advance of the run. This piggy-backs on top of the daemon externalization work.

Result

Fixes #3941.

illicitonion · 2017-11-03T13:54:34Z

src/python/pants/pantsd/process_manager.py

+      sep = sep or self.FINGERPRINT_CMD_SEP
+      cmdline = cmdline or []
+      for cmd_part in cmdline:
+        if cmd_part.startswith('{}='.format(key, sep)):


illicitonion · 2017-11-03T14:01:06Z

tests/python/pants_test/pantsd/test_pantsd_integration.py

+      checker.await_pantsd()
+
+      # Check for no stray pantsd-runner prcesses.
+      self.assertFalse(check_process_exists_by_command('pantsd-runner'))


There used to be a checker.assert_running() here which feels important

checker.await_pantsd() implicitly calls assert_running(), so it should be equivalent.

stuhood

Thanks Kris!

stuhood · 2017-11-03T17:58:40Z

src/python/pants/option/option_value_container.py

+    :param list exclude_keys: A list of keys to exclude from fingerprint computation.
+    """
+    exclude_keys = set(exclude_keys or [])
+    acceptable_types = set((bytes, str, list, tuple, dict, int, float, bool, type(None)))


Would be good to define this set statically.

stuhood · 2017-11-03T18:00:42Z

src/python/pants/option/option_value_container.py

+      hasher.update(k)
+      v = self.get(k)
+      assert type(v) in acceptable_types, 'unhashable option type: {}'.format(type(v))
+      # N.B. This relies on implicit string conversion to do the right thing for


This should use src/python/pants/option/options_fingerprinter.py to get the correct behaviour for file-options, etc.

refactored.

stuhood · 2017-11-03T18:42:33Z

src/python/pants/pantsd/pants_daemon.py

@@ -169,6 +169,20 @@ def watchman_launcher(self):
  def is_killed(self):
    return self._kill_switch.is_set()

+  @property
+  def options_fingerprint(self):
+    return self._bootstrap_options.sha1(


There is already a facility for marking options as fingerprint=True... it would be good to reuse that here if possible:

pants/src/python/pants/option/options.py

Line 320 in 33e98e1

def get_fingerprintable_for_scope(self, scope, include_passthru=False):

great idea. refactored.

stuhood · 2017-11-03T18:58:40Z

src/python/pants/pantsd/process_manager.py

+  def fingerprint(self):
+    """The fingerprint of the current process.
+
+    :returns: The fingerprint of the running process as read from the process table, ProcessManager


It's not clear why this is an OR... a bit more information about why we would check X then Y would be good.

added a comment in the docstring.

stuhood · 2017-11-04T00:40:11Z

Well, crap. It did not occur to me that so many of these are not already marked fingerprint=True, because they should not affect target fingerprinting.

Can probably completely ignore my feedback. Sorry!

stuhood

(pre-approving: but you'll need to revert the addition of fingerprint=True)

kwlzn · 2017-11-04T04:39:25Z

@stuhood think I see another way - can mark these daemon=True and parameterize the match key in get_fingerprintable_for_scope() accordingly. pushed a change - let me know what you think.

stuhood

This feels cleaner to me then the blacklist of options, so I think it's the right call.

But some sanity check that daemon=True has been passed outside of global options (I think: for any scope other than the global scope ?) would be good. Another thought would be to see whether it's possible to invert the default in this context.

kwlzn · 2017-11-04T07:30:47Z

But some sanity check that daemon=True has been passed outside of global options (I think: for any scope other than the global scope ?) would be good. Another thought would be to see whether it's possible to invert the default in this context.

done and done.

Problem Currently, if options (from cli args, pants.ini, env vars, rc files et al) affecting the daemon change between runs the daemon does not detect this and will happily run with trapped, stale state. Solution Fingerprint the relevant subset of bootstrap options at startup time in the thin client and compare that fingerprint to the one used at launch time for the current pantsd process to determine if the daemon needs to be restarted in advance of the run. This piggy-backs on top of the daemon externalization work. Result Fixes pantsbuild#3941.

kwlzn mentioned this pull request Nov 2, 2017

daemon invalidation for source code modifications #5046

Closed

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch 5 times, most recently from 353ce56 to 1ef31bf Compare November 2, 2017 23:08

[pantsd] Improve options invalidation.

503cdba

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch 2 times, most recently from b43fd8e to df179a8 Compare November 3, 2017 04:38

kwlzn changed the title ~~[WIP] [pantsd] Daemon lifecycle for options changes.~~ [pantsd] Daemon lifecycle for options changes. Nov 3, 2017

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch 2 times, most recently from 5fcc82c to c32408e Compare November 3, 2017 05:30

Test.

148e236

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch from c32408e to 148e236 Compare November 3, 2017 05:58

illicitonion approved these changes Nov 3, 2017

View reviewed changes

Format str.

1563523

kwlzn requested review from stuhood, jsirois and benjyw November 3, 2017 16:24

Alternate between cli arg and env var.

4f83872

stuhood reviewed Nov 3, 2017

View reviewed changes

Isort.

dc0b9b9

stuhood approved these changes Nov 4, 2017

View reviewed changes

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch from 826473f to 279678a Compare November 4, 2017 04:35

fingerprint=True -> daemon=True

6115528

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch from 279678a to 6115528 Compare November 4, 2017 06:13

stuhood approved these changes Nov 4, 2017

View reviewed changes

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch 2 times, most recently from 4e4c8e5 to 53f1141 Compare November 4, 2017 08:18

Validation and option default inversion.

5e85274

kwlzn force-pushed the kwlzn/daemon_option_invalidation branch from 53f1141 to 5e85274 Compare November 4, 2017 08:23

kwlzn merged commit 1148b07 into pantsbuild:master Nov 4, 2017

stuhood mentioned this pull request Dec 6, 2017

Import statements can be banned in BUILD files #5180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pantsd] Daemon lifecycle for options changes. #5045

[pantsd] Daemon lifecycle for options changes. #5045

kwlzn commented Nov 2, 2017

illicitonion Nov 3, 2017

kwlzn Nov 3, 2017

illicitonion Nov 3, 2017

kwlzn Nov 3, 2017

stuhood left a comment

stuhood Nov 3, 2017

kwlzn Nov 4, 2017

stuhood Nov 3, 2017

kwlzn Nov 4, 2017

stuhood Nov 3, 2017

kwlzn Nov 4, 2017

stuhood Nov 3, 2017

kwlzn Nov 4, 2017

stuhood commented Nov 4, 2017 •

edited

Loading

stuhood left a comment •

edited

Loading

kwlzn commented Nov 4, 2017

stuhood left a comment •

edited

Loading

kwlzn commented Nov 4, 2017

[pantsd] Daemon lifecycle for options changes. #5045

[pantsd] Daemon lifecycle for options changes. #5045

Conversation

kwlzn commented Nov 2, 2017

Problem

Solution

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Nov 4, 2017 • edited Loading

stuhood left a comment • edited Loading

Choose a reason for hiding this comment

kwlzn commented Nov 4, 2017

stuhood left a comment • edited Loading

Choose a reason for hiding this comment

kwlzn commented Nov 4, 2017

stuhood commented Nov 4, 2017 •

edited

Loading

stuhood left a comment •

edited

Loading

stuhood left a comment •

edited

Loading