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

Jump to bottom

Serve gzipped version of console asset if it exists #1345

Merged

david-crespo merged 5 commits into main from gzipped-assets

Jul 5, 2022

Contributor

david-crespo commented Jul 1, 2022 •

edited

Loading

gzip takes our biggest console bundle from 860 KiB to like 250, i.e., from too big to fine. So it’s pretty valuable for initial uncached pageload time.

This PR is the Nexus side of oxidecomputer/console#1031. These PRs can be merged independently because Nexus works exactly the same as it did before if there are no .gz files in the assets directory, and if there are .gz files in there before this is merged they will be ignored.

I still believe we will want on the fly compression for large response bodies (oxidecomputer/dropshot#221), but even if we had that, for console assets we can avoid the runtime cost because they are pre-built anyway.

It works!


          serve gzipped version of asset if it exists

053b425

david-crespo commented

View reviewed changes

nexus/src/external_api/console_api.rs Outdated

+                  let (file, gzipped) = match find_file(path_gz, &assets_dir) {
+                      Ok(file) => (file, true),
+                      _ => (find_file(path, &assets_dir)?, false),
+                  };

Contributor Author

david-crespo Jul 1, 2022

this is either the best or worst 3 lines I've ever written

Contributor

ahl Jul 5, 2022

best

david-crespo commented

View reviewed changes

nexus/src/external_api/console_api.rs

+                  if gzipped {
+                      resp = resp.header(http::header::CONTENT_ENCODING, "gzip");
+                  }

Contributor Author

david-crespo Jul 1, 2022

Is this the way to conditionally add a header?

david-crespo commented

View reviewed changes

nexus/src/external_api/console_api.rs

-                  if !file_ext_allowed(&current) {
-                      return Err(not_found("file extension not allowed"));
-                  }

Contributor Author

david-crespo Jul 1, 2022

Moved this check out of find_file because it seemed silly to have to pop off .gz to do it. I also tried adding things like js.gz to the list of allowed extensions, but extension() only gives us the .gz. We could make it work by looping through the list and using some kind ends_with thing.

david-crespo commented

View reviewed changes

nexus/src/external_api/console_api.rs

-                          .unwrap_err();
-                      assert_eq!(error.status_code, StatusCode::NOT_FOUND);
-                      assert_eq!(error.internal_message, "file extension not allowed");
-                  }

Contributor Author

david-crespo Jul 1, 2022 •

edited

Loading

This behavior has been moved out of find_file and there's already an integration test for it at the endpoint level.

Contributor Author

david-crespo commented Jul 1, 2022 •

edited

Loading

Probably need to condition this on the accept-encoding request header.


          fix clippy, only send gzip if accept-encoding header says to

0eee3a3

david-crespo force-pushed the gzipped-assets branch from 9f48b8f to 0eee3a3 Compare

July 2, 2022 04:45


          don't bother finding gzipped file if we're not going to serve it anyway

david-crespo mentioned this pull request

Include gzipped version of assets in console tarball oxidecomputer/console#1031

Merged


          code golf, thanks @jgallagher

78dc3e4

david-crespo requested review from ahl and zephraph

July 5, 2022 15:52

david-crespo commented

View reviewed changes

nexus/src/external_api/console_api.rs

+                  let last = new_path.pop().unwrap(); // we know there's one there
+                  new_path.push(format!("{last}.gz"));
+                  new_path
+              }

This comment was marked as resolved.

Sign in to view

ahl approved these changes

View reviewed changes

nexus/src/external_api/console_api.rs Outdated

+                  let (file, gzipped) = match find_file(path_gz, &assets_dir) {
+                      Ok(file) => (file, true),
+                      _ => (find_file(path, &assets_dir)?, false),
+                  };

Contributor

ahl Jul 5, 2022

best


          debatably simplify by passing around PathBuf instead of Vec<String>

4782e84

david-crespo enabled auto-merge (squash)

July 5, 2022 17:49

david-crespo merged commit 600c2c9 into main

david-crespo deleted the gzipped-assets branch

July 5, 2022 18:16

david-crespo mentioned this pull request

Serve compressed console assets oxidecomputer/console#946

Closed

leftwo pushed a commit that referenced this pull request


          Update crucible, add DTrace crucible to host image

3a09cb9

Added a new package, crucible-dtrace that pulls from buildomat a package
that contains a set of DTrace scripts.  These scripts are extracted into
the global zone at /opt/oxide/crucible_dtrace/

