Skip to content
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.

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

Remote Execution: Symlinks created by ctx.actions.symlink are not represented as symlinks remotely #16070

Closed
castler opened this issue Aug 9, 2022 · 9 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug

Comments

@castler
Copy link

castler commented Aug 9, 2022

Description of the bug:

Using the remote execution capabilities of Bazel the behavior differs between a local and a remote execution.

When using ctx.actions.symlink() one would expect that the declared file is a symlink to some sort of source.
On a local execution this is the case. On a remote execution the declare file is directly represented by the symlinked file.

Thus, the content of the file is correct, but the semantics of the symlink are gone.
Build actions might rely on the semantics of a symlink and query, if a file is a symlink. This will lead to different results of local and remote executions which in worst case can cause a thread poisioning.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

A minimum reproducible example has been created:

https://github.com/castler/buildbarn_bazel_symlink_issue_repro#how-to-reproduce-the-issue

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

development version

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

Bazelisk using last_green.

Using commit: 6efc2ab

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

I first thought its a Remote Execution Service issue, which was analyzed here: buildbarn/bb-remote-execution#104

#11119 talks about that symlinks are not cached remotely. Maybe that has some similar root-cause.

#6547

666fce5 seemed related to me, but sadly did not fix the underlying problem.

Any other information, logs, or outputs that you want to share?

No response

@sgowroji sgowroji added type: bug untriaged team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Aug 9, 2022
@tjgq
Copy link
Contributor

tjgq commented Aug 9, 2022

ctx.actions.symlink actually does two completely different things, depending on whether the output argument is a ctx.actions.declare_symlink or a ctx.actions.declare_file. The first case indeed creates a symlink. The second case is akin to a file copy (I think it's unfortunate we decided to use the same API for both, but it's too late to change it now). This must be so because an action cannot retroactively change the semantics of a File object passed to it.

For the second form of ctx.actions.symlink, you are correct that local execution materializes the ctx.actions.symlink output as a symlink in blaze-bin, while remote execution materialize it as a regular file. However, Bazel will in both cases track the artifact internally as a regular file, not as a symlink; therefore no caches should be poisoned.

Finally, I should note that it's a bad idea for actions to be sensitive to whether an input is a file or a symlink to said file, since sandboxed execution (which works by executing the action inside a symlink tree) would affect their outcome.

@fmeum
Copy link
Collaborator

fmeum commented Aug 9, 2022

@castler If your actions really care about things being symlinks, you want to use --experimental_allow_unresolved_symlinks. Note that this flag doesn't properly work with RBE and runfiles yet - you can follow #10298 for the ongoing effort to fix that.

@tjgq
Copy link
Contributor

tjgq commented Aug 9, 2022

But --experimental_allow_unresolved_symlinks only applies to ctx.action.symlink with a ctx.actions.declare_symlink output argument, right? The provided repro is specifically about a ctx.actions.declare_file argument, which I believe is working as intended.

@fmeum
Copy link
Collaborator

fmeum commented Aug 9, 2022

@tjgq Yes, that's completely correct. I only wanted to draw attention to the fact that even ctx.actions.declare_symlink will not work as advertised, although it should be the right choice in the future when it has been stabilized.

@castler
Copy link
Author

castler commented Aug 9, 2022

@tjgq :
My understanding is that I use the experimental feature ctx.actions.declare_symlink if I want to allow dangling symlinks that are created by an action. In my case I neither want that the action creates the symlink, nor that it can be dangling. Thus, given the current documentation, I would decide in any case for the combination of ctx.actions.declare_file with ctx.actions.symlink (also if it would work with remote execution and it would be stable).

Is my understanding correct?

If so, I would like to understand this a little better:

For the second form of ctx.actions.symlink, you are correct that local execution materializes the ctx.actions.symlink output as a symlink in blaze-bin, while remote execution materialize it as a regular file.

Do we see this behavior as final? That's how I interpret your answer.

So basically I read that symlink with with declare_file() does not guarantee to create a symlink. If that is our understanding I would propose to document that in the API and then close this issue.

For future use-cases and guaranteed symlinks people then shall use declare_symlink with symlink.

Finally, I should note that it's a bad idea for actions to be sensitive to whether an input is a file or a symlink to said file, since sandboxed execution (which works by executing the action inside a symlink tree) would affect their outcome.

I agree with you, I anyhow understood from the current point of documentation, that a symlink creation is guaranteed.

@fmeum : Yes, thank you for the hint to #10298

@tjgq
Copy link
Contributor

tjgq commented Aug 9, 2022

@tjgq : My understanding is that I use the experimental feature ctx.actions.declare_symlink if I want to allow dangling symlinks that are created by an action. In my case I neither want that the action creates the symlink, nor that it can be dangling. Thus, given the current documentation, I would decide in any case for the combination of ctx.actions.declare_file with ctx.actions.symlink (also if it would work with remote execution and it would be stable).

Is my understanding correct?

