discovery: add support for openstack instance flavor name in latest API versions

[paulojmdias]

• feat: add support for gathering flavor name instead of id in latest versions
• feat: remove needs of Floating IPs API which is deprecated. See https://docs.openstack.org/api-ref/compute/#list-floating-ip-addresses

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 current addresses entry from servers detail list. This replacement gives the same behavior as the previous one.

References

• https://docs.openstack.org/api-ref/compute/#list-servers-detailed
• https://docs.openstack.org/api-guide/compute/microversions.html
• https://docs.openstack.org/api-ref/compute/#list-floating-ip-addresses

[machine424]

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?
It may require an extra request than your solution but I think it'll be cleaner and more stable than relying on those OS-EXT-IPS-XXX attributes, wdyt?

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?
Also, if the field was deprecated for a long time and the new one bring better info, we can just replace it, as we don't guarantee stability for OS SD https://prometheus.io/docs/prometheus/latest/stability/

If you could split this into two PR: one for floating IPs and the other for flavors, it'd be great.

[paulojmdias]

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? It may require an extra request than your solution but I think it'll be cleaner and more stable than relying on those OS-EXT-IPS-XXX attributes, wdyt?

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 admin role. So if the config has the all_tenants: true option and the user doesn't have the admin role, this will not take any effect.

Lists floating IPs visible to the user.
Default policy settings return only the floating IPs owned by the user’s project, unless the user has admin role.

☝️ This is the info explained on /v2.0/floatingips page of Networking APIs.

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? Also, if the field was deprecated for a long time and the new one bring better info, we can just replace it, as we don't guarantee stability for OS SD https://prometheus.io/docs/prometheus/latest/stability/

I'm ok with it, I completely remove support for flavor.id and just keep using flavor.original_name as the source.

I'll just wait your answer on the first topic to take further actions 🙌

[machine424]

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 admin role. So if the config has the all_tenants: true option and the user doesn't have the admin role, this will not take any effect.

Is that any different from the deprecated API that we're currently using?
I can read These APIs are proxy calls to the Network service [...] and

Lists floating IP addresses associated with the tenant or account.

Policy defaults enable only users with the administrative role or the owner of the server to perform this operation. Cloud providers can change these permissions through the policy.json file.

from https://docs.openstack.org/api-ref/compute/#list-floating-ip-addresses

I'm ok with it, I completely remove support for flavor.id and just keep using flavor.original_name as the source.

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.

[paulojmdias]

Is that any different from the deprecated API that we're currently using? I can read These APIs are proxy calls to the Network service [...] and

Lists floating IP addresses associated with the tenant or account.

Policy defaults enable only users with the administrative role or the owner of the server to perform this operation. Cloud providers can change these permissions through the policy.json file.

from https://docs.openstack.org/api-ref/compute/#list-floating-ip-addresses

Even with no difference, I don't think this is working properly since the /os-floating-ips (compute) or /floatingips (network) API only returns data owned by the user's project.
I also don't see it as safer, to mark this feature will need an admin role to get all the floating IPs when allTenants: true is defined, which will be rejected by most OpenStack admins.

Lists floating IPs visible to the user.

Default policy settings return only the floating IPs owned by the user’s project, unless the user has admin role.

I based my changes on output from /servers/detail API which already gives the needs to make this work without an additional API request which needs an admin role to work properly. What are your thoughts?

"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"
      }
    ]
 },

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.

Questions before moving forward with this:

• I should keep support for client_microversion on that PR?
  • If yes, if client_microversion is defined with latest as default, the discovery will break since the /os-floating-ips won't exists anymore
  • I can define it with a lower value by default, which is 2.1, and will not break anything

Let me know what do you think.

[machine424]

Even with no difference, I don't think this is working properly since the /os-floating-ips (compute) or /floatingips (network) API only returns data owned by the user's project.
I also don't see it as safer, to mark this feature will need an admin role to get all the floating IPs when allTenants: true is defined, which will be rejected by most OpenStack admins. [...]

I understand your point, but let me quote myself: While it might require an extra request compared to your solution, I believe it'll result in a cleaner and more stable system than relying on those OS-EXT-IPS-XXX attributes. After some investigation, I found that this field is indeed considered an implementation detail. From the addresses field documentation: Please consult the OpenStack Networking API for up-to-date information. This statement was added here https://review.opendev.org/c/openstack/nova/+/827856?tab=comments where more details are provided.

