RFC: moving chromium infra specific paths from recipes-py to build repo

[Paweł Hajdan, Jr.]

I'd like to move chromium infra specific paths from https://github.com/luci/recipes-py/blob/master/recipe_modules/path/config.py and https://github.com/luci/recipes-py/blob/master/recipe_modules/path/api.py to e.g. infra_paths recipe module in build repo.

This is the code in question:

def _lazy_set_config(self):

    if self._config_set:

      return

    self._config_set = True

    path_config = self.m.properties.get('path_config')

    if path_config in ('kitchen', 'swarming'):

      self.set_config(path_config)

    else:

      self.set_config('buildbot')

@config_ctx()

def buildbot(c):

  c.base_paths['root'] = c.CURRENT_WORKING_DIR[:-4]

  c.base_paths['slave_build'] = c.CURRENT_WORKING_DIR

  c.base_paths['cache'] = c.base_paths['root'] + (

      'build', 'slave', 'cache')

  c.base_paths['git_cache'] = c.base_paths['root'] + (

      'build', 'slave', 'cache_dir')

  c.base_paths['goma_cache'] = c.base_paths['root'] + (

      'build', 'slave', 'goma_cache')

  for token in ('build_internal', 'build', 'depot_tools'):

    c.base_paths[token] = c.base_paths['root'] + (token,)

  c.dynamic_paths['checkout'] = None

@config_ctx(includes=['buildbot'])

def swarming(c):

  c.base_paths['slave_build'] = (

      c.CURRENT_WORKING_DIR[:1] +

      ('b', 'fake_build', 'slave', 'fake_slave', 'build'))

@config_ctx()

def kitchen(c):

  c.base_paths['root'] = c.CURRENT_WORKING_DIR

  c.base_paths['slave_build'] = c.CURRENT_WORKING_DIR

  # TODO(phajdan.jr): have one cache dir, let clients append suffixes.

  # TODO(phajdan.jr): set persistent cache path for remaining platforms.

  # NOTE: do not use /b/swarm_slave here - it gets deleted on bot redeploy,

  # and may happen even after a reboot.

  if c.PLATFORM == 'linux':

    c.base_paths['cache'] = (

        '/', 'b', 'cache', 'chromium')

    c.base_paths['git_cache'] = (

        '/', 'b', 'cache', 'chromium', 'git_cache')

    c.base_paths['goma_cache'] = (

        '/', 'b', 'cache', 'chromium', 'goma_cache')

  else:

    c.base_paths['cache'] = c.base_paths['root'] + ('cache',)

    c.base_paths['git_cache'] = c.base_paths['root'] + ('cache_dir',)

    c.base_paths['goma_cache'] = c.base_paths['root'] + ('goma_cache',)

  c.dynamic_paths['checkout'] = None

WDYT?

Paweł

[Marc-Antoine Ruel]

Does this ^^ needs to be kept?

 

@config_ctx()

def kitchen(c):

  c.base_paths['root'] = c.CURRENT_WORKING_DIR

  c.base_paths['slave_build'] = c.CURRENT_WORKING_DIR

  # TODO(phajdan.jr): have one cache dir, let clients append suffixes.

  # TODO(phajdan.jr): set persistent cache path for remaining platforms.

  # NOTE: do not use /b/swarm_slave here - it gets deleted on bot redeploy,

  # and may happen even after a reboot.

  if c.PLATFORM == 'linux':

    c.base_paths['cache'] = (

        '/', 'b', 'cache', 'chromium')

    c.base_paths['git_cache'] = (

        '/', 'b', 'cache', 'chromium', 'git_cache')

    c.base_paths['goma_cache'] = (

        '/', 'b', 'cache', 'chromium', 'goma_cache')

  else:

    c.base_paths['cache'] = c.base_paths['root'] + ('cache',)

    c.base_paths['git_cache'] = c.base_paths['root'] + ('cache_dir',)

    c.base_paths['goma_cache'] = c.base_paths['root'] + ('goma_cache',)

  c.dynamic_paths['checkout'] = None

WDYT?

Paweł

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPZ0k7gD8rBqz0HthC65sT8MdYMw2nQySjTFzRQH2fx1mg%40mail.gmail.com.

--

M-A

[Robert Iannucci]

Moving that stuff out of recipes-py sounds good to me.

I do think that all of those things will eventually be replaced with actual recipe module dependencies as the recipes-in-repos sort of untangle from the current recipes-in-a-giant-ball situation. But moving infra specific stuff out of recipes-py always sgtm.

R

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CANAQWOXpvYVwp2wsS%3DUcR%2BmhsFq%2BMXMSOUwuKqwLw9JYDG5G%3DQ%40mail.gmail.com.

[Paweł Hajdan, Jr.]

On Thu, Apr 21, 2016 at 8:18 PM, Robert Iannucci <iann...@chromium.org> wrote:

Moving that stuff out of recipes-py sounds good to me.

Cool! FWIW, I'm working on CLs that would move things to depot_tools - since e.g. the bot_update and other modules depend on this infra-specific parts like slave_build. This isn't necessarily ideal, and I'll be thinking how to improve that, but I'd prefer to proceed in small steps, and IMO taking this out of recipe engine will be one of the bigger wins here.

Let me know how that sounds to you.

Responding to M-A's question, the swarming config can probably go away.

 

I do think that all of those things will eventually be replaced with actual recipe module dependencies as the recipes-in-repos sort of untangle from the current recipes-in-a-giant-ball situation. But moving infra specific stuff out of recipes-py always sgtm.

