Add region support #1054
Add region support #1054
Conversation
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/0b34e98cefc24060b0cd3c0d65f5254a ❌ openstack-meta-content-provider FAILURE in 9m 20s |
Right now, we hardcode the region. Lets get it from keystoneApi instead. Also, cinder uses os_region_name and not region_name. And we were missing a setting in ironic. Add tests for region support
vakwetu
force-pushed
the
add_region_support
branch
from
022200b to
f988f53
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/40b85831a552466e9bdad1be1972d598 ✔️ openstack-meta-content-provider SUCCESS in 2h 50m 34s |
| "openstack_region_name": "regionOne", // fixme | ||
| "default_project_domain": "Default", // fixme | ||
| "default_user_domain": "Default", // fixme | ||
| "openstack_region_name": region, |
There was a problem hiding this comment.
nit:why not directly inlined keystoneAPI.GetRegion() for consistency
its not being used anywhere else
| l logr.Logger, | ||
| namespace string, | ||
| ) (*gophercloud.ServiceClient, error) { | ||
| authURL := auth.GetKeystoneAuthURL() |
There was a problem hiding this comment.
This call already queries the KeystoneAPI CR. So re-quering the same CR now at L687 feels unnecessary duplication. Could you refactor KeytoneAPI CR usage now that we only query it once and use that CR to both get the AuthURL and the Region?
internal/controller/common.go
Outdated
| } | ||
| } | ||
|
|
||
| keystoneAPI, err := keystonev1.GetKeystoneAPI(ctx, h, namespace, map[string]string{}) |
There was a problem hiding this comment.
Do we need to check if the KeystoneAPI returned here has ready status before we can user GetRegion() call on it and trust that it returns a valid value? I.e. does KeystoneAPI.status.region always has a valid value even right after the creation of the KeystoneAPI CR itself?
| } | ||
|
|
||
| // Get region from keystoneAPI | ||
| keystoneAPI, err := keystonev1.GetKeystoneAPI(ctx, h, instance.Namespace, map[string]string{}) |
There was a problem hiding this comment.
I'm not super happy about querying KeystoneAPI CR here. So far instead of querying KeystoneAPI on each individual sub CR level for getting the KeystoneAuthURL we queried that data in the Nova CR level and passed it down from there to each sub CR like NovaAPI. This is why below we use "keystone_internal_url": instance.Spec.KeystoneAuthURL,. So I suggest to either
- a. pass down the Region as well the same way
- b. pass down neither the AuthURL nor the Region and query both on each subCR level.
I personally prefer the option a) for both, but I really want to avoid handling AuthURL as a) but Region as b).
(this comment is valid for the proposed changes for each individual subCR in this PR)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vakwetu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/689d2f6e0aa64dafa9be66312f925308 ✔️ openstack-meta-content-provider SUCCESS in 2h 54m 49s |
|
@gibizer hey - let me know if you're happy with this approach. If so, I'll work on getting the tests to pass and doing final testing. |
The namespace parameter was added in the region_name changes but is not actually used in the function. Remove it from the function signature and all call sites.
The region tests were failing because they checked the config immediately after updating the spec, but the config might not have been regenerated yet. Wrap all config checks in an Eventually block to wait for the config to be regenerated with the new region value.
According to the OpenStack inter-service region audit, Nova connects to Barbican via Castellan, which uses 'barbican_region_name' (NOT 'region_name') as the config option. Castellan defines this as a StrOpt in its Barbican key manager, and it's used for endpoint discovery. This change: - Updates the [barbican] section in nova.conf template to use barbican_region_name instead of region_name - Updates functional tests to verify barbican_region_name instead of region_name Reference: openstack-inter-service-region-audit.md line 30, 275-276
3 participants
No description provided.