search

apache / buildstream

BuildStream, the software integration tool
https://buildstream.build/
Apache License 2.0
83 stars 28 forks source link

set_ref not properly globally reflecting to node state changes #1642

Open nanonyme opened 2 years ago

nanonyme commented 2 years ago

Noticed while using cargo plugin with pypi plugin (which does not need to fetch sources on track). Using cargo plugin with any plugin where tracking results in source fetch hides the bug.

Sequence goes on the ballpark

  1. configure gets called for first source, reads data from node
  2. track gets called for first source, resolves data for update
  3. set_ref gets called for first source, writes data to node
  4. track gets called for second source, requests fetch for previous element source
  5. configure gets called again for first source (apparently with different plugin instance), reads data from node but this is not the data from previous tracking attempt but stale data
  6. tracking for second element fails because correct data for first source is not in cache
gtristan commented 2 years ago

After discussing this on slack for a while and examining the code, we've pretty much determined that the issue here is with the pypi plugin.

Notice that in Source.fetch(): https://gitlab.com/BuildStream/bst-external/-/blob/master/bst_external/sources/pypi.py#L170 we directly use self.url, but we do not update self.url in Source.set_ref()...

BuildStream will handover the result of Source.track() to the source via Source.set_ref() and then expect that source to fetch the updated ref in Source.fetch().

As a side note, the plugin should probable raise an error around https://gitlab.com/BuildStream/bst-external/-/blob/master/bst_external/sources/pypi.py#L190 in the case that the downloaded tarball does not match the expected sha256sum.

nanonyme commented 2 years ago

Everything looks now ok. Plugin is rewritten through https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/merge_requests/8316. It was a clear bug with assumptions in the plugin about how translated URL's work.

nanonyme commented 2 years ago

So API and cache keys were broken as a result but this seems unavoidable and maybe it's not a huge problem since it was not done in a stable release of freedesktop-sdk.

nanonyme commented 2 years ago

I'm re-opening this. I'm not convinced that it's only the plugin which was being weird. Tested locally

--:--:--][????????][track:components/python3-cryptography.bst] INFO    Found new revision: {'sha256sum': 'd610d0ee14dd9109006215c7c0de15eee91230b70a9bce2263461cf7c3720b83', 'suffix': '3d/5f/addb8b91fd356792d28e59a8275fec833323cb28604fb3a497c35d7cf0a3/cryptography-37.0.1.tar.gz'}, old ref{'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}
    Exception: {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7ffa17b6be80>, 'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}
nanonyme commented 2 years ago

This still seems like node handling in bst1 is not handled correctly in dependency cases. BuildStream does here call set_ref as expected but it does not take impact deep enough. Tracking works correctly when dependencies are not used.

nanonyme commented 2 years ago

Looks like indeed this is a state propagation issue with multiple processes. Track results in 

[--:--:--][        ][ main:cryptography                  ] INFO    6002: 140571381474512 ChainMap({'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd953422470>, 'name': 'cryptography', 'ref': {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd9534222c0>, 'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}}, {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd953d15810>, 'url': 'pypi:'})
[--:--:--][????????][track:components/python3-cryptography.bst] INFO    6007: 140571367322096 ChainMap({'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd9526a1060>, 'name': 'cryptography', 'ref': {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd9526a3100>, 'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}}, {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd953d15810>, 'url': 'pypi:'})
[--:--:--][????????][track:components/python3-cryptography.bst] INFO    Found new revision: {'sha256sum': 'd610d0ee14dd9109006215c7c0de15eee91230b70a9bce2263461cf7c3720b83', 'suffix': '3d/5f/addb8b91fd356792d28e59a8275fec833323cb28604fb3a497c35d7cf0a3/cryptography-37.0.1.tar.gz'}, old ref{'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}
[--:--:--][????????][track:components/python3-cryptography.bst] INFO    6007: 140571367849504 ChainMap({'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd952723e50>, 'name': 'cryptography', 'ref': {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd952723f40>, 'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}}, {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7fd953d15810>, 'url': 'pypi:'})
nanonyme commented 2 years ago

