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

Crucible dependencies on oximeter-producer doubling build times for several packages #1537

Open
smklein opened this issue Aug 3, 2022 · 3 comments
Assignees

Comments

@smklein
Copy link
Collaborator

smklein commented Aug 3, 2022

I believe this is the root cause of #1481 , but the issue also impacts build times. Filed as a separate issue to track reduction of duplicate dependencies in addition to the build flake.

The following command:

RUSTDOCFLAGS="-Dwarnings" cargo doc --workspace

Returns the following output:

warning: output filename collision.
The lib target `nexus-client` in package `nexus-client v0.1.0 (https://github.com/oxidecomputer/omicron?branch=main#c458fddc)` has the same output filename as the lib target `nexus-client` in package `nexus-client v0.1.0 (/home/smklein/repos/oxide/omicron/nexus-client)`.
Colliding filename is: /home/smklein/repos/oxide/omicron/target/doc/nexus_client/index.html                          
The targets should have unique names.                     
This is a known bug where multiple crates with the same name use                                                     
the same path; see <https://github.com/rust-lang/cargo/issues/6313>.                                                   
warning: output filename collision.                                                                                  
The lib target `omicron-common` in package `omicron-common v0.1.0 (https://github.com/oxidecomputer/omicron?branch=main#c458fddc)` has the same output filename as the lib target `omicron-common` in package `omicron-common v0.1.0 (/home/smklein/repos/oxide/omicron/common)`.
Colliding filename is: /home/smklein/repos/oxide/omicron/target/doc/omicron_common/index.html                          
The targets should have unique names.                                                                                
This is a known bug where multiple crates with the same name use                                                     
the same path; see <https://github.com/rust-lang/cargo/issues/6313>.                                                   
warning: output filename collision.                                                                                  
The lib target `oximeter` in package `oximeter v0.1.0 (https://github.com/oxidecomputer/omicron?branch=main#c458fddc)` has the same output filename as the lib target `oximeter` in package `oximeter v0.1.0 (/home/smklein/repos/oxide/omicron/oximeter/oximeter)`.
Colliding filename is: /home/smklein/repos/oxide/omicron/target/doc/oximeter/index.html                              
The targets should have unique names.                                                                                
This is a known bug where multiple crates with the same name use                                                       
the same path; see <https://github.com/rust-lang/cargo/issues/6313>.                                                 
warning: output filename collision.                                                                                  
The bin target `oximeter` in package `oximeter-collector v0.1.0 (/home/smklein/repos/oxide/omicron/oximeter/collector)` has the same output filename as the lib target `oximeter` in package `oximeter v0.1.0 (https://github.com/oxidecomputer/omicron?branch=main#c458fddc)`.
Colliding filename is: /home/smklein/repos/oxide/omicron/target/doc/oximeter/index.html                              
The targets should have unique names.
This is a known bug where multiple crates with the same name use                                                     
the same path; see <https://github.com/rust-lang/cargo/issues/6313>.                                                 
warning: output filename collision.
The lib target `oximeter-producer` in package `oximeter-producer v0.1.0 (https://github.com/oxidecomputer/omicron?branch=main#c458fddc)` has the same output filename as the lib target `oximeter-producer` in package `oximeter-producer v0.1.0 (/home/smklein/repos/oxide/omicron/oximeter/producer)`.
Colliding filename is: /home/smklein/repos/oxide/omicron/target/doc/oximeter_producer/index.html                     
The targets should have unique names.                                                                                  
This is a known bug where multiple crates with the same name use                                                       
the same path; see <https://github.com/rust-lang/cargo/issues/6313>.   

Inspecting the Cargo.lock file, we can see this is a valid complaint, tracing back to oximeter-producer.

In Cargo.lock, there are two [[package]]s for oximeter-producer:

  • One is pulled directly from the current tree, as 0.1.0. This is used by most of Omicron.
  • Another is pinned to the main branch. This appears to be used by crucible, which itself is used by propolis-client and the simulated sled agent.

The other mentioned targets also have duplicate [[package]]s, but I think that's because they're dependencies of oximeter-producer.

@smklein
Copy link
Collaborator Author

smklein commented Aug 3, 2022

This is the current dependency tree. Arrows mean "depends on".

crucible-deps (1)

The problem here is that we are building Crucible, and all transitive dependencies of Crucible within Omicron, just to use a couple structs.

If we clear out the dependencies on crucible in omicron (which transitively depend on oximeter-producer), I think we can solve this issue.

  • omicron-sled-agent depends on crucible::VolumeConstructionRequest in the simulated sled agent, to emulate a server.
  • propolis-client also depends on crucible::VolumeConstructionRequest

If we could pull these structures out of the main Crucible repo, we could produce the following:

crucible-deps (2)

This would no longer require Omicron to build Crucible, and would fix the warning from the docs generator.

@leftwo , @jgallagher , WDYT?

@leftwo
Copy link
Contributor

leftwo commented Aug 3, 2022

