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

PVF: Add back socket path parameter, use tmp socket path #1780

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

mrcnski
Copy link
Contributor

@mrcnski mrcnski commented Oct 3, 2023

Problem

Running zombienet locally:

WARN tokio-runtime-worker parachain::pvf: cannot bind unix socket: Custom { kind: InvalidInput, 
error: "path must be shorter than libc::sockaddr_un.sun_path" } 
debug_id=prepare program_path="/home/alexggh/repos/polkadot-sdk/target/testnet/polkadot-prepare-worker" 
extra_args=["prepare-worker", "--node-impl-version", "1.1.0"] 
worker_dir=WorkerDir { path: "/tmp/zombie-a609ef68a12d8517c8174b62ee3a0899_-484651-6nFd48QbCHfO/alice/data/chains/rococo_local_testnet/db/full/pvf-artifacts/worker-dir-prepare-2Q4B1v7qio" } 
socket_path="/tmp/zombie-a609ef68a12d8517c8174b62ee3a0899_-484651-6nFd48QbCHfO/alice/data/chains/rococo_local_testnet/db/full/pvf-artifacts/worker-dir-prepare-2Q4B1v7qio/socket"

This is because by default zombienet decides to prefix things with /tmp/zombie-a609ef68a12d8517c8174b62ee3a0899_-484651-6nFd48QbCHfO and that gets us beyond the socket limitation size.

Fix

Looks like the socket path limit is 108 bytes. This should definitely be fixed, users are allowed to use an arbitrary cache path. There is no need for the socket to be under the cache path though, we can revert to using a tmp location like we were before.

Related

Closes #1765
This was introduced with #1373

Looks like [the socket path limit is 108
bytes](https://man7.org/linux/man-pages/man7/unix.7.html). This should
definitely be fixed, users are allowed to use an arbitrary cache path. There may
be no need for the socket to be under the cache path though, I think we can
revert to using a tmp location like we were before.

Closes #1765
@mrcnski mrcnski added the T0-node This PR/Issue is related to the topic “node”. label Oct 3, 2023
@mrcnski mrcnski self-assigned this Oct 3, 2023
Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

LGTM, minus the failing test

@s0me0ne-unkn0wn
Copy link
Contributor

@pepoviola what's the rationale behind having that long directory suffixes in the first place?

@s0me0ne-unkn0wn
Copy link
Contributor

@mrcnski strictly speaking, the proposed fix fixes the case, but not the entire problem. That weird sun_path limit is the reason why it is a good practice to put socket files under /run, /var/run, or directly under /tmp. Is there any reason why shouldn't we follow that practice?

Ah yes, we're running as non-root, and /run is not guaranteed to be writeable. But why not just /tmp then?

Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

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

LGTM as a quick fix, but probably better approach should be considered in the future.

@pepoviola
Copy link
Contributor

@pepoviola what's the rationale behind having that long directory suffixes in the first place?

The idea was to have some pattern like namespace_random but I think we can shrink to only use the namespace that will be already pretty unique. I will open an issue to track this.
Thanks!

@mrcnski
Copy link
Contributor Author

mrcnski commented Oct 5, 2023

@mrcnski strictly speaking, the proposed fix fixes the case, but not the entire problem. That weird sun_path limit is the reason why it is a good practice to put socket files under /run, /var/run, or directly under /tmp. Is there any reason why shouldn't we follow that practice?

Ah yes, we're running as non-root, and /run is not guaranteed to be writeable. But why not just /tmp then?

We're using std::env::temp_dir (in with_transient_socket_path we call tmppath which calls temp_dir), which should return /tmp:

On Unix, returns the value of the TMPDIR environment variable if it is set, otherwise for non-Android it returns /tmp.

@mrcnski mrcnski enabled auto-merge (squash) October 5, 2023 15:50
@mrcnski mrcnski merged commit 51c0c24 into master Oct 5, 2023
104 of 106 checks passed
@mrcnski mrcnski deleted the mrcnski/pvf-tmp-socket-path branch October 5, 2023 16:37
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 26, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Mar 27, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
serban300 added a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* Add integrity check for signed extensions

* Remove unneeded type specification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parachains zombienet fails on local machines
5 participants