Skip to content

buildelement: Add the digest-environment config property #1991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Kekun wants to merge 4 commits into from
Closed

buildelement: Add the digest-environment config property #1991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
75aafc6
_sandboxremote.py: stop trying to pull partial artifacts
abderrahim Jul 8, 2025
e5059e2
sandbox.py: add method to create a sub-sandbox
abderrahim Jul 9, 2025
d38563d
buildelement: Add the digest-environment config property
abderrahim Jun 17, 2025
614dc37
_sandboxremote.py: Upload blobs for subsandbox roots
abderrahim Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/buildstream/buildelement.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ def configure(self, node):
def configure_dependencies(self, dependencies):

self.__layout = {} # pylint: disable=attribute-defined-outside-init
self.__digest_environment = {} # pylint: disable=attribute-defined-outside-init

# FIXME: Currently this forcefully validates configurations
# for all BuildElement subclasses so they are unable to
Expand All @@ -227,9 +228,18 @@ def configure_dependencies(self, dependencies):
for dep in dependencies:
# Determine the location to stage each element, default is "/"
location = "/"

if dep.config:
dep.config.validate_keys(["location"])
location = dep.config.get_str("location")
dep.config.validate_keys(["digest-environment", "location"])

location = dep.config.get_str("location", "/")

digest_var_name = dep.config.get_str("digest-environment", None)

if digest_var_name is not None:
element_list = self.__digest_environment.setdefault(digest_var_name, [])
element_list.append(dep.element)

try:
element_list = self.__layout[location]
except KeyError:
Expand Down Expand Up @@ -285,10 +295,17 @@ def configure_sandbox(self, sandbox):
command_dir = build_root
sandbox.set_work_directory(command_dir)

def stage(self, sandbox):
# Setup environment
Copy link
Contributor

@abderrahim abderrahim Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to move this from configure_sandbox() to stage() because buildstream wouldn't let me stage dependency artifacts outside of stage()

sandbox.set_environment(self.get_environment())
env = self.get_environment()

def stage(self, sandbox):
for digest_variable, element_list in self.__digest_environment.items():
dummy_sandbox = sandbox.create_sub_sandbox()
self.stage_dependency_artifacts(dummy_sandbox, element_list)
Copy link
Contributor

@gtristan gtristan Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case that we are staging elements in element_list which don't depend on eachother, I don't believe that Element.stage_dependency_artifacts() will do any sorting.

In the case that we have resulting overlaps in the resulting staged trees, I am worried that the resulting CAS digest may differ depending on the order in which the dependencies are declared in the buildstream .bst element.

Also there is a concern that overlaps may not turn out to be the same as when staging the element's overall dependencies.

I think abderrahim also noted the sandbox staging seems a bit weird, or could be done nicer.

I don't believe it should be algorithmically necessary to do the actual staging in order to calculate what the resulting CAS digest would be, probably there should be a way to support this better in the CasBasedDirectory code without doing an actual staging process.

Copy link
Contributor

@abderrahim abderrahim Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case that we are staging elements in element_list which don't depend on eachother, I don't believe that Element.stage_dependency_artifacts() will do any sorting.

I find that statement weird given that stage_dependency_artifact() is advertised to do just that.

I would also argue that if this is true, then it is orthogonal to the changes in this pull request. There is no difference between

filename:
- a.bst
- b.bst
- c.bst
config:
  location: /sysroot

and

filename:
- a.bst
- b.bst
- c.bst
config:
  digest-environment: DEPENDENCIES_CAS_DIGEST

in the current code, except for the fact that the former stages in a subdirectory of the build sandbox and the latter stages in a dummy sandbox.

I don't believe it should be algorithmically necessary to do the actual staging in order to calculate what the resulting CAS digest would be, probably there should be a way to support this better in the CasBasedDirectory code without doing an actual staging process.

Well, it depends. Currently an element plugin has no way to create a CasBasedDirectory using public API. The only way to get a CasBasedDirectory is throgh Sandbox.get_virtual_directory() (and even that advertises it returns a Directory base class). It also can't get the artifact contents of an element as a Directory: it only has access to Element.stage_artifact(). While this BuildElement is in core and can technically use private APIs, I tried to avoid going that route.

I think the difference we need to consider between the two approaches is whether or not to consider the dependencies? i.e. if I depend on gcc, is your expectation that this only gets the digest of the gcc artifact or should it be gcc and its runtime dependencies?

Copy link
Contributor

@gtristan gtristan Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abderrahim writes:

In the case that we are staging elements in element_list which don't depend on eachother, I don't believe that Element.stage_dependency_artifacts() will do any sorting.