I'm just about to dig into the volume layer, so doing something like this (assuming it would work) would be good in term of timing for me.

@jgallagher
Copy link
Contributor

Yes, I'd vote for doing this. @andrewjstone and I ran into some issues when writing sprockets related to the current dependency tree, and suggested splitting out a lighter-weight crucible crate at the time (https://matrix.to/#/!sHZivBWjdefFxgKtLU:oxide.computer/$8ccUIZiqOZZJTqLhHgyy2aDfFiiCzsYkcWAMwTyWfyM?via=oxide.computer&via=unix.house&via=timecube.club is the beginning of that conversation). IIRC we worked around the issue we were having by tweaking some dependency versions instead of pursuing this, though.

@davepacheco davepacheco mentioned this issue Aug 3, 2022
5 tasks
@leftwo leftwo self-assigned this Aug 5, 2022
leftwo pushed a commit that referenced this issue Nov 19, 2024
No Propolis changes other than to update Crucible

Crucible changes are:
Add debug/timeout to test_memory.sh (#1563)
Consolidate ack checking (#1561)
Rename for crutest: RegionInfo -> DiskInfo (#1562)
Fix dtrace system level scripts (#1560)
Remove `ackable_work`; ack immediately instead (#1552)
No more New jobs, no more New jobs column (#1559)
Remove delay-based backpressure in favor of explicit queue limits (#1515)
Only send flushes when Downstairs is idle; send Barrier otherwise (#1505)
Update Rust crate reqwest to v0.12.9 (#1536)
Update Rust crate omicron-zone-package to 0.11.1 (#1535)
Remove separate validation array (#1522)
Remove more unnecessary `DsState` variants (#1550)
Consolidate `DownstairsClient::reinitialize` (#1549)
Update Rust crate uuid to v1.11.0 (#1546)
Update Rust crate reedline to 0.36.0 (#1544)
Update Rust crate bytes to v1.8.0 (#1541)
Update Rust crate thiserror to v1.0.66 (#1539)
Update Rust crate serde_json to v1.0.132 (#1538)
Update Rust crate serde to v1.0.214 (#1537)
Remove transient states in `DsState` (#1526)
Update Rust crate libc to v0.2.161 (#1534)
Update Rust crate futures to v0.3.31 (#1532)
Update Rust crate clap to v4.5.20 (#1531)
Update Rust crate async-trait to 0.1.83 (#1530)
Update Rust crate anyhow to v1.0.92 (#1529)
Remove obsolete crutest perf test (#1528)
Update dependency rust to v1.82.0 (#1512)
Still more updates to support Volume layer activities. (#1508)
Remove remaining IOPS/bandwidth limiting code (#1525)
Add unit test for VersionMismatch (#1524)
Removing panic paths by only destructuring once (#1523)
Update actions/checkout digest to 11bd719 (#1518)
Switch to using `Duration` for times (#1520)
leftwo added a commit that referenced this issue Nov 20, 2024
No Propolis changes other than to update Crucible

Crucible changes are:
Add debug/timeout to test_memory.sh (#1563)
Consolidate ack checking (#1561)
Rename for crutest: RegionInfo -> DiskInfo (#1562) Fix dtrace system
level scripts (#1560)
Remove `ackable_work`; ack immediately instead (#1552) No more New jobs,
no more New jobs column (#1559)
Remove delay-based backpressure in favor of explicit queue limits
(#1515) Only send flushes when Downstairs is idle; send Barrier
otherwise (#1505) Update Rust crate reqwest to v0.12.9 (#1536)
Update Rust crate omicron-zone-package to 0.11.1 (#1535) Remove separate
validation array (#1522)
Remove more unnecessary `DsState` variants (#1550) Consolidate
`DownstairsClient::reinitialize` (#1549) Update Rust crate uuid to
v1.11.0 (#1546)
Update Rust crate reedline to 0.36.0 (#1544)
Update Rust crate bytes to v1.8.0 (#1541)
Update Rust crate thiserror to v1.0.66 (#1539)
Update Rust crate serde_json to v1.0.132 (#1538)
Update Rust crate serde to v1.0.214 (#1537)
Remove transient states in `DsState` (#1526)
Update Rust crate libc to v0.2.161 (#1534)
Update Rust crate futures to v0.3.31 (#1532)
Update Rust crate clap to v4.5.20 (#1531)
Update Rust crate async-trait to 0.1.83 (#1530)
Update Rust crate anyhow to v1.0.92 (#1529)
Remove obsolete crutest perf test (#1528)
Update dependency rust to v1.82.0 (#1512)
Still more updates to support Volume layer activities. (#1508) Remove
remaining IOPS/bandwidth limiting code (#1525) Add unit test for
VersionMismatch (#1524)
Removing panic paths by only destructuring once (#1523) Update
actions/checkout digest to 11bd719 (#1518)
Switch to using `Duration` for times (#1520)

Co-authored-by: Alan Hanson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants