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

[gateway] update MGS Git dependency #7924

Merged
merged 9 commits into from
Apr 8, 2025
Merged

[gateway] update MGS Git dependency #7924

merged 9 commits into from
Apr 8, 2025

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 4, 2025

It turns out that our Git dependency on the
oxidecomputer/management-gateway-service repo hasn't been updated in... a while. We're currently on a commit from September of last year, oxidecomputer/management-gateway-service@9bbac47. This branch updates it to the current HEAD commit, oxidecomputer/management-gateway-service@f9566e6.

The only changes in MGS that required code changes in Omicron are:

In my PR #7903 implementing ereport ingestion from SPs, I had to make these changes as part of changing the MGS dependency to pull in the new gateway-sp-comms code for ereports. Since this isn't actually related, and is just necessary to update the Git dep, I figured I'd pull that commit (49973ae) into its own PR.

It turns out that our Git dependency on the
oxidecomputer/management-gateway-service repo hasn't been updated in...
a while. We're currently on a commit from September of last year,
oxidecomputer/management-gateway-service@9bbac47.
This branch updates it to the current HEAD commit,
oxidecomputer/management-gateway-service@f9566e6.

The only changes in MGS that required code changes in Omicron are:

- oxidecomputer/management-gateway-service#291, where I added a new
  `MeasurementKind` for AMD CPU T<sub>ctl</sub> values (which are not temperatures in degrees Celcius, but a secret third thing).
- oxidecomputer/management-gateway-service#316 by @mkeeter, adding the
  interface to read SP task dumps over the network. Since this adds
  methods to the `sp_impl::SpHandler` trait, the SP simulator
  implementations need to be updated, or else they will no longer
  compile. For now, I've just made these `unimplemented!()`, as we're
  not currently actually _using_ them.

In my PR #7903 implementing ereport ingestion from SPs, I had to make
these changes as part of changing the MGS dependency to pull in the new
`gateway-sp-comms` code for ereports. Since this isn't actually related,
and is just necessary to update the Git dep, I figured I'd pull that
commit (49973ae) into its own PR.
@hawkw hawkw requested review from mkeeter and jgallagher April 4, 2025 21:12
Copy link
Contributor

@jgallagher jgallagher left a comment

Choose a reason for hiding this comment

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

Sorry! I had done most of this in #7666, but meant to follow up with you on the CpuTctl stuff to see if I had updated that correctly and never did. I'll close that in favor of this one; the changes are almost the same (left a couple comments where they differed) but this one is pointing to a newer commit.

Test failures probably a one-line change to nexus/tests/integration_tests/metrics.rs?

@@ -1502,6 +1504,28 @@ impl SpHandler for Handler {
}
}
}

fn get_task_dump_count(&mut self) -> Result<u32, SpError> {
unimplemented!()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd replace these with returning "not supported" errors, I think. (That's what I did on #7666.)

Co-authored-by: John Gallagher <[email protected]>
@hawkw
Copy link
Member Author

hawkw commented Apr 7, 2025

Sorry! I had done most of this in #7666, but meant to follow up with you on the CpuTctl stuff to see if I had updated that correctly and never did.

Ah, whoops, I didn't notice that branch when I went and did this! Thanks for the review!

@hawkw
Copy link
Member Author

hawkw commented Apr 7, 2025

...oh, looking at #7666, it seems like you did tag me and I approved it, but then the branch stalled out for some other reason, I guess?

@hawkw hawkw requested a review from jgallagher April 7, 2025 18:19
Copy link
Contributor

@jgallagher jgallagher left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Test failure looks legit but just needs a "rerun locally and commit" thingy?

@hawkw hawkw enabled auto-merge (squash) April 7, 2025 21:52
@hawkw hawkw merged commit e53c3e3 into main Apr 8, 2025
19 checks passed
@hawkw hawkw deleted the eliza/update-mgs branch April 8, 2025 07:09
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

Successfully merging this pull request may close these issues.

2 participants