I find that statement weird given that stage_dependency_artifact() is advertised to do just that.

Right, this text could be clarified further, this function says:

"This is primarily a convenience wrapper around Element.stage_artifact() which takes care of staging all the dependencies in staging order and issueing the appropriate warnings."

Which is not false, it does stage all of the specified element and it does so in staging order, however if you look at what the code does, it does not do any toplevel sorting of the input element sequence.

I would also argue that if this is true, then it is orthogonal to the changes in this pull request. There is no difference between:

depends:
- a.bst
- b.bst
- c.bst
  config:
    location: /sysroot

and

depends:
- a.bst
- b.bst
- c.bst
  config:
    digest-environment: DEPENDENCIES_CAS_DIGEST

in the current code, except for the fact that the former stages in a subdirectory of the build sandbox and the latter stages in a dummy sandbox.

I think you are missing my meaning with your example.

So here is my hypothesis; given your a, b, c example, let us assume that the toplevel element e in question depends on b and c which may also depend on a, where b and c install the same file.

e.g.:

            e
          /   \
         b     c  <--- both "b" and "c" install /etc/abc.conf
          \   / 
            a

What I am saying, is that with this implementation, the value of DEPENDENCIES_CAS_DIGEST may differ in the following two configurations.

Configuration 1

Here we stage element e deterministically into the sandbox, and stage c's version of /etc/abc.conf over b's version of /etc/abc.conf in an orthogonal, extra stage operation which we use to determine the cas digest DEPENDENCIES_CAS_DIGEST.

depends:
- a.bst
- b.bst
  config:
    digest-environment: DEPENDENCIES_CAS_DIGEST
- c.bst
  config:
    digest-environment: DEPENDENCIES_CAS_DIGEST

Configuration 2

Here we do the same as configuration 1, except that b's version of /etc/abc.conf is staged on top of c's version of the same file, resulting in a different digest.

depends:
- a.bst
- c.bst
  config:
    digest-environment: DEPENDENCIES_CAS_DIGEST
- b.bst
  config:
    digest-environment: DEPENDENCIES_CAS_DIGEST

A closer look

The body of Element.stage_dependency_artifacts() is doing the following:

with self._overlap_collector.session(action, path):
    for dep in self.dependencies(selection):
        dep._stage_artifact(sandbox, path=path, include=include, exclude=exclude, orphans=orphans, owner=self)

And, looking at Element.dependencies(), we can quickly see that when the selection argument is given to select "Subsets of the dependency graph", the self element is not considered at all in the algorithm.

This means that the overall deterministic staging order of element e is not considered when looping over the dependencies of the selection argument given to Element.stage_dependency_artifacts().

Arguably this algorithm in Element.stage_dependency_artifacts() needs to be made more deterministic in this regard, although I wouldn't necessarily say that the docs are lying here - they are yielded in staging order relative to eachother, but they are not yielded in the staging order of the given element.

And so I come back to your original argument:

I would also argue that if this is true, then it is orthogonal to the changes in this pull request.

I have a point of contention here.

I have not yet looked at whether the location dependency configuration suffers from this issue as well, however, if we have this issue with the location dependency configuration, it needs to be fixed, and I also do not want to go as far as landing this change in the knowledge that this is indeed an issue.

Prove me wrong :)

There may be other ways that I am wrong about this bug, but I can think of one possibility:

It is possible that in the Element loading/instantiation process, that the dependency configurations are exposed to the plugin in staging order rather than in the order in which they are listed in the element declaration (.bst file)... if this is the case, Element.dependencies() may be sufficient for both of these use cases.

It would however be good to add a test with this merge request, asserting that we get the same CAS digest environment variable using both orderings of element e in my overlapping example above.

Copy link
Contributor

@abderrahim abderrahim Jun 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would however be good to add a test with this merge request, asserting that we get the same CAS digest environment variable using both orderings of element e in my overlapping example above.

I agree with this. I was just pointing out that if this behaviour is non-deterministic, then it's a pitfall that also applies to all plugins that make use of Element.stage_dependency_artifacts() and is not something specific to this use case.

Copy link
Contributor

@nanonyme nanonyme Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would however be good to add a test with this merge request, asserting that we get the same CAS digest environment variable using both orderings of element e in my overlapping example above.

I agree with this. I was just pointing out that if this behaviour is non-deterministic, then it's a pitfall that also applies to all plugins that make use of Element.stage_dependency_artifacts() and is not something specific to this use case.

