vm-monitor: Remove dependency on workspace_hack

[sharnoff]

neondatabase/autoscaling builds libs/vm-monitor during CI because it's a necessary component of autoscaling.

workspace_hack includes a lot of crates that are not necessary for vm-monitor, which artificially inflates the build time on the autoscaling side, so hopefully removing the dependency should speed things up.

[github-actions]

2358 tests run: 2241 passed, 0 failed, 117 skipped (full report)

---

Flaky tests (3)

Postgres 16

• test_statvfs_pressure_usage: debug

Postgres 14

• test_branching_with_pgbench[flat-1-10]: debug
• test_eviction_across_generations: release

Code coverage (full report)

• functions: 53.3% (8690 of 16292 functions)
• lines: 81.4% (50162 of 61590 lines)

---

_{The comment gets automatically updated with the latest test results
5446f2f at 2023-11-06T02:46:49.730Z :recycle:}

[bayandin]

I'm not the best person to review such changes, I would appreciate a review from a real rust-person.

I've tried to build vm-monitor without openssl-dev (added in neondatabase/autoscaling#570), and it worked out, so I think it is worth doing.

[koivunej]

I am not fully up to date with the latest cargo resolution changes, workspace hack and what ignoring would mean. I am also not entirely sure why does the build within autoscaling end up doing so much work, because there is no artifact for workspace-hack. Trying to understand..

[koivunej]

Approval is dependent on you agreeing with my comment suggestion (not super critical to apply it but at least we need to agree with the contents).

Rephrasing my understanding:

• we want to keep vm-monitor in neondatabase/neon for our Cargo.lock (dep versions) and basic maintenance support like rust-1.72.0 changes #5255 or later
• we mostly (all only?) build vm-monitor in neondatabase/autoscaling
• there is zero interest in maintaining dependency feature parity because vm-monitor is built separatedly, that will only cost in dependency build time and linker needing to throw away most of the code

Could you confirm that this change will not make our build times suffer (as in dependencies needing to be built differently as part of our normal build?). If that would happen, then we would probably need some cfg or feature and use that to select when the vm-monitor is built with our workspace-hack and when not (absence of cfg or feature).

[sharnoff]

Wrote up a long reply, but honestly I think it's ultimately safer and more maintainable to go the cfg/feature route, so I'll see if that's possible (although, it might not be, depending on how configurable cargo hakari is).

Original reply

• we want to keep vm-monitor in neondatabase/neon for our Cargo.lock (dep versions) and basic maintenance support like rust-1.72.0 changes #5255 or later

More than that, we want to keep it here because it's fundamentally tied to the compute image, and having it in neondatabase/neon makes it much more feasible to

• we mostly (all only?) build vm-monitor in neondatabase/autoscaling

AFAIK, neondatabase/autoscaling is the only place that builds vm-monitor as a standalone binary. From within this repo, it's used as a library from compute_ctl. Not quite sure if that's what you mean

• there is zero interest in maintaining dependency feature parity because vm-monitor is built separatedly, that will only cost in dependency build time and linker needing to throw away most of the code

For building the standalone binary, that's exactly it, yeah. We still need feature parity during compute_ctl builds (or whatever else), maybe ?

Could you confirm that this change will not make our build times suffer (as in dependencies needing to be built differently as part of our normal build?)

To be honest, I'm unfamiliar with our existing usage - if you have specific recommendations, happy to check.

AFAICT, the places where we could hit issues here are if the features that vm-monitor uses are not a subset of what's used by workspace_hack, because then compute_ctl would have different builds than other components.

Currently, that's very unlikely to be true because all of the deps point to workspace.<name>, but in theory that may no longer be true in the future.

[sharnoff]

Ah ok actually, because this PR disables the vm-monitor dependency with final-excludes (rather than e.g. traversal-excludes), any additional features required by vm-monitor will still be included in workspace_hack, just not the other way around, which is what we want. (refer to the docs for more)

So, assuming we don't e.g. build all binary targets, then this should be the same speed building from this repo, and faster building just vm-monitor from elsewhere.

For some actual data, I compared the duration of the relevant "Build and Test" jobs for the latest commit on this branch vs its merge base with main (5952f35) ­— AFAICT it's not clearly worse, but there's perhaps too much noise to get a clear signal.

[conradludgate]

What hakari helps with is for feature resolution.

Example: you have a workspace with members foo and bar, which both depend on tokio.
foo uses tokio feature rt
bar uses tokio feature sync.

If you build them individually they will get only those features enabled, if you build the entire workspace you get both features and cargo will be forced to recompile tokio if you switch. (this can also be a source of obscure bugs if features enable behaviour)

[conradludgate]

I did some testing with cargo build followed by cargo build --bin <xyz> where xyz in [proxy, pageserver, safekeeper, vm-monitor] and I observed no rebuilds so it seems like we're not actively benefiting from our workspace hack crate right now

[koivunej]

@sharnoff replied:

More than that, we want to keep it here because it's fundamentally tied to the compute image, and having it in neondatabase/neon makes it much more feasible to

and:

AFAIK, neondatabase/autoscaling is the only place that builds vm-monitor as a standalone binary. From within this repo, it's used as a library from compute_ctl. Not quite sure if that's what you mean

Yeah apologies, I had forgotten library usage and the compute image building rendering my points moot. Good I wrote them I guess :)

For building the standalone binary, that's exactly it, yeah. We still need feature parity during compute_ctl builds (or whatever else), maybe ?

For library usage of vm_monitor it seems that it will always get the libraries and features selected by compute_ctl. So yeah, for me it seems it does not need to be part of the hakari trick if that even matters, per @conradludgate's findings.

On some of the rustc version bumps we did switch to use the later resolver which may have rendered the cargo-hakari trick useless, but at least I did not have time to read up on it again.

[sharnoff]

On some of the rustc version bumps we did switch to use the later resolver which may have rendered the cargo-hakari trick useless, but at least I did not have time to read up on it again.

From reading the cargo reference section on the resolver version 2, AFAICT it's an orthogonal change. My understanding is that the difference between v1 and v2 is that within a single build, v2 is more precise about which dependencies' features need to be enabled, and correctly recognizes a handful of cases that were false-positives for v1. But there's nothing in there about tracking dependencies from multiple bin targets.

[sharnoff]

If there's no remaining objections, I will merge this in ~24h.