-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
discovery: add support for openstack instance flavor name in latest API versions #14112
Conversation
bdd92ab
to
6827549
Compare
Thanks for this, For the floating IPs, the deprecated link you shared suggests using the Network APIs directly https://docs.openstack.org/api-ref/network/v2/#networks, why not using that? Regarding flavors and microversion, if we keep trying the flavor.id first and then fallback to the new one (or the other way around), we can avoid the breaking change no? If you could split this into two PR: one for floating IPs and the other for flavors, it'd be great. |
I thought about it before opening the PR, but the Network APIs for Floating IPs don't give data for all tenants if you don't have a user with the
☝️ This is the info explained on
I'm ok with it, I completely remove support for I'll just wait your answer on the first topic to take further actions 🙌 |
Is that any different from the deprecated API that we're currently using?
from https://docs.openstack.org/api-ref/compute/#list-floating-ip-addresses
Yes, if it’s possible and if it wouldn't complicate things to fallback into flavor.id if flavor.original_name doesn't exists, it would be even better. If it’s not, we can just switch. |
Even with no difference, I don't think this is working properly since the
I based my changes on output from "addresses": {
"network1": [
{
"version": 4,
"addr": "192.168.2.203",
"OS-EXT-IPS:type": "fixed",
"OS-EXT-IPS-MAC:mac_addr": "fa:16:3e:a5:d5:7b"
},
{
"version": 4,
"addr": "10.26.48.143",
"OS-EXT-IPS:type": "floating",
"OS-EXT-IPS-MAC:mac_addr": "fa:16:3e:a5:d5:7b"
}
]
},
Questions before moving forward with this:
Let me know what do you think. |
I understand your point, but let me quote myself: As for whether the APIs require overly powerful permissions, I believe that's a discussion for the Openstack side.
In a perfect world (from a maintainer's perspective), Prometheus would require one hardcoded microversion, which is the minimum version needed to retrieve all the data in the simplest way. Users would then ensure their APIs support that microversion. However, this approach is a bit of an “all or nothing/take it or leave it” situation, which is a bit extreme. Prometheus wouldn’t stop working if some metadata were missing. Plus, this approach would place more responsibility on the users. (Approach 1) (This is how Kubernetes’ CAPO operates: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/7a19fb6f038ece8702dea31df228877a446b9b99/pkg/clients/compute.go#L36-L45. However, there’s a proposal to change this as it’s seen as a rigid approach: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/7a19fb6f038ece8702dea31df228877a446b9b99/docs/proposals/20230525-openstack-microversion-support.md?plain=1#L1) Allowing users to customize that microversion param wouldn’t free them from the maintenance work (they’d still need to keep the attribute up to date, etc.). More importantly, Prometheus would still need to support all the microversions that the user could choose to set. We’d still need to maintain all the conditional branching in the code to get the right attribute or call the right API. That’s why I believe that if we want to remain flexible (support multiple microservices as long as prometheus/discovery/kubernetes/kubernetes.go Line 617 in 8894d65
Some utils (intended for the CAPO proposal above) that would help deal with microversions were added recently to (AFAIU Let's see what the others think about the two approaches. |
I think approach 2 makes more sense since seems more scalable with less effort or error-prone. But until we wait for Below you can see the output of query the Compute API endpoint, and I think we can use the ❯ curl -sk https://os.redacted.com:8774/v2.1/ | jq .
{
"version": {
"id": "v2.1",
"status": "CURRENT",
"version": "2.95",
"min_version": "2.1",
"updated": "2013-07-23T11:33:21Z",
"links": [
{
"rel": "self",
"href": "https://os.redacted.com:8774/v2.1/"
},
{
"rel": "describedby",
"type": "text/html",
"href": "http://docs.openstack.org/"
}
],
"media-types": [
{
"base": "application/json",
"type": "application/vnd.openstack.compute+json;version=2.1"
}
]
}
} Resuming what I propose to change before moving forward:
Waiting for your feedback before start creating another PRs 👨💻 |
@machine424 i created another PR in #14166 based on our discussion. I think we can move the discussions to there. |
Description
In newer OpenStack Compute API's (starting from 2.47) the field
flavor.id
doesn't exist and was been replaced by most concrete data about the flavor that is being used. To avoid this being a breaking change, I propose to add a new field in which users can define the API microversion that they want to use.At the same time, the
/os-floating-ips
API has already been deprecated and unavailable since microversion 2.36 which will fail with a 404 error. To avoid also being a breaking change, I suggest replacing the current function with a different procedure which will get the floating IP data from the currentaddresses
entry from servers detail list. This replacement gives the same behavior as the previous one.References