Maybe even worth regarding this a bug (design flaw) that should have its own ticket.

digest = dummy_sandbox.get_virtual_directory()._get_digest()
Copy link
Contributor

@abderrahim abderrahim Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter complains here because _get_digest() is only defined for CasBasedDirectory (not the base Directory class) and get_virtual_directory() is advertised to return a Directory.

What would be the best way forward?

Copy link
Contributor

@nanonyme nanonyme Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would cast work if you know here that you're actually dealing with specific subtype?

env[digest_variable] = "{}/{}".format(digest.hash, digest.size_bytes)
Copy link
Contributor

@abderrahim abderrahim Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue I discovered when testing with remote execution: If using this with recc/bazel (#1751), for setting a remote execution platform property (e.g. chrootRootDigest) then we also need to upload it to the remote execution CAS.


sandbox.set_environment(env)

# First stage it all
#
Expand Down
25 changes: 11 additions & 14 deletions src/buildstream/sandbox/_sandboxremote.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from .._protos.build.bazel.remote.execution.v2 import remote_execution_pb2, remote_execution_pb2_grpc
from .._protos.build.buildgrid import local_cas_pb2
from .._protos.google.rpc import code_pb2
from .._exceptions import BstError, SandboxError
from .._exceptions import SandboxError
from .._protos.google.longrunning import operations_pb2, operations_pb2_grpc
from .._cas import CASRemote

Expand Down Expand Up @@ -247,9 +247,7 @@ def _execute_action(self, action, flags):
stdout, stderr = self._get_output()

context = self._get_context()
project = self._get_project()
cascache = context.get_cascache()
artifactcache = context.artifactcache

action_digest = cascache.add_object(buffer=action.SerializeToString())

Expand All @@ -263,21 +261,20 @@ def _execute_action(self, action, flags):
"Uploading input root", element_name=self._get_element_name()
):
# Determine blobs missing on remote
root_digests = [action.input_root_digest]

# Add virtual directories for subsandboxes
for subsandbox in self._get_subsandboxes():
vdir = subsandbox.get_virtual_directory()
root_digests.append(vdir._get_digest())

missing_blobs = []
try:
input_root_digest = action.input_root_digest
missing_blobs = list(cascache.missing_blobs_for_directory(input_root_digest, remote=casremote))
for root_digest in root_digests:
missing_blobs.extend(cascache.missing_blobs_for_directory(root_digest, remote=casremote))
except grpc.RpcError as e:
raise SandboxError("Failed to determine missing blobs: {}".format(e)) from e

# Check if any blobs are also missing locally (partial artifact)
# and pull them from the artifact cache.
try:
local_missing_blobs = cascache.missing_blobs(missing_blobs)
if local_missing_blobs:
artifactcache.fetch_missing_blobs(project, local_missing_blobs)
except (grpc.RpcError, BstError) as e:
raise SandboxError("Failed to pull missing blobs from artifact cache: {}".format(e)) from e

# Add command and action messages to blob list to push
missing_blobs.append(action.command_digest)
missing_blobs.append(action_digest)
Expand Down
28 changes: 27 additions & 1 deletion src/buildstream/sandbox/sandbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,18 @@ def __init__(self, context: "Context", project: "Project", **kwargs):
self.__env = None # type: Optional[Dict[str, str]]
self.__mount_sources = {} # type: Dict[str, str]
self.__allow_run = True
self.__subsandboxes = []

# Plugin element full name for logging
plugin = kwargs.get("plugin", None)
if plugin:
self.__element_name = plugin._get_full_name()
else:
self.__element_name = None
parent = kwargs.get("parent", None)
if parent:
self.__element_name = parent._get_element_name()
else:
self.__element_name = None

# Configuration from kwargs common to all subclasses
self.__config = kwargs["config"]
Expand Down Expand Up @@ -264,6 +269,24 @@ def batch(

batch.execute()

def create_subsandbox(self, **kwargs):
"""Create an empty sandbox

This allows an element to use a secondary sandbox for manipulating artifacts
that does not affect the build sandbox
"""

sub = Sandbox(
self.__context,
self.__project,
parent=self,
stdout=self.__stdout,
stderr=self.__stderr,
config=self.__config,
)
self.__subsandboxes.append(sub)
return sub

#####################################################
# Abstract Methods for Sandbox implementations #
#####################################################
Expand Down Expand Up @@ -565,6 +588,9 @@ def _get_element_name(self):
def _disable_run(self):
self.__allow_run = False

def _get_subsandboxes(self):
return self.__subsandboxes


# SandboxFlags()
#
Expand Down