As for whether the APIs require overly powerful permissions, I believe that's a discussion for the Openstack side.

Here are some questions before we proceed:

* Should I maintain support for `client_microversion` in this PR?
  
  * If yes, if `client_microversion` is set to `latest` by default, the discovery will fail since `/os-floating-ips` will no longer exist
  * I could set it to a lower default value, such as `2.1`, which wouldn't break anything

I'd love to hear your thoughts.

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 gophercloud allows us to do so), we can just add the logic without having to expose the microversion parameter, as we already do in Kube SD, for example:

prometheus/discovery/kubernetes/kubernetes.go

Line 617 in 8894d65

v1Supported, err = checkNetworkingV1Supported(d.client)

Some utils (intended for the CAPO proposal above) that would help deal with microversions were added recently to gophercloud: https://github.com/gophercloud/gophercloud/blob/930d18cf918bf57f04a6c50a9f7cae23cad9c87a/openstack/utils/choose_version.go#L126. But we’ll need to wait for 2.0.0 to get them. With that, we’ll be able to tailor-make. (Approach 2)

(AFAIU microversion is intended to be per request, but it seems gophercloud only allows it to be set client-wide. I’ll check with the folks to see if it’s safe/intended to change that on the fly. Regardless, I think we’ll be able to find a workaround.)

Let's see what the others think about the two approaches.

[paulojmdias]

Even with no difference, I don't think this is working properly since the /os-floating-ips (compute) or /floatingips (network) API only returns data owned by the user's project.
I also don't see it as safer, to mark this feature will need an admin role to get all the floating IPs when allTenants: true is defined, which will be rejected by most OpenStack admins. [...]

I understand your point, but let me quote myself: While it might require an extra request compared to your solution, I believe it'll result in a cleaner and more stable system than relying on those OS-EXT-IPS-XXX attributes. After some investigation, I found that this field is indeed considered an implementation detail. From the addresses field documentation: Please consult the OpenStack Networking API for up-to-date information. This statement was added here https://review.opendev.org/c/openstack/nova/+/827856?tab=comments where more details are provided.

As for whether the APIs require overly powerful permissions, I believe that's a discussion for the Openstack side.

Here are some questions before we proceed:

* Should I maintain support for `client_microversion` in this PR?
  
  * If yes, if `client_microversion` is set to `latest` by default, the discovery will fail since `/os-floating-ips` will no longer exist
  * I could set it to a lower default value, such as `2.1`, which wouldn't break anything

I'd love to hear your thoughts.

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 gophercloud allows us to do so), we can just add the logic without having to expose the microversion parameter, as we already do in Kube SD, for example:

prometheus/discovery/kubernetes/kubernetes.go

Line 617 in 8894d65

v1Supported, err = checkNetworkingV1Supported(d.client)

Some utils (intended for the CAPO proposal above) that would help deal with microversions were added recently to gophercloud: https://github.com/gophercloud/gophercloud/blob/930d18cf918bf57f04a6c50a9f7cae23cad9c87a/openstack/utils/choose_version.go#L126. But we’ll need to wait for 2.0.0 to get them. With that, we’ll be able to tailor-make. (Approach 2)

(AFAIU microversion is intended to be per request, but it seems gophercloud only allows it to be set client-wide. I’ll check with the folks to see if it’s safe/intended to change that on the fly. Regardless, I think we’ll be able to find a workaround.)

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 2.0.0 release of gophercloud, what do you think if we have a function which gets the maximum version available for Openstack compute API and uses it within client.Microversion?

Below you can see the output of query the Compute API endpoint, and I think we can use the version.version value to take actions on it.

❯ 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:

• Move Floating IPs Function to use the Network APIs instead of compute APIs since Compute API for Floating IPs has acted as a proxy for Network APIs since the beginning and was deprecated on 2.36 micro version.
  • https://github.com/prometheus/prometheus/blob/main/discovery/openstack/instance.go#L94-L115
• Develop a function which gets the highest microversion supported from Compute API and use it as a setting in client.Microversion value.
• Try to use flavor.original_name and fallback for flavor.id if the first doesn't exists.

Waiting for your feedback before start creating another PRs 👨‍💻

[paulojmdias]

@machine424 i created another PR in #14166 based on our discussion. I think we can move the discussions to there.