I'd like to understand above more, in the context of infra-specific parts. While things like build, depot_tools, and slave_build would likely indeed go away, we'd need some way to get paths for the bot-side cache.

That's going way more into the future - I'm just interested; if you have some thoughts on this, please share.

Paweł

[Paweł Hajdan, Jr.]

To make the discussion more specific, I uploaded https://codereview.chromium.org/1906323003 and https://codereview.chromium.org/1915463002 . From there, adjusting build and build_limited should be rather mechanical. I consider the CLs ready for final review if you want to, but in terms of design it's a starting point for discussion. If you like it - great. If you have change suggestions, please share. Note I've tried to avoid too radical changes to the recipe engine for this.

Paweł

[Paweł Hajdan, Jr.]

I'd like to comment Robbie's suggestion from https://chromiumcodereview.appspot.com/1915463002/diff/1/recipe_modules/infra_paths/api.py#newcode33 here:

I have considered just moving the config for path recipe module outside of recipe engine, and having some kind of hook for repos to do a setup common for that repo.

IMO having every single recipe call some piece of code just to initialize paths properly is too much of a burden and boilerplate.

What I'm worried in case we allow repo hooks, is what order should they be evaluated in. With multiple repos there may be several valid ways to topologically sort the graph, which would result in different orders of execution, leading to different results. That seems dangerous.

I posted the CLs (again: https://codereview.chromium.org/1906323003 and https://codereview.chromium.org/1915463002) mostly to have some specific starting point to refer to. For alternative ideas, such as hooks or anything, could you aim for something similarly specific? If not CLs, at least as precise description as possible, and comment on the caveats I mentioned above.

Paweł

[Robert Iannucci]

Yeah what you say has worried me a bit too. Path config is really a product of the runtime environment, not a property of the recipe. I think there is a good solution, but I'm not sure what it is yet.

[Paweł Hajdan, Jr.]

+skia folks because this broke skia

+dnj because looks like chromeos was also affected

I've made an attempt to land all of this today (https://codereview.chromium.org/1919193002) but failed - the change has been reverted.

Some thoughts on this - please add yours.

1. The change was bigger and more dangerous than I initially expected. After seeing the result of today's attempt, I'd never roll it this way.

2. Andrii had good point about landing in chunks and preserving backward compatibility. That's my plan for the next attempt. It'll also help ensure recipe roller not being blocked.

3. The good thing is - we probably know everything that this may break.

4. Looks like skia builds were broken because of non-standard bot setup which doesn't have /b . I'd like to learn more about how skia bots are set up. Example failure: https://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_64-Release-CMake/builds/1691

5. As commented by Dan in https://codereview.chromium.org/1919193002#msg20 , looks like for some reason we generated wrong paths for chromeos (/cbuild instead of /b/cbuild). Since this apparently shows up in expectations (but was not noticed because of size of the patch), I'll make sure it's fixed.

6. Some build CQ failures were attributed to this change, which is a bit mysterious to me. Example failure: https://codereview.chromium.org/1914243002/#ps1 : https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20Test%20Trusty64/builds/1212 .

7. Do we have monitoring and alerting that'd allow us to detect elevated failure rate on any of the waterfalls we have? This change (which I'll make backward compatible and split) affects all builders using recipes. It doesn't seem feasible to watch of them manually, and I'd be looking for a way to ensure nothing is broken, and quickly detecting any breakage.

8. The recipes-on-swarming project I'm working on would allow testing a change like this before committing it. Landing this CL was part of making that possible.

WDYT?

Paweł

[Stephen Martinis]

Jam said this on the initial attempt to land this in https://codereview.chromium.org/1919193002, which I want to echo:

"

this also broke the infra cq. i.e. see https://codereview.chromium.org/1914243002/#ps1 why was this manually landed? i realize there are merge conflicts, but seems like a hinderance and a price to pay for large cl's

"

Looking at the second patchset, almost every tryjob is red, including the recipe try tests, like https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20Test%20Trusty64/builds/1201, which did break once this CL landed. 

This was also a pain to revert. Tandrii did some of the reverting, and I ended up doing the rest of it; recipes should finally be rolling ok.

Is there a postmortem on this? There are some good lessons learned and next steps we should take about how to revert cross repo recipe changes.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPY6JbM0TNooD0-BeRP6pqW%2BQVqC8ish6byN_h4w5-iEFQ%40mail.gmail.com.

[Paweł Hajdan, Jr.]

On Wed, Apr 27, 2016 at 12:02 AM, Stephen Martinis <mart...@chromium.org> wrote:

Jam said this on the initial attempt to land this in https://codereview.chromium.org/1919193002, which I want to echo:

"

this also broke the infra cq. i.e. see https://codereview.chromium.org/1914243002/#ps1 why was this manually landed? i realize there are merge conflicts, but seems like a hinderance and a price to pay for large cl's

"

Looking at the second patchset, almost every tryjob is red, including the recipe try tests, like https://build.chromium.org/p/tryserver.infra/builders/Build%20Try%20Recipe%20Test%20Trusty64/builds/1201, which did break once this CL landed. 

This was also a pain to revert. Tandrii did some of the reverting, and I ended up doing the rest of it; recipes should finally be rolling ok.

Is there a postmortem on this? There are some good lessons learned and next steps we should take about how to revert cross repo recipe changes.

Agreed. It was not a good idea to make the change this way. A postmortem is in progress.

I came up with what I believe is a much safer way to do this, completely backwards compatible: https://codereview.chromium.org/1923363003 , https://codereview.chromium.org/1926033002 . Please take a look and let me know what you think.

Paweł