Update Crucible to latest includes these updates:
Clean up dependency checking, fixing space leak (#1372)
Make a DTrace package (#1367)
Use a single context in all messages (#1363)
Remove `DownstairsWork`, because it's redundant (#1371)
Remove `WorkState`, because it's implicit (#1370)
Do work immediately upon receipt of a job, if possible (#1366)
Move 'do work for one job' into a helper function (#1365)
Remove `DownstairsWork` from map when handling it (#1361)
Using `block_in_place` for IO operations (#1357)
update omicron deps; use re-exported dropshot types in oximeter-producer configuration (#1369)
Parameterize more tests (#1364)
Misc cleanup, remove sqlite references. (#1360)
Fix `Extent::close` docstring (#1359)
Make many `Region` functions synchronous (#1356)
Remove `Workstate::Done` (unused) (#1355)
Return a sorted `VecDeque` directly (#1354)
Combine `proc_frame` and `do_work_for` (#1351)
Move `do_work_for` and `do_work` into `ActiveConnection` (#1350)
Support arbitrary Volumes during replace compare (#1349)
Remove the SQLite backend (#1352)
Add a custom timeout for buildomat tests (#1344)
Move `proc_frame` into `ActiveConnection` (#1348)
Remove `UpstairsConnection` from `DownstairsWork` (#1341)
Move Work into ConnectionState (#1340)
Make `ConnectionState` an enum type (#1339)
Parameterize `test_repair.sh` directories (#1345)
Remove `Arc<Mutex<Downstairs>>` (#1338)
Send message to Downstairs directly (#1336)
Consolidate `on_disconnected` and `remove_connection` (#1333)
Move disconnect logic to the Downstairs (#1332)
Remove invalid DTrace probes. (#1335)
Fix outdated comments (#1331)
Use message passing when a new connection starts (#1330)
Move cancellation into Downstairs, using a token to kill IO tasks (#1329)
Make the Downstairs own per-connection state (#1328)
Move remaining local state into a `struct ConnectionState` (#1327)
Consolidate negotiation + IO operations into one loop (#1322)
Allow replacement of a target in a read_only_parent (#1281)
Do all IO through IO tasks (#1321)
Make `reqwest_client` only present if it's used (#1326)
Move negotiation into Downstairs as well (#1320)
Update Rust crate clap to v4.5.4 (#1301)
Reuse a reqwest client when creating Nexus clients (#1317)
Reuse a reqwest client when creating repair client (#1324)
Add % to keep buildomat happy (#1323)
Downstairs task cleanup (#1313)
Update crutest replace test, and mismatch printing. (#1314)
Added more DTrace scripts. (#1309)
Update Rust crate async-trait to 0.1.80 (#1298)

leftwo mentioned this pull request

Update crucible, add DTrace crucible to host image #5954

Merged

leftwo added a commit that referenced this pull request


          Update crucible, add DTrace crucible to host image (#5954)

6e29409

Update Crucible and Propolis to the latest

Added a new package, crucible-dtrace that pulls from buildomat a package
that contains a set of DTrace scripts. These scripts are extracted into the 
global zone at /opt/oxide/crucible_dtrace/

Crucible latest includes these updates:
Clean up dependency checking, fixing space leak (#1372) Make a DTrace
package (#1367)
Use a single context in all messages (#1363)
Remove `DownstairsWork`, because it's redundant (#1371) Remove
`WorkState`, because it's implicit (#1370)
Do work immediately upon receipt of a job, if possible (#1366) Move 'do
work for one job' into a helper function (#1365) Remove `DownstairsWork`
from map when handling it (#1361) Using `block_in_place` for IO
operations (#1357)
update omicron deps; use re-exported dropshot types in oximeter-producer
configuration (#1369) Parameterize more tests (#1364)
Misc cleanup, remove sqlite references. (#1360)
Fix `Extent::close` docstring (#1359)
Make many `Region` functions synchronous (#1356)
Remove `Workstate::Done` (unused) (#1355)
Return a sorted `VecDeque` directly (#1354)
Combine `proc_frame` and `do_work_for` (#1351)
Move `do_work_for` and `do_work` into `ActiveConnection` (#1350) Support
arbitrary Volumes during replace compare (#1349) Remove the SQLite
backend (#1352)
Add a custom timeout for buildomat tests (#1344)
Move `proc_frame` into `ActiveConnection` (#1348)
Remove `UpstairsConnection` from `DownstairsWork` (#1341) Move Work into
ConnectionState (#1340)
Make `ConnectionState` an enum type (#1339)
Parameterize `test_repair.sh` directories (#1345)
Remove `Arc<Mutex<Downstairs>>` (#1338)
Send message to Downstairs directly (#1336)
Consolidate `on_disconnected` and `remove_connection` (#1333) Move
disconnect logic to the Downstairs (#1332)
Remove invalid DTrace probes. (#1335)
Fix outdated comments (#1331)
Use message passing when a new connection starts (#1330) Move
cancellation into Downstairs, using a token to kill IO tasks (#1329)
Make the Downstairs own per-connection state (#1328) Move remaining
local state into a `struct ConnectionState` (#1327) Consolidate
negotiation + IO operations into one loop (#1322) Allow replacement of a
target in a read_only_parent (#1281) Do all IO through IO tasks (#1321)
Make `reqwest_client` only present if it's used (#1326) Move negotiation
into Downstairs as well (#1320)
Update Rust crate clap to v4.5.4 (#1301)
Reuse a reqwest client when creating Nexus clients (#1317) Reuse a
reqwest client when creating repair client (#1324) Add % to keep
buildomat happy (#1323)
Downstairs task cleanup (#1313)
Update crutest replace test, and mismatch printing. (#1314) Added more
DTrace scripts. (#1309)
Update Rust crate async-trait to 0.1.80 (#1298)

Propolis just has this one update:
Allow boot order config in propolis-standalone
---------

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