-
Notifications
You must be signed in to change notification settings - Fork 52
OSASINFRA-3960: Migrate OpenStack InfraCluster controller in-tree #411
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
OSASINFRA-3960: Migrate OpenStack InfraCluster controller in-tree #411
Conversation
|
Pipeline controller notification For optional jobs, comment |
WalkthroughAdds OpenStack-specific infra discovery and OpenStackCluster creation to the InfraCluster controller, delegates OpenStack handling to a new ensureOpenStackCluster routine, implements subnet/router discovery from control-plane machines, adjusts PowerVS error handling, adds OpenStack e2e test and scheme registration, and updates images/manifests and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Ctrl as InfraClusterController
participant Ens as ensureOpenStackCluster
participant Sub as getDefaultSubnetFromMachines
participant Rtr as getDefaultRouterFromSubnet
participant KC as Kubernetes API
participant NC as OpenStack Network API
Ctrl->>Ens: ensureOpenStackCluster(ctx, log)
Ens->>Ens: validate PlatformStatus & LB type
Ens->>Sub: getDefaultSubnetFromMachines(ctx,...)
Sub->>KC: list control-plane Machines
loop per machine
Sub->>NC: list ports for instance
Sub->>NC: map ports → subnets
end
Sub-->>Ens: return subnet or error
Ens->>Rtr: getDefaultRouterFromSubnet(subnet)
Rtr->>NC: find port for subnet gateway IP
Rtr->>NC: fetch router & verify external gateway
Rtr-->>Ens: router or error
Ens->>KC: create/update OpenStackCluster with Network/Router/ExternalNetwork/IdentityRef
Ens-->>Ctrl: return OpenStackCluster object or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/controllers/infracluster/powervs.go (1)
39-42: SharederrInvalidPlatformStatussentinel is a reasonable consolidationSwitching to a package-level
errInvalidPlatformStatusfor the nilPlatformStatuscase keeps behavior the same while making it easier to reuse and to assert on in tests. No functional issues spotted. If we end up using this sentinel from more controllers (as already done in the OpenStack path), consider moving it into a small sharederrors.goin this package for discoverability.Also applies to: 44-44, 74-76
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (198)
go.sumis excluded by!**/*.sumvendor/github.com/gofrs/uuid/v5/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/.pre-commit-config.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/codec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/generator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/sql.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/auth_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/endpoint_location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/delegate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/utils/base_endpoint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/utils/choose_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/http.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/linked.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/marker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/pager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/pkg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/single.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/client/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/client/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/env/env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/env/env_windows.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/gnocchi/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/gnocchi/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/internal/pkg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/internal/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/utils.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/call.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/callset.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/controller.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/string.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/mockgen/model/model.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/compute.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/image.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/compute.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/image.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/network.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/volume.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/networking.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/volume.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/metrics/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/hash.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/mock.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/provider.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/scope.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/openstack/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/openstack/microversion.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/version/version.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.mod(4 hunks)pkg/controllers/infracluster/infracluster_controller.go(2 hunks)pkg/controllers/infracluster/openstack.go(1 hunks)pkg/controllers/infracluster/powervs.go(2 hunks)
🔇 Additional comments (3)
go.mod (1)
13-13: New OpenStack and testing dependencies look consistent with the new controllerThe additions of
gophercloud/v2,gophercloud/utils/v2,gofrs/uuid/v5, andgo.uber.org/mockfit the new in-tree OpenStack InfraCluster controller and its tests; nothing stands out as problematic here. Please just make surego.sumis updated and that these versions are compatible with the CAPO/OpenStack client versions you rely on in this repo.Also applies to: 138-138, 152-152, 278-278
pkg/controllers/infracluster/infracluster_controller.go (1)
206-213: OpenStack and PowerVS branches inensureInfraClusterare wired consistentlyDelegating the OpenStack platform path to
ensureOpenStackClusterand aligning the PowerVS error message with the other providers ("error getting InfraCluster object") keepsensureInfraClusteruniform and pushes provider-specific logic into dedicated helpers. The control flow and error propagation here look sound.Also applies to: 228-233
pkg/controllers/infracluster/openstack.go (1)
144-149: No explicit cleanup required — scope pattern is correct as-isVerification confirms that
openstackscope.NewFactory(...).NewClientScopeFromObjectdoes not expose or require an explicitClose()call. The recommended usage pattern is to create a scope per reconciliation/operation (short-lived) and let it naturally go out of scope at the end of the controller reconcile. The current code follows this pattern correctly and requires no changes.
|
/retitle OSASINFRA-3960: Migrate OpenStack InfraCluster controller in-tree |
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
ef23116 to
31fbbd1
Compare
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/controllers/infracluster/openstack.go (1)
156-163: Thread configurable MAPI namespace intogetDefaultSubnetFromMachines(still effectively hard-coded)
getDefaultSubnetFromMachinescurrently always lists machines indefaultMAPINamespace:if err := kubeclient.List( ctx, &mapiMachines, client.InNamespace(defaultMAPINamespace), client.MatchingLabels{"machine.openshift.io/cluster-api-machine-role": "master"}, ); err != nil { ... }and
ensureOpenStackClustercalls it without any namespace argument:defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus)Even though other parts of the controller make the Machine API namespace configurable (via a
MAPINamespacefield / config), this helper bypasses that and will break OpenStack subnet discovery wheneverMAPINamespaceis overridden (tests, non‑standard installs, etc.). This is the same underlying issue that was previously flagged; only the literal string was replaced by a constant.Consider passing the namespace through instead so discovery honors configuration, for example:
-func getDefaultSubnetFromMachines(ctx context.Context, log logr.Logger, kubeclient client.Client, networkClient openstackclients.NetworkClient, platformStatus *configv1.OpenStackPlatformStatus) (*subnets.Subnet, error) { +func getDefaultSubnetFromMachines( + ctx context.Context, + log logr.Logger, + kubeclient client.Client, + networkClient openstackclients.NetworkClient, + platformStatus *configv1.OpenStackPlatformStatus, + machineNamespace string, +) (*subnets.Subnet, error) { @@ - if err := kubeclient.List( + if err := kubeclient.List( ctx, &mapiMachines, - client.InNamespace(defaultMAPINamespace), + client.InNamespace(machineNamespace), client.MatchingLabels{"machine.openshift.io/cluster-api-machine-role": "master"}, ); err != nil {and in
ensureOpenStackCluster:- defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus) + defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus, r.MAPINamespace)That keeps behavior identical for the default case while fixing configurations/tests that deliberately override
MAPINamespace.Also applies to: 235-245
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (199)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumvendor/github.com/gofrs/uuid/v5/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/.pre-commit-config.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/codec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/generator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/sql.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/auth_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/endpoint_location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/delegate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/utils/base_endpoint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/utils/choose_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/http.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/linked.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/marker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/pager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/pkg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/single.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/client/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/client/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/env/env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/env/env_windows.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/gnocchi/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/gnocchi/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/internal/pkg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/internal/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/utils.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/call.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/callset.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/controller.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/string.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/mockgen/model/model.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/compute.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/image.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/compute.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/image.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/network.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/volume.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/networking.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/volume.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/metrics/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/hash.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/mock.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/provider.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/scope.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/openstack/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/openstack/microversion.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/version/version.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
e2e/go.mod(2 hunks)e2e/openstack_test.go(1 hunks)go.mod(4 hunks)pkg/controllers/infracluster/infracluster_controller.go(2 hunks)pkg/controllers/infracluster/openstack.go(1 hunks)pkg/controllers/infracluster/powervs.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/infracluster/infracluster_controller.go
🔇 Additional comments (3)
e2e/go.mod (1)
24-24: OpenStack e2e dependencies look consistent with the main moduleAdding
cluster-api-provider-openstack v0.12.4andgophercloud/v2 v2.7.0here matches the root module and aligns with the new OpenStack e2e coverage; no issues from a dependency-graph standpoint. Just ensurego mod tidyfor./e2estays clean as follow‑ups land.Also applies to: 67-67
go.mod (1)
13-13: New OpenStack and test support dependencies look appropriateDirect
gophercloud/v2plusgophercloud/utils/v2,gofrs/uuid/v5, andgo.uber.org/mockare all consistent with the new OpenStack infracluster controller and its tests; nothing stands out as problematic here. Just keep an eye on module tidy/upgrade drift as CAPO/gophercloud rev over time.Also applies to: 138-138, 152-152, 278-278
pkg/controllers/infracluster/powervs.go (1)
38-42: SharederrInvalidPlatformStatussentinel is a good consolidationSwitching the PowerVS
PlatformStatus == nilpath to reuseerrInvalidPlatformStatuskeeps behavior the same while giving you a reusable, comparable error across platforms (and matches the new OpenStack controller). No further changes needed here.Also applies to: 74-76
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test ci/prow/e2e-openstack-capi-techpreview |
|
@stephenfin: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/openstack_test.go (1)
114-116: Add bounds check before accessing SecurityGroups[0].The previous review identified that line 116 directly indexes
mapiProviderSpec.SecurityGroups[0]without verifying the slice length, which will panic ifSecurityGroupsis empty. This issue remains unresolved.Add a guard before line 116:
// NOTE(stephenfin): We intentionally ignore additional security for now. +Expect(len(mapiProviderSpec.SecurityGroups)).To(BeNumerically(">", 0)) var securityGroupParam openstackv1.SecurityGroupParam securityGroup := mapiProviderSpec.SecurityGroups[0]
🧹 Nitpick comments (1)
e2e/openstack_test.go (1)
152-154: Remove or uncomment the DeferCleanup code.Lines 152-154 contain commented-out cleanup code for the
OpenStackMachineTemplate. If cleanup is intentionally deferred to the parent MachineSet deletion, remove these lines entirely. Otherwise, uncomment them to ensure proper resource cleanup.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
e2e/openstack_test.go(1 hunks)
🔇 Additional comments (1)
e2e/openstack_test.go (1)
95-112: Image handling now correctly addresses boot-from-volume case.The previous review concern about the image field being empty when
RootVolumeis set has been resolved. Line 109 now correctly assignsimage.ID = ptr.To(mapiProviderSpec.RootVolume.SourceUUID), ensuring the CAPO ImageParam is properly populated for boot-from-volume deployments.
5d3b5a7 to
a831353
Compare
|
/test e2e-openstack-capi-techpreview |
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @mandre |
|
/test e2e-openstack-capi-techpreview |
err will always be nil in the context of this error. Signed-off-by: Stephen Finucane <[email protected]>
Previously, this was managed out-of-tree in the openshift fork of cluster-api-provider-openstack. This has proven difficult to maintain, and other platforms have not followed suit. Time to change course and align with what everyone else is doing. Signed-off-by: Stephen Finucane <[email protected]>
Move the network detection logic from the legacy controller [1] in-tree. [1] https://github.com/openshift/cluster-api-provider-openstack/blob/19b666d6f/openshift/pkg/infraclustercontroller/openstackcluster.go#L262-L305 Signed-off-by: Stephen Finucane <[email protected]>
a831353 to
16a833a
Compare
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/controllers/infracluster/openstack.go (1)
235-245: Honor configurable MAPI namespace instead ofdefaultMAPINamespace
getDefaultSubnetFromMachinescurrently lists control plane machines usingclient.InNamespace(defaultMAPINamespace). Elsewhere in this controllerMAPINamespaceis intentionally configurable (withdefaultMAPINamespaceonly used as a fallback inSetupWithManager). Hard‑coding the default here means:
- Tests that override
r.MAPINamespacewon’t be able to exercise this path.- Non‑standard deployments using a different machine namespace will fail OpenStack subnet discovery.
Consider threading the namespace through rather than using the constant, e.g.:
-func getDefaultSubnetFromMachines(ctx context.Context, log logr.Logger, kubeclient client.Client, networkClient openstackclients.NetworkClient, platformStatus *configv1.OpenStackPlatformStatus) (*subnets.Subnet, error) { +func getDefaultSubnetFromMachines( + ctx context.Context, + log logr.Logger, + kubeclient client.Client, + networkClient openstackclients.NetworkClient, + platformStatus *configv1.OpenStackPlatformStatus, + machineNamespace string, +) (*subnets.Subnet, error) { @@ - if err := kubeclient.List( + if err := kubeclient.List( ctx, &mapiMachines, - client.InNamespace(defaultMAPINamespace), + client.InNamespace(machineNamespace), client.MatchingLabels{"machine.openshift.io/cluster-api-machine-role": "master"}, ); err != nil {and in
ensureOpenStackCluster:- defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus) + defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus, r.MAPINamespace)That keeps behavior identical in the default case while respecting the configurable namespace.
🧹 Nitpick comments (3)
pkg/controllers/infracluster/powervs.go (1)
38-42: SharederrInvalidPlatformStatususage looks goodUsing a single package‑level
errInvalidPlatformStatusand reusing it here simplifies callers that want to special‑case “no PlatformStatus” and keeps the message consistent. If more platforms start using it (as OpenStack already does), consider moving it to a small sharederrors.goin this package for discoverability, but this is purely cosmetic.Also applies to: 74-76
pkg/controllers/infracluster/infracluster_controller.go (1)
206-233: OpenStack/PowerVS branches are wired correctly intoensureInfraClusterDelegating OpenStack via
ensureOpenStackClusterand aligning the PowerVS error wrapping with the other platforms is consistent and keeps the reconcile path uniform. The only nit is the slightly generic"error getting InfraCluster object"message, which now covers several provider‑specific ensure* helpers, but it’s acceptable as‑is.pkg/controllers/infracluster/openstack.go (1)
91-106: Minor robustness improvements for OpenStack platform validation and discoveryThe overall flow in
ensureOpenStackClusterand the discovery helpers looks good, but a few small tweaks would make it more defensive without changing the behavior:
- Guard
PlatformStatus.OpenStackexplicitlyYou already check
r.Infra.Status.PlatformStatus != nil, butPlatformStatus.OpenStackcould still be nil in theory. A quick guard avoids a potential nil dereference if the infra object is malformed:- platformStatus := r.Infra.Status.PlatformStatus.OpenStack + platformStatus := r.Infra.Status.PlatformStatus.OpenStack + if platformStatus == nil { + return nil, fmt.Errorf("%w: missing OpenStack platform status", errInvalidPlatformStatus) + }
- Handle malformed
APIServerInternalIPsmore explicitlyIn
getDefaultSubnetFromMachines,net.ParseIPreturningnilwill silently produce a “no matches” outcome. If you want clearer failure modes, you could either:
- Drop invalid entries with a log line, or
- Fail fast on the first malformed IP.
For example (drop invalid):
for i, ipStr := range platformStatus.APIServerInternalIPs { - apiServerInternalIPs[i] = net.ParseIP(ipStr) + ip := net.ParseIP(ipStr) + if ip == nil { + log.V(2).Info("Skipping malformed APIServerInternalIP", "ip", ipStr) + continue + } + apiServerInternalIPs[i] = ip }
- Don’t hard‑fail on a single instance with no ports
Right now, the first control plane machine that has zero Neutron ports causes an error:
if len(ports) == 0 { return nil, fmt.Errorf("%w: no ports found for instance %s", errOpenStackNoDefaultSubnet, instanceID) }If one machine is misconfigured but others are fine, this prevents falling back to them. Consider logging and
continueinstead:- if len(ports) == 0 { - return nil, fmt.Errorf("%w: no ports found for instance %s", errOpenStackNoDefaultSubnet, instanceID) - } + if len(ports) == 0 { + log.V(3).Info("Skipping machine: no ports found", "instanceID", instanceID) + continue + }The final
"no matching subnets found"error still captures the overall failure while making the heuristic more tolerant of one-off anomalies.None of these are blockers, but they would make the controller more resilient and easier to debug in odd environments.
Also applies to: 118-139, 141-175, 247-319
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (205)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumvendor/github.com/gofrs/uuid/v5/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/.pre-commit-config.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/codec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/generator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/sql.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gofrs/uuid/v5/uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/endpoint_search.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/auth_env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/blockstorage/v3/volumes/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/common/extensions/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/attachinterfaces/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/availabilityzones/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/flavors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servergroups/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/endpoint_location.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tenants/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v2/tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/ec2tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/oauth1/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/identity/v3/tokens/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imagedata/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/imageimport/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/image/v2/images/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/apiversions/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/flavors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/l7policies/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/listeners/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/monitors/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/pools/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/providers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/attributestags/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/delegate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/trunks/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets/urls.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/utils/base_endpoint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/utils/choose_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/http.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/linked.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/marker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/pager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/pkg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/single.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/provider_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/service_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/client/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/client/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/env/env.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/env/env_windows.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/gnocchi/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/gnocchi/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/internal/pkg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/internal/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/utils/v2/openstack/clientconfig/utils.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/call.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/callset.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/controller.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/string.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/mockgen/model/model.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/compute.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/image.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/compute.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/image.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/network.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock/volume.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/networking.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/volume.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/metrics/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/hash.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/mock.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/provider.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/scope/scope.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/openstack/loadbalancer.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/pkg/utils/openstack/microversion.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/cluster-api-provider-openstack/version/version.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (7)
e2e/go.mod(2 hunks)e2e/openstack_test.go(1 hunks)go.mod(4 hunks)hack/e2e-openstack.sh(1 hunks)pkg/controllers/infracluster/infracluster_controller.go(2 hunks)pkg/controllers/infracluster/openstack.go(1 hunks)pkg/controllers/infracluster/powervs.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- hack/e2e-openstack.sh
- e2e/openstack_test.go
- e2e/go.mod
- go.mod
As noted inline, we actually want CAPO's OpenStackCluster controller to reconcile our OpenStackCluster objects, since this is the thing that populates the .Status attribute on same. Signed-off-by: Stephen Finucane <[email protected]>
Align us with the other providers. Signed-off-by: Stephen Finucane <[email protected]>
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/controllers/infracluster/openstack.go (1)
235-243: Hard-coded namespace breaks configurability.Line 239 uses
defaultMAPINamespaceinstead of the controller's configurableMAPINamespacefield. This prevents the function from working correctly whenMAPINamespaceis overridden for tests or non-standard deployments.As previously noted in past reviews, consider threading the namespace through as a parameter:
func getDefaultSubnetFromMachines( ctx context.Context, log logr.Logger, kubeclient client.Client, networkClient openstackclients.NetworkClient, platformStatus *configv1.OpenStackPlatformStatus, + machineNamespace string, ) (*subnets.Subnet, error) { mapiMachines := mapiv1beta1.MachineList{} if err := kubeclient.List( ctx, &mapiMachines, - client.InNamespace(defaultMAPINamespace), + client.InNamespace(machineNamespace), client.MatchingLabels{"machine.openshift.io/cluster-api-machine-role": "master"}, ); err != nil {And update the caller on line 154:
- defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus) + defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus, r.MAPINamespace)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
e2e/openstack_test.go(1 hunks)pkg/controllers/infracluster/openstack.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/openstack_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/infracluster/openstack.go (2)
pkg/controllers/infracluster/infracluster_controller.go (1)
InfraClusterController(74-83)e2e/framework/framework.go (1)
CAPINamespace(14-14)
🔇 Additional comments (6)
pkg/controllers/infracluster/openstack.go (6)
1-44: LGTM!License, package declaration, and imports are appropriate for the OpenStack InfraCluster controller implementation.
60-107: LGTM!The validation logic correctly checks for OpenStackCluster existence, validates platform status, ensures supported load balancer type, and verifies APIServerInternalIPs are present.
139-147: Reminder: Resolve the caCertificates FIXME.Based on past review comments, this empty
caCertificatesinitialization is pending confirmation from @mdbooth regarding whether it should be removed or properly sourced. The current implementation may cause issues with self-signed certificates.
154-171: LGTM!The network discovery logic correctly retrieves the default subnet from control plane machines, derives the router from the subnet, and populates the OpenStackCluster spec with appropriate network, router, and external network references.
185-223: LGTM!The router discovery logic correctly identifies the router by finding the port associated with the subnet's gateway IP, then validates the router has an external gateway. The documented gophercloud limitation is appropriately noted.
254-317: LGTM!The subnet discovery logic correctly iterates through control plane machines, examines their network ports, and identifies the subnet whose CIDR contains the API server internal IPs. The detailed debug logging and comprehensive error handling are well-implemented.
We were doing this implicitly by not initialising the variable. Do so explicitly instead. Signed-off-by: Stephen Finucane <[email protected]>
d0b7fd2 to
a028609
Compare
|
@stephenfin: This pull request references OSASINFRA-3960 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/controllers/infracluster/openstack.go (1)
235-235: Use configurabler.MAPINamespaceinstead ofdefaultMAPINamespace.The hard-coded
defaultMAPINamespaceconstant bypasses the controller's configurableMAPINamespacefield, which will break subnet discovery in test environments or non-standard deployments where the namespace is overridden.Apply this diff to use the configurable namespace:
func getDefaultSubnetFromMachines(ctx context.Context, log logr.Logger, kubeclient client.Client, networkClient openstackclients.NetworkClient, platformStatus *configv1.OpenStackPlatformStatus) (*subnets.Subnet, error) { + return getDefaultSubnetFromMachinesWithNamespace(ctx, log, kubeclient, networkClient, platformStatus, defaultMAPINamespace) +} + +func getDefaultSubnetFromMachinesWithNamespace( + ctx context.Context, + log logr.Logger, + kubeclient client.Client, + networkClient openstackclients.NetworkClient, + platformStatus *configv1.OpenStackPlatformStatus, + machineNamespace string, +) (*subnets.Subnet, error) { mapiMachines := mapiv1beta1.MachineList{} if err := kubeclient.List( ctx, &mapiMachines, - client.InNamespace(defaultMAPINamespace), + client.InNamespace(machineNamespace), client.MatchingLabels{"machine.openshift.io/cluster-api-machine-role": "master"}, ); err != nil {Alternative (simpler): Thread
r.MAPINamespaceas a parameter:-func getDefaultSubnetFromMachines(ctx context.Context, log logr.Logger, kubeclient client.Client, networkClient openstackclients.NetworkClient, platformStatus *configv1.OpenStackPlatformStatus) (*subnets.Subnet, error) { +func getDefaultSubnetFromMachines( + ctx context.Context, + log logr.Logger, + kubeclient client.Client, + networkClient openstackclients.NetworkClient, + platformStatus *configv1.OpenStackPlatformStatus, + machineNamespace string, +) (*subnets.Subnet, error) { mapiMachines := mapiv1beta1.MachineList{} if err := kubeclient.List( ctx, &mapiMachines, - client.InNamespace(defaultMAPINamespace), + client.InNamespace(machineNamespace), client.MatchingLabels{"machine.openshift.io/cluster-api-machine-role": "master"}, ); err != nil {And update the caller at line 150:
- defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus) + defaultSubnet, err := getDefaultSubnetFromMachines(ctx, log, r.Client, networkClient, platformStatus, r.MAPINamespace)Based on learnings, this is a previously flagged issue that should be addressed to ensure the controller honors configured namespaces in all environments.
🧹 Nitpick comments (2)
pkg/controllers/infracluster/openstack.go (2)
75-85: Clarify whether API URL parsing is needed.The FIXME comment indicates uncertainty about whether the API URL parsing logic (used by other providers) should be implemented for OpenStack. This commented-out code would extract host and port from
r.Infra.Status.APIServerInternalURL, instead of the current hard-coded approach at lines 117-118.If this is a known limitation to address before GA, consider opening a tracking issue.
Do you want me to open an issue to track this decision, or can you confirm the current hard-coded approach (using the first APIServerInternalIP and port 6443) is correct for OpenStack?
188-191: Consider adding a link to the gophercloud issue.The XXX comment references a gophercloud limitation but doesn't link to the issue. Adding the issue URL would help future maintainers track when this workaround can be removed.
// XXX: We should search on both subnet and IP // address here, but can't because of - // https://github.com/gophercloud/gophercloud/issues/2807 + // https://github.com/gophercloud/gophercloud/issues/2807. + // Remove this workaround once the issue is resolved. // SubnetID: subnet.ID,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/controllers/infracluster/openstack.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/infracluster/openstack.go (2)
pkg/controllers/infracluster/infracluster_controller.go (1)
InfraClusterController(74-83)e2e/framework/framework.go (1)
CAPINamespace(14-14)
🔇 Additional comments (7)
pkg/controllers/infracluster/openstack.go (7)
1-51: LGTM!The package declaration, imports, and sentinel error variables are well-structured and appropriate for OpenStack infrastructure cluster management.
95-98: LGTM – LoadBalancer type validation is appropriate.Validating that only
LoadBalancerTypeOpenShiftManagedDefaultis supported ensures the subnet discovery heuristic (which relies on this load balancer type) will work correctly.
145-148: LGTM – Error message correctly identifies network client creation.The error message accurately reflects that a network client (not compute) is being created. This addresses a previous review concern.
150-176: LGTM – Network discovery and cluster creation logic is sound.The flow correctly discovers the default subnet from control plane machines, infers the router, and populates the OpenStackCluster spec before creating it. The comment at line 164-166 helpfully explains why ExternalNetworkID is set.
178-219: LGTM – Router discovery logic is correct.The function correctly finds the router by locating the port with the subnet's gateway IP, extracting the router ID, and validating the external gateway. The error handling covers edge cases (no ports, multiple ports, missing gateway).
245-312: LGTM – Subnet discovery heuristic is sound.The logic correctly iterates through control plane machines, retrieves their OpenStack ports, examines associated subnets, and identifies the subnet whose CIDR contains the API server internal IP. The nested iteration is acceptable given the small scale of control plane resources.
121-124: Verify hard-coded IdentityRef is correct for all OpenStack deployments.The IdentityRef name and CloudName are hard-coded to "openstack-cloud-credentials" and "openstack" respectively. Confirm these values are consistent across all supported OpenStack environments, or if they should be sourced from configuration.
|
/hold I am re-testing this locally. |
|
/unhold All looks good locally. |
mandre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
Scheduling tests matching the |
|
/test e2e-openstack-capi-techpreview |
1 similar comment
|
/test e2e-openstack-capi-techpreview |
|
@stephenfin it looks like that job has been very red for a while now: https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-openshift-cluster-capi-operator-main-e2e-openstack-capi-techpreview Might be worth having a look if you can |
|
/test e2e-openstack-ovn-techpreview I am still working on the fix for the |
|
/test e2e-openstack-capi-techpreview |
|
/override e2e-azure-capi-techpreview We need this PR in to unblock the sync of github.com/openshift/cluster-api-provider-openstack with upstream this cycle. The azure and gcp issues are known and unrelated to this change. Let's get this in. |
|
@stephenfin: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@stephenfin: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/override ci/prow/e2e-azure-capi-techpreview |
|
@stephenfin: Overrode contexts on behalf of stephenfin: ci/prow/e2e-azure-capi-techpreview, ci/prow/e2e-gcp-ovn-techpreview DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@stephenfin: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This currently lives out-of-tree in the
openshiftdirectory of openshift/cluster-api-provider-openstack. While there were good reasons to pursue this approach initially, no one else has followed suit, leaving us (OpenStack) as the odd ones out. The current model also introduces dependency issues due to conflicts between the requires of the InfraCluster controller, CCAPIO, and CAPO itself.The solution is to move the controller in-tree. This is broadly modeled on the existing controller - with some tweaks to align with the existing controllers. Reviewers will likely find it easiest to open the new controller module alongside the legacy one for visual diffing.
e2e tests will be added once this approach is confirmed as reasonable.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.