The first integer is PID, the second is object id since I wanted to see for sure that we're dealing with different node objects. We are.

nanonyme commented 2 years ago

Some more log output

[--:--:--][        ][ main:cryptography                  ] INFO    configure 6823: ChainMap({'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f972ac3e470>, 'name': 'cryptography', 'ref': {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f972ac3e2c0>, 'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}}, {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f972b551810>, 'url': 'pypi:'})
[--:--:--][????????][track:components/python3-cryptography.bst] INFO    configure 6828: ChainMap({'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f9729eb71c0>, 'name': 'cryptography', 'ref': {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f9729eb7100>, 'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}}, {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f972b551810>, 'url': 'pypi:'})
[--:--:--][????????][track:components/python3-cryptography.bst] INFO    Found new revision: {'sha256sum': 'd610d0ee14dd9109006215c7c0de15eee91230b70a9bce2263461cf7c3720b83', 'suffix': '3d/5f/addb8b91fd356792d28e59a8275fec833323cb28604fb3a497c35d7cf0a3/cryptography-37.0.1.tar.gz'}, old ref{'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}
[--:--:--][????????][track:components/python3-cryptography.bst] INFO    set_ref 6828: ordereddict([('kind', 'pypi'), ('name', 'cryptography'), ('ref', ordereddict([('sha256sum', '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9'), ('suffix', '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz')]))])
[--:--:--][????????][track:components/python3-cryptography.bst] INFO    configure 6828: ChainMap({'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f9729f37dc0>, 'name': 'cryptography', 'ref': {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f9729f37d90>, 'sha256sum': '70f8f4f7bb2ac9f340655cbac89d68c527af5bb4387522a8413e841e3e6628c9', 'suffix': '10/a7/51953e73828deef2b58ba1604de9167843ee9cd4185d8aaffcb45dd1932d/cryptography-36.0.2.tar.gz'}}, {'__bst_provenance_info': <buildstream._yaml.DictProvenance object at 0x7f972b551810>, 'url': 'pypi:'})
nanonyme commented 2 years ago

So set_ref is clearly called in track process, not main process.

gtristan commented 2 years ago

So set_ref is clearly called in track process, not main process.

This is probably true (and wouldn't be an issue in bst2 due to the new threaded scheduler)... however it's questionable whether this causes an issue (or how this causes any issue).

Note that processing of track or fetch happens on a per element granularity, and when a cargo source or pypi source ensures that a previous element is fetched and staged before performing it's own track operation - that will all happen in the context of the same process.

Are we talking about a situation where we're running bst build --track in bst-1 ?

I'm confused again as to what exactly is going wrong.

benjamb commented 2 years ago

You can reproduce the issue with https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/commit/62f478ff0414a590f1b3fb4cb4370dde1ab76612

And running bst track components/python3-cryptography.bst.

nanonyme commented 2 years ago

It doesn't help it happens in some process, something is resetting state

nanonyme commented 2 years ago

Tested wtih bst2, looks it is also affected

[--:--:--] START   [????????] components/python3-cryptography.bst: Track
[--:--:--] INFO    components/python3-cryptography.bst: Found new revision: {'sha256sum': 'f224ad253cc9cea7568f49077007d2263efa57396a2f2f78114066fd54b5c68e', 'suffix': '51/05/bb2b681f6a77276fc423d04187c39dafdb65b799c8d87b62ca82659f9ead/cryptography-37.0.2.tar.gz'}
[--:--:--] BUG     [????????] components/python3-cryptography.bst: Track

    An unhandled exception occured:

    Traceback (most recent call last):
      File "/usr/lib/python3.9/site-packages/buildstream/_scheduler/jobs/job.py", line 441, in child_action
        result = self.child_process()  # pylint: disable=assignment-from-no-return
      File "/usr/lib/python3.9/site-packages/buildstream/_scheduler/jobs/elementjob.py", line 92, in child_process
        return self._action_cb(self._element)
      File "/usr/lib/python3.9/site-packages/buildstream/_scheduler/queues/trackqueue.py", line 67, in _track_element
        return element._track()
      File "/usr/lib/python3.9/site-packages/buildstream/element.py", line 1363, in _track
        return self.__sources.track(self._get_workspace())
      File "/usr/lib/python3.9/site-packages/buildstream/_elementsources.py", line 97, in track
        with self._stage_previous_sources(source) as staging_directory:
      File "/usr/lib/python3.9/contextlib.py", line 119, in __enter__
        return next(self.gen)
      File "/usr/lib/python3.9/site-packages/buildstream/_elementsources.py", line 496, in _stage_previous_sources
        self.fetch_sources(stop=source)
      File "/usr/lib/python3.9/site-packages/buildstream/_elementsources.py", line 254, in fetch_sources
        self._fetch_source(source)
      File "/usr/lib/python3.9/site-packages/buildstream/_elementsources.py", line 435, in _fetch_source
        source._fetch()
      File "/usr/lib/python3.9/site-packages/buildstream/source.py", line 853, in _fetch
        self.__do_fetch()
      File "/usr/lib/python3.9/site-packages/buildstream/source.py", line 1346, in __do_fetch
        new_source.fetch(**kwargs)
      File "/var/home/nanonyme/git/freedesktop-sdk/.bst/staged-junctions/plugins/bst-plugins-experimental.bst/a6bed3e8fa5fcb4c123fb35efe6948f5f5b49fd05dda2789209cc57090a6a76e/src/bst_plugins_experimental/sources/pypi.py", line 197, in fetch
        default_name = os.path.basename(self.url)
      File "/var/home/nanonyme/git/freedesktop-sdk/.bst/staged-junctions/plugins/bst-plugins-experimental.bst/a6bed3e8fa5fcb4c123fb35efe6948f5f5b49fd05dda2789209cc57090a6a76e/src/bst_plugins_experimental/sources/pypi.py", line 95, in url
        self.ref["suffix"],
    TypeError: 'NoneType' object is not subscriptable

Looks like the ref resolved through track is not used when tracking next source which depends on first one.

abderrahim commented 2 years ago

So I took a dive trying to debug this. The problem seems to be that Source.__clone_for_uri() doesn't satisfy this invariant (which seems wrong for something called clone)

new_source = self.__clone_for_uri(uri)
assert new_source.get_ref() == self.get_ref()

Looking deeper, it looks like __clone_for_uri() relies on a field called __meta, defined along with the following comments.

        # FIXME: Reconstruct a MetaSource from a Source instead of storing it.
        self.__meta = meta                              # MetaSource stored so we can copy this source later.

The problem is that this __meta doesn't get updated when a new ref is set via set_ref().

Incidentally, I think that this will only happen when using inline refs and not when using a project.refs file, as the refs wouldn't be taken from the meta source in that case.

nanonyme commented 2 years ago

I did try to replicate the logic from bst2. But I don't see any evidence that reconstructing MetaSource actually changes anything. In BuildStream it's constructed something like https://github.com/apache/buildstream/blob/master/src/buildstream/source.py#L1263-L1271

nanonyme commented 2 years ago

It uses self.__config as input but I don't see any sign of _set_ref manipulating it. Especially in tracking where there is no save to disk.

nanonyme commented 2 years ago

Another interesting information point here is https://gitlab.com/BuildStream/bst-external/-/merge_requests/178 seems to work. The difference between these plugins is git2 uses source fetches where pypi plugin does not. It is recommended single source plugins do not. Or then I again spoiled my test results with cached sources...

nanonyme commented 2 years ago

Right. I refactored pypi source plugin to use a source fetcher through https://gitlab.com/BuildStream/bst-external/-/merge_requests/181 and the problem vanished immediately

nanonyme commented 2 years ago

@gtristan so I'm not entirely convinced plugins that do not use source fetchers work correctly with previous source fetch. Plugins that use most likely are fine. Documentation claims using these is optional.

nanonyme commented 1 year ago

Removed prefix since same problem applies both for bst1 and bst2.