Skip to content
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

Parity with python client for version variables? #178

Open
lentzi90 opened this issue Dec 9, 2022 · 6 comments
Open

Parity with python client for version variables? #178

lentzi90 opened this issue Dec 9, 2022 · 6 comments

Comments

@lentzi90
Copy link

lentzi90 commented Dec 9, 2022

I have a use case for setting the compute API version in clouds.yaml and/or thought the env variable COMPUTE_API_VERSION, but only identity and volume API versions are currently available. It is however, available in python.

After some more research on this, there seems to be more missing in gophercloud than just the compute API version. Would it be acceptable to bring it into parity with the python client? These are the variables that can be used with the python client but are ignored by gophercloud from what I can tell:

export OS_BAREMETAL_API_VERSION
export OS_COMPUTE_API_VERSION
export OS_IMAGE_API_VERSION
export OS_NETWORK_API_VERSION
export OS_ORCHESTRATION_API_VERSION
export OS_IRONIC_API_VERSION
export OS_PLACEMENT_API_VERSION

Would it be possible to add the compute API version here as well? I would be happy to work on a PR!

Edit: Edited to expand from just comput API version to bring the clientconfig up to parity with the python client.

I'm not well versed in the OpenStack SDK, but I have managed to dig up some hopefully relevant code.
Link to OpenStackSDK files:

@lentzi90 lentzi90 changed the title Support compute API version in clouds.yaml Parity with python client for version variables? Dec 16, 2022
@pierreprinetti
Copy link
Contributor

I believe that Utils currently supports setting versions through clouds.yaml, and not microversions. The difference for us is that versions determine a different behaviour client-side, while microversions are just passed up to OpenStack (example: older microversions of Nova accept server groups "policies", newer just "policy". Gophercloud delegates the issue to the consumer and expects them to set the microversion accordingly).

As a consequence we may just parse the microversion variable, if any, and insert it into the gophercloud.ServiceClient. This could happen right into clientconfig.NewServiceClient.

Is that what you had in mind?

@lentzi90
Copy link
Author

I'm a bit confused about the distinction still, but the goal is to be able to set the OS_*_API_VERSION variables (either in clouds.yaml or environment) and have them respected. From what I can tell they would need to be listed in here.

@lentzi90
Copy link
Author

Ok after some more digging I have a better understanding of this now.
The goal for me would be to be able to set the API versions in clouds.yaml that I pass to Cluster API provider OpenStack. Since CAPO relies on gophercloud for the communication with the OpenStack API I assumed it would make sense to detect what versions to use on the gophercloud side. Basically what would be needed is to check for all the OS_*_API_VERSION variables in environment, clouds.yaml and flags, and then set the relevant microversion (if any). Similar to how the versions are already handled.

However, I see now that in the docs for microversions it is expected that the client sets these versions. So essentially it would be the job of CAPO to detect and set them. I do see the logic in letting the client provide some "defaults" or overrides (e.g. CAPO has a minimum required nova microversion). However, I think it would make sense to also automatically detect and use any microversions set through environment variables, clouds.yaml or flags, and doing that in gophercloud makes more sense than each client doing it separately.

So essentially what I would like to do is change

case "compute":
return openstack.NewComputeV2(pClient, eo)

into something like

	case "compute":
                client, err := openstack.NewComputeV2(pClient, eo)
                client.Microversion = detectComputeMicroversion(cloud)
		return client, err

(But obviously with a nicer implementation.)

Does it make sense?

@pierreprinetti
Copy link
Contributor

I think that what you propose makes complete sense, especially here in utils where we don't have backwards-compatibility constraints.

@pierreprinetti
Copy link
Contributor

Consistently with what we do when adding API calls, please include pointers to the Python SDK code to justify any microversion parsing in clouds.yaml.

@lentzi90
Copy link
Author

Thanks @pierreprinetti ! I have updated the description with some links that I think show that the python SDK is parsing the clouds.yaml. I will start working on a draft PR and see if I can make sense of this 🙂

@lentzi90 lentzi90 mentioned this issue Feb 9, 2023
3 tasks
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 a pull request may close this issue.

2 participants