ctx.actions.symlink with a ctx.actions.declare_symlink output can create both a dangling or a non-dangling symlink; whether it dangles depends on whether the target of the symlink exists. The only thing Bazel guarantees is that calling readlink on the symlink will return the target supplied to ctx.actions.symlink (irrespective of whether it was supplied as a File, via the target_file argument, or as a string, via the target_path argument). If an action were to receive as an input the symlink but not the file it points to, it would not be able to read the target file (under sandboxed conditions). This is why Bazel calls it an "unresolved" rather than a "dangling" symlink: Bazel won't attempt to resolve it, but that doesn't necessarily mean it won't resolve successfully.

By contrast, when you call ctx.actions.symlink with a ctx.actions.declare_file output, Bazel guarantees that an action can observe the contents of the file at the other end of the symlink, even when the declare_file is the only input. The way I see it, the symlink is just an implementation detail; making a copy of the original file would be equivalent, albeit less efficient.

If so, I would like to understand this a little better:

For the second form of ctx.actions.symlink, you are correct that local execution materializes the ctx.actions.symlink output as a symlink in blaze-bin, while remote execution materialize it as a regular file.

Do we see this behavior as final? That's how I interpret your answer.

I don't have a strong opinion. One could argue that making the contents of bazel-bin dependent on the execution strategy is a bug. But Bazel already uses symlinks to stand in for real files in other places (again, consider sandboxed execution) so one could also say that the distinction between a symlink and the file it points to is not to be relied upon. I lean towards the latter position.

An important point I want to stress again, though, is that the contents of bazel-bin do not necessarily reflect Bazel's internal state; a declare_file will be tracked internally as a regular file, irrespective of whether it materializes as a symlink.

So basically I read that symlink with with declare_file() does not guarantee to create a symlink. If that is our understanding I would propose to document that in the API and then close this issue.

Yes, I agree that the documentation is unclear; I was confused myself until it was pointed out to me that ctx.actions.symlink does two different things. I will send a PR to improve it.

For future use-cases and guaranteed symlinks people then shall use declare_symlink with symlink.

I agree, although sadly you don't currently have that option if you wish to use remote execution (due to #10298).

Finally, I should note that it's a bad idea for actions to be sensitive to whether an input is a file or a symlink to said file, since sandboxed execution (which works by executing the action inside a symlink tree) would affect their outcome.

I agree with you, I anyhow understood from the current point of documentation, that a symlink creation is guaranteed.

@fmeum : Yes, thank you for the hint to #10298

@Yannic
Copy link
Contributor

Yannic commented Aug 9, 2022

@tjgq
Copy link
Contributor

tjgq commented Oct 20, 2022

I've just submitted 32b0f5a, which will cause non-symlink outputs created via ctx.actions.symlink (i.e., with a target_file parameter) to be materialized on the local filesystem as symlinks when --remote_downloads_minimal is enabled. This should avoid duplicate downloads of the same object when multiple symlink target it. (Except possibly on Windows, of course - I haven't had a chance to look into it yet.)

copybara-service bot pushed a commit that referenced this issue Oct 25, 2022
This API does two different things and users occasionally get confused by it (see e.g. #16070). In particular, it is crucial to understand whether Bazel tracks the symlink on its own or the contents of its target.

PiperOrigin-RevId: 483604065
Change-Id: Iab14c264eb320cd324b64b3af2a400df948dbf3c
tjgq added a commit that referenced this issue Oct 25, 2022
This API does two different things and users occasionally get confused by it (see e.g. #16070). In particular, it is crucial to understand whether Bazel tracks the symlink on its own or the contents of its target.

PiperOrigin-RevId: 483604065
Change-Id: Iab14c264eb320cd324b64b3af2a400df948dbf3c
tjgq added a commit to tjgq/bazel that referenced this issue Oct 26, 2022
This API does two different things and users occasionally get confused by it (see e.g. bazelbuild#16070). In particular, it is crucial to understand whether Bazel tracks the symlink on its own or the contents of its target.

PiperOrigin-RevId: 483604065
Change-Id: Iab14c264eb320cd324b64b3af2a400df948dbf3c
ShreeM01 added a commit that referenced this issue Oct 26, 2022
This API does two different things and users occasionally get confused by it (see e.g. #16070). In particular, it is crucial to understand whether Bazel tracks the symlink on its own or the contents of its target.

PiperOrigin-RevId: 483604065
Change-Id: Iab14c264eb320cd324b64b3af2a400df948dbf3c

Co-authored-by: kshyanashree <[email protected]>
@coeuvre coeuvre added the P2 We'll consider working on this in future. (Assignee optional) label Nov 22, 2022
@tjgq
Copy link
Contributor

tjgq commented Jun 13, 2023

Re-reading the discussion, I don't believe there's anything left to do here, but let me know if you think otherwise.

@tjgq tjgq closed this as completed Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug
Projects
None yet
Development

No branches or pull requests

7 participants