Skip to content

Conversation

@jgallagher
Copy link
Contributor

This allows us to fetch things like monorail port status information:

bash% curl -s http://[::1]:12345/sp/switch/0/component/monorail | jq
[
  {
    "type": "port_status",
    "port": 0,
    "cfg": {
      "mode": {
        "mode": "sgmii",
        "speed": {
          "speed": "speed100_m"
        }
      },
      "dev_type": {
        "type": "dev1g"
      },
      "dev_num": 0,
      "serdes_type": {
        "type": "serdes1g"
      },
      "serdes_num": 1
    },
    "link_status": {
      "status": "down"
    },
    "phy_status": null,
    "counters": {
      "rx": {
        "multicast": 0,
        "unicast": 0,
        "broadcast": 0
      },
      "tx": {
        "multicast": 4636081,
        "unicast": 2,
        "broadcast": 0
      },
      "link_down_sticky": true,
      "phy_link_down_sticky": false
    }
  },
  ... repeated for the rest of the ports ...

and current sensor readings for any devices that have them:

# use JQ to find the first couple devices with capabilities == 2; this really ought to be `capabilities & 2 == 2` but bitwise ops in jq don't exist, I think?
bash% curl -s http://[::1]:12345/sp/sled/19/component | jq 'limit(2; .components | .[] | select(.capabilities == 2))'
{
  "component": "dev-0",
  "device": "tmp117",
  "serial_number": null,
  "description": "Southwest temperature sensor",
  "capabilities": 2,
  "presence": "present"
}
{
  "component": "dev-1",
  "device": "tmp117",
  "serial_number": null,
  "description": "South temperature sensor",
  "capabilities": 2,
  "presence": "present"
}

# fetch the details of those two components
bash% curl -s http://[::1]:12345/sp/sled/19/component/dev-0
[{"type":"measurement","name":"Southwest","kind":{"kind":"temperature"},"value":30.5}]
bash% curl -s http://[::1]:12345/sp/sled/19/component/dev-1
[{"type":"measurement","name":"South","kind":{"kind":"temperature"},"value":30.679688}]

This builds on #2284 and should be merged after it. It's not nearly as large as the diff implies: the vast majority of that is changes to the OpenAPI spec, and the majority of what's left after that is From impls.

Copy link
Contributor

@andrewjstone andrewjstone left a comment

Choose a reason for hiding this comment

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

This code looks good, and is indeed much smaller than its apparent size.

However, unlike in my last review for #2284, the From impls here are mostly identical type translations to what's in gateway_messages. Doing this type of translation repeatedly seems unfortunate. Is the translation only because of the way we are using progenitor here, or is there a more substantial reason for using different types on the MGS interface and the SP interface? If it's only because of progenitor, maybe we should consider how we can reuse most types if we want to.

None of the above is anywhere near urgent, and the code is quite clean. I'm just curious if we can make our lives easier with fewer conversions, or if this actually provides a benefit from translation that I'm not seeing.

@jgallagher
Copy link
Contributor Author

There are some subtle differences in the way some of the types are structured due to OpenAPI restrictions; e.g., gateway-messages defines PortMode as

pub enum PortMode {
    Sfi,
    BaseKr,
    Sgmii(Speed),
    Qsgmii(Speed),
}

but here it's defined as

pub enum PortMode {
    Sfi,
    BaseKr,
    Sgmii { speed: Speed },
    Qsgmii { speed: Speed },
}

because the variant-with-one-tuple-value isn't allowed by OpenAPI (at least where we use PortMode). There are a few other similar cases.

Stepping back a second - in order to use gateway_messages types directly in our HTTP interface, (a) they would need to implement JsonSchema and (b) that would mean changes to gateway_messages could indirectly change the API interface of MGS. (a) is slightly annoying because gateway_messages is no_std, but could be overcome by putting JsonSchema behind a cargo feature; (b) is more concerning to me. I know there's a lot of boilerplate and copy/paste, but I'm much more comfortable with the idea that the only changes to MGS's APIs come by changing the MGS crate, not any of its dependencies. Maybe I'm overly attached to that idea though, not sure.

Base automatically changed from mgs-host-startup-options to main February 2, 2023 14:37
@jgallagher jgallagher force-pushed the mgs-component-details branch from e38069d to bd08cf8 Compare February 2, 2023 14:38
@jgallagher jgallagher merged commit cdda0d6 into main Feb 2, 2023
@jgallagher jgallagher deleted the mgs-component-details branch February 2, 2023 17:14
@jgallagher
Copy link
Contributor Author

(Happy to continue the conversation about duplicated types even though this is merged!)

@andrewjstone
Copy link
Contributor

Thanks for the details @jgallagher. I think you are probably right here, and the differences and subtle nuances will keep arising such that we'd always have some weird mix of "gateway-messages" types and MGS api types.

gateway_messages could indirectly change the API interface of MGS.

I believe this is precisely the reason progenitor doesn't automatically re-use types. I think @ahl was more worried about this with regard to public APIs, but I think your concern here is also valid.

In short, let's keep things how they are and I'll stop looking at them as "duplicates" .

@bnaecker
Copy link
Collaborator

bnaecker commented Feb 2, 2023

I don't want to weigh in on whether it makes sense in this case, but Adam recently added support for "replacement" types in progenitor. One specifies a set of names, and the corresponding type to use, and it'll generate code that directly uses those, instead of recreating an equivalent type from the OpenAPI spec. I used it with great success in dendrite. In that particular case, we have an IPv6 CIDR type, and we would much rather use that directly than write a regular expression for IPv6 addresses with which progenitor would validate a string. That's a pretty niche use-case, but I wanted to point out that this support is now first-class in progenitor.

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.

4 participants