Conversation
7815457 to
761af95
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
majewsky
left a comment
There was a problem hiding this comment.
First bunch of comments.
There was a problem hiding this comment.
I don't want to have the v2 API specs as Markdown documents, because those tend to drift from reality faster than code comments. The v2 API should be documented in the same style as https://pkg.go.dev/github.com/sapcc/go-api-declarations/liquid, as a package with type declarations and overall documentation in the package docstring.
We can have this Markdown file as an entrypoint, but then it should point to pkg.go.dev for the actual specs.
There was a problem hiding this comment.
Please place this in a new package, e.g. internal/api/v2, that does not depend on the old stuff. I would like to be able to just delete the old package once we're done with the migration.
Also, following up on the comment about spec formatting above, I would like to have this organized into packages with just type declarations, that can move into go-api-declarations once we are happy with the API, and the implementation package containing the HTTP handlers and associated logic.
There was a problem hiding this comment.
What about functions that we need in both versions, do we copy them then?
I would argue that we can have dependencies to the reusable v1-stuff and then copy just them as soon as we delete the old provider?
There was a problem hiding this comment.
We decided to do it the other way around: Move the re-usable functions to v2 and use them from v1.
| type V2ServiceInfoReport struct { | ||
| Version int64 `json:"version"` | ||
| DisplayName string `json:"display_name"` | ||
| ResourceCategories map[string]V2ServiceCategoryReport `json:"resources"` |
There was a problem hiding this comment.
| ResourceCategories map[string]V2ServiceCategoryReport `json:"resources"` | |
| ResourceCategories map[string]V2ServiceCategoryReport `json:"categories"` |
| type V2ServiceInfoReport struct { | ||
| Version int64 `json:"version"` | ||
| DisplayName string `json:"display_name"` | ||
| ResourceCategories map[string]V2ServiceCategoryReport `json:"resources"` |
There was a problem hiding this comment.
I had a shower thought about this yesterday, that I want to flag for further consideration: I requested the explicit structuring as areas -> services -> categories -> resources instead of services (has area) -> resources (has category) on the basis of "it makes more sense from a data structure point of view".
However, if categories end up changing over time, that might be annoying for users who interact with the Limes API through e.g. curl+jq. Imagine writing out .service_areas["compute"].services["nova"].categories["hv_version_2"].resources["ram_hv_version_2"] in your jq script, and then a month later the liquid-nova maintainer decides to rename the category from hv_version_2 to sapphire_rapids or something. That would be needless breakage for the customer that would be avoided by remaining with the old services -> resources structure since service and resource names are more likely to be understood as being part of the API contract.
In writing this out, I am already realizing that my concern might be overblown, but I want to flag it as worthy of at least one more thought. :)
There was a problem hiding this comment.
We decided: It's fine, variability should be limited to the display name. (Mark our words...)
| // V2InfoReport is the response type for GET /v2/info. | ||
| // It contains all metadata information about the clusters services. | ||
| type V2InfoReport struct { | ||
| ServiceAreas map[string]V2ServiceAreaReport `json:"service_areas"` |
There was a problem hiding this comment.
| ServiceAreas map[string]V2ServiceAreaReport `json:"service_areas"` | |
| Areas map[string]V2AreaReport `json:"areas"` |
There are no other types of areas that we need to distinguish from.
| // TODO: Do we need to do a v2-name-mapping or are the v2 names 1:1 from the database and the | ||
| // display names are sufficient? |
There was a problem hiding this comment.
The latter. I want to avoid name mapping in v2 until we absolutely must.
| report := V2InfoReport{ | ||
| ServiceAreas: make(map[string]V2ServiceAreaReport), | ||
| } | ||
| serviceInfos, err := p.Cluster.AllServiceInfos() |
There was a problem hiding this comment.
AllServiceInfos is a leftover from when we held all ServiceInfo in memory at all times. I would like to eventually get rid of it. For the v1 API, we will not change it because that has to go anyway. For collector tasks, I will eventually want to do a small refactor to only have it load the few attributes that each particular task needs.
For v2, this place might be the only one where we are pulling from services and resources. Everything else ought to be able to work on what is in the info that is assembled here. Can we rewrite the queries that this function is already doing to retrieve services.* and resources.*?
There was a problem hiding this comment.
Some of the points I recall from the discussion we just had:
- Most likely, we don't want to mix the retrieval of
serviceandresourcewith the retrieval of the actual data in the report APIs, because that would mean we have to do a bunch of(resource = $1 OR $1 = ""). This would make the statements not only more complex to read, but also pricey for the database - We might want to cache the
servicesandresourcesand use postgresql's notify feature to invalidate this cache from thecollect-taskto theservice-tasklater. - We thought about the format: Probably, we want to get rid of the format of
ServiceInfoLimes-internally. TheServiceInfocomes from liquid, is persisted on change and then we work internally with thedbartifacts directly. - Also, we thought about caching the
serviceandresourcein the form of the/info-API report, but this would mean conversion into that format when reading the cache (which is ugly, likeServiceInfo) and also some overhead when retrieving data (e.g. with the category).
|
|
||
| // GetInfo handles GET /v2/info. | ||
| func (p *v2Provider) GetInfo(w http.ResponseWriter, r *http.Request) { | ||
| httpapi.IdentifyEndpoint(r, "/v2/info") |
There was a problem hiding this comment.
I would like to keep the split between resource and rate API, because most users of resources are not going to be interested in rates and vice versa. We should have this as /resources/v2/info and /rates/v2/info. I hope that this is possible to do without needing too much code duplication.
There was a problem hiding this comment.
You mean /v2/resources/info and not resources/v2/info? Likewise for rates.
There was a problem hiding this comment.
Has historic reasons in the split of resources and rates, also in the keystone catalogue.
There was a problem hiding this comment.
One of my big gripes with the v1 API test suite is having a billion fixtures/*.json files. I don't want to repeat that, esp. with all the filtering that can happen once we get into combinations of ?area=foo&service=bar&category=baz&resource=qux and all the ?with= flags that we have in mind.
As a possible idea, for each "class" of tests, we could have a baseline as an assert.JSONObject literal, and then have modifier functions can we can stack on top, like
var fullInfo = assert.JSONObject { ... }
func withoutCommitmentInfo(info assert.JSONObject) assert.JSONObject { ... }
So then we could test /v2/info as cloud-admin against fullInfo and /v2/info as domain admin without commitments enabled against withoutCommitmentInfo(fullInfo), and then have all other combinations of stuff follow from that.
We will have to see how this scales to full reports with real data. I'm also open for alternative suggestions for how to structure this. But I would like to not go down the "bunch of slightly different JSON files" route again.
There was a problem hiding this comment.
How would you modify the JSONObject in an efficient way?
I would support this, but only if we don't need x loops to iterate over the deep json to delete or modify the keys we want to have.
Using the jq/yq syntax would be kind of neat (IMHO), but I understand that this is not a nice dependency to have.
There was a problem hiding this comment.
We agreed that modifying the full JSON with some kind of additional language would make sense, but the question is which one. We make this an exploratory task to find the right package to modify the full json objects.
| assert.HTTPRequest{ | ||
| Method: "GET", | ||
| Path: "/v2/info", | ||
| ExpectStatus: 200, | ||
| ExpectBody: assert.JSONFixtureFile("./fixtures/info-cluster.json"), | ||
| }.Check(t, s.Handler) |
There was a problem hiding this comment.
assert.HTTPRequest is soft-deprecated. Please write new tests with https://pkg.go.dev/github.com/sapcc/go-bits/httptest.
This ties into my previous comment about JSON fixture files: httptest does not properly support JSON fixture files because of my aforementioned gripes with those.
|
For next steps for this PR, we discussed the following order:
|
This marks the start of development of the actual
/v2API. We start with the info endpoint, which will include all static configuration in future, so it can be divided from movement data.For now, this remains a draft. Before merging this, we should take care of most of the
TODOsI marked for discussions with @majewsky.