Add RFC document for introspection APIs

[tristan957]

The proposed APIs will allow for a user to query information about HSE.

Fixes #4

[alexttx]

How would this impact other routes in the REST tree. It has routes for event counters, perf counters, tree shape, etc. Would this work update those routes to make use of the KVDB alias?

[tristan957]

Up in the air. It could. I don't feel like the work would be too hard to update the other routes. Maybe just two hours of work. Also happy to leave it as is, and as time marches on we can update routes. Could be left to "maintenance monday" tickets.

[tristan957]

I guess one thing this proposal didn't touch on is the potential for an opaque struct hse_mclass_storage_info (renamed since this is media class info). Are we shooting ourselves in the foot by making the struct public? Can we brainstorm other information that could be valuable? Nabeel already brought up fsid, although if a user has the paths to the media classes, they could easily check the fsid's themselves.

Is giving back the media class path good enough?

[nabeelmmd]

I guess one thing this proposal didn't touch on is the potential for an opaque struct hse_mclass_storage_info (renamed since this is media class info). Are we shooting ourselves in the foot by making the struct public? Can we brainstorm other information that could be valuable? Nabeel already brought up fsid, although if a user has the paths to the media classes, they could easily check the fsid's themselves.

Is giving back the media class path good enough?

Although having an opaque struct hse_mclass_info allows us to be flexible with this struct, it could bloat our public header file with getters for each and every storage stat.

The requirement for an API to retrieve storage stats primarily came from the Ceph connector. It is now being using by the CLI as well to make life easy for the SQA scripts.

When running mpool on a file-system, some of the reported stats are redundant with what can be obtained using the standard Linux FS utilities.

Maybe these stats will prove to be useful when we add support to run mpool on a raw device and/or pmem. To accommodate any future expansion, I think it's good enough if we reserve a few uint64_t fields in this struct. Maybe we'll never use it but that's fine.

[tristan957]

I don't really see the bloat as a bad thing, but I am fine with the reservation since the path seems to be the only large buffer thing we could ever care about.

You think reserving 4 uint64_ts sounds good?

[tristan957]

I am going to start iterating on this. I think we have landed at a good spot, and other things will be minor adjustments.

[tristan957]

As far as naming for the struct members, do we want the abbreviation prefix like we have internally?

[nabeelmmd]

I don't really see the bloat as a bad thing, but I am fine with the reservation since the path seems to be the only large buffer thing we could ever care about.

You think reserving 4 uint64_ts sounds good?

Sounds good to me.

[nabeelmmd]

As far as naming for the struct members, do we want the abbreviation prefix like we have internally?

Seems like we need an abbreviation prefix to be consistent with other structs in types.h.

[alexttx]

As far as naming for the struct members, do we want the abbreviation prefix like we have internally?

Seems like we need an abbreviation prefix to be consistent with other structs in types.h.

I agree.

[tristan957]

I would I say that I don't want hse_kvdb_class_info_get() to be a re-hashing of various system calls (statfs, statvfs, etc.). I think it is only worth presenting information that the user could not get themselves easily, for instance only HSE would know how many bytes it has reserved but are currently unused.

[smoyergh]

Agreed re not duplicating the functionality of common system calls in the HSE API.

[nabeelmmd]

I would I say that I don't want hse_kvdb_class_info_get() to be a re-hashing of various system calls (statfs, statvfs, etc.). I think it is only worth presenting information that the user could not get themselves easily, for instance only HSE would know how many bytes it has reserved but are currently unused.

The "total_bytes" and "available_bytes" are fetched using statvfs(), and this can be retrieved by the user. The user just needs to know the media class path to run statvfs() on, which we anyway plan to return in the mclass get API. The reason why we return these two fields today is (1) Requirement from the Ceph connector (2) To dump a complete "hse storage info" output that SQA can also use of in their scripts. (2) will be moot after we return path in the mclass get API, as the cli can deduce this using statvfs(). If (1) is not a requirement anymore, please feel free to remove these two fields from the API.

The "allocated_bytes" and "used_bytes" is something that HSE knows better about.

[tristan957]

Cool, so I think at this point we have a struct that looks like:

struct hse_mclass_info {
  uint64_t allocated_bytes;
  uint64_t used_bytes;
  char     path[PATH_MAX];
};

Point 1 is moot because Ceph connector can do its own thing once it knows the path.

[tristan957]

Maybe it makes sense to have a bool in the media class info struct to mean "is the media class configured". Or returning ENOENT could make sense actually if asking for info about a media class which isn't configured.

[nabeelmmd]

Maybe it makes sense to have a bool in the media class info struct to mean "is the media class configured". Or returning ENOENT could make sense actually if asking for info about a media class which isn't configured.

I vote for returning ENOENT.

[tristan957]

It looks like you actually do that within mpool itself, so it is already implemented :).

[tristan957]

As far as I am aware, the only thing blocking the inclusion of this RFC is agreement on the opaque vs non-opaque API. I went ahead with a non-opaque version in the implementing PR. Ideally I would like to merge this before Friday and accommodate any changes.

[tristan957]

It is also possible we could go with the non-opaque API and we just break the ABI if we chose not to reserve space in the struct.

[smoyergh]

I'm good with non-opaque -- I'm not a huge fan of getters all over the API.

[tristan957]

Current implementation calls for 4 reserved uint64_t slots. Should we go with no reserved data, less reserved data, or more reserved data?

[nabeelmmd]

I would reserve 8 x uint64_t, and I think that should be good enough for any future expansion..

[smoyergh]

Agreed, 8x u64 seems like a reasonable guess.

[tristan957]

Will amend the RFC and merge it tomorrow.

[tristan957]

Looks like I need to be re-approved after rewriting the Git history, but the changes are now made.

[tristan957]

@bhaveshvasandani did we want to keep the "Dismiss stale reviews" option in this repo?

[bhaveshvasandani]

@bhaveshvasandani did we want to keep the "Dismiss stale reviews" option in this repo?

I have disabled the "Dismiss stale reviews"

[tristan957]

Thanks for the reviews. I think the RFC ended up in a pretty good spot.