core: refactor lbaas util functions by kayrus · Pull Request #3075 · kubernetes/cloud-provider-openstack

kayrus · 2026-02-26T12:26:33Z

What this PR does / why we need it:

This is an experimental code refactoring using Claude AI as a development assistant. Each change was discussed and reviewed by me during the development process, it took about 6h in total. The refactoring reduces code duplication, fixes several bugs (missing metrics, potential panics, double observations), and optimizes API call patterns while preserving all existing functionality. All tests pass without modification. @zetaab @dulek @stephenfin I'd like you to review this as well. What do you think about such AI-assisted PRs?

Below is AI summary:

Key improvements

Reduced code duplication (-118 lines, ~15% reduction) - Extracted common patterns into reusable generic helpers:
- executeAndWaitActive / executeExtractAndWaitActive - Centralizes metrics tracking, error handling, and LB wait logic
- list / listWithUniqueResult - Standardizes pagination with early-exit optimization
- getSingleResource - Consistent single-resource retrieval with metrics
Fixed bugs and inconsistencies:
- Fixed missing metrics observation in 5 functions (GetPools, GetL7policies, GetL7Rules, GetListenersByLoadBalancerID, getOctaviaVersion extraction error)
- Fixed double metrics observation bug in DeleteListener
- Eliminated redundant check in GetPoolByName
Performance optimization:
- GetPoolByListener now uses direct listener Get instead of listing all pools (major API call reduction)
- listWithUniqueResult implements early-exit when multiple results detected on first page
Improved consistency:
- Centralized NotFound error handling for all delete operations
- Standardized error message format across all operations
- Unified function documentation style (following AWS SDK Go conventions)
- Consistent metrics tracking across all operations
Better organization:
- Moved UpdatePool and DeletePool to correct section (Pool Operations, not Pool Member Operations)
- Added clear section separators
- Fixed function naming (GetMembersbyPool → GetPoolMembers)

Testing

All existing unit tests pass without modification
No functional changes - pure refactoring
Verified with go test ./pkg/util/openstack/...

Files changed

pkg/util/openstack/loadbalancer.go - Main refactoring

Special notes for reviewers:

Release note:

NONE

k8s-ci-robot · 2026-02-26T12:26:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kayrus for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gouthampacha · 2026-03-18T20:00:24Z

Hi @kayrus:

I don't have +2 powers here, so I'm just exercising my review judgement as a long time OpenStack maintainer and sharing my 0.02 cents of opinion.

I think the intent to address bugs is good, but, i'd like to suggest a different approach. You should break this down into multiple commits - get Claude to do it. I know we don't apply the same commit standards here as we do in OpenStack or core Kubernetes projects; but, for the sake of maintainability, seeing single-intent commits is preferred.

stephenfin · 2026-03-20T14:38:27Z

Hi @kayrus:

I don't have +2 powers here, so I'm just exercising my review judgement as a long time OpenStack maintainer and sharing my 0.02 cents of opinion.

I think the intent to address bugs is good, but, i'd like to suggest a different approach. You should break this down into multiple commits - get Claude to do it. I know we don't apply the same commit standards here as we do in OpenStack or core Kubernetes projects; but, for the sake of maintainability, seeing single-intent commits is preferred.

I asked for the same thing 😅 Must be an OpenStack thing...

k8s-triage-robot · 2026-06-18T15:10:33Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

valeryia-hurynovich · 2026-07-01T11:00:12Z

I see that the bug fixes and refactoring are deeply intertwined. The bugs (missing metrics observations) are fixed implicitly via the new helpers, so there's no standalone commit of fixed metrics possible without either the helpers or inline fixes. However it is possible to split this logically via commits if it's important to split this anyway. I can propose to split these fixes into 5 or 6 logical commits:

Bug fixes (standalone)
Renaming of functions
Extract generic helpers
Refactor existing functions to use the helpers
Performance
Code organization

valeryia-hurynovich · 2026-07-01T13:33:31Z

@kayrus I was thinking about that this PR has some general independent parts:

renaming of GetMembersbyPool to GetPoolMembers
The GetPoolByListener performance optimization
Code organization as it written down in explanation of this PR

Splitting those out while keeping the bug-fix/helper-extraction/refactor as one atomic commit would reduce the noise in review without fragmenting what can't actually be separated, so I think that split this into some general commits will be feasible and it can be not a broken glass if just to separate some things, it's not a big deal here
I'd propose to split it like:

Rename GetMembersbyPool to GetPoolMembers (affects 4 files)
Performance GetPoolByListener
Code organization: section separators, doc comments, constant annotations
Core: bug fixes + helper extraction + refactor together (the atomic part that can't be split)

@gouthampacha would like to ask just how much reasonable to split it into clear commits?
Otherwise, I consider it's not like a big deal to split this if it's worth to be

kubernetes-prow · 2026-07-07T09:45:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign stephenfin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kayrus · 2026-07-07T09:48:27Z

I left only the code improvement with new helpers. @zetaab @gouthampacha @stephenfin please give this PR a try one more time.

gouthampacha

/lgtm

This works for me, but, a couple of comments inline that could be addressed if you make another patch

yahor-kurachkin

Nice work! LGTM

valeryia-hurynovich

LGTM

kayrus · 2026-07-16T13:09:14Z

@jihyun-huh @gouthampacha @stephenfin @zetaab a few improvements were made (by me, not AI :) ). please review one more time.

gouthampacha · 2026-07-23T05:41:25Z

/lgtm

minor issue with returning a stale object in executeExtractAndWaitActive remains

kayrus · 2026-07-23T07:45:45Z

minor issue with returning a stale object in executeExtractAndWaitActive remains

it is fixed, see line 190

anguslees

/lgtm

Next time, however - a PR like this is very hard to review. You have mixed unrelated no-op code cleanups across multiple areas with real bug fixes. Please don't do this again, since if we have to roll back an unexpected behaviour change, then we're also rolling back all these other improvements.

Using AI for this type of cleanup is a great idea. Easier code gen means even more reason to optimise for review however, not the reverse.

kayrus · 2026-07-30T07:11:14Z

@anguslees I understand the concern about mixing changes. The refactoring was necessary because the new helper function needed to be integrated across multiple functions, I could have introduced the helper in one PR, then applied it to each function in dozens of follow-up PRs. However, that approach wouldn't scale and would look bizarre.

zetaab · 2026-08-08T16:03:16Z

+	mc := metrics.NewMetricContext(resourceType, operation)
+	result, err := fn()
+	if mc.ObserveRequest(err) != nil {
+		return result, fmt.Errorf("failed to %s %s: %v", operation, resourceType, err)


instead of %v, we should use %w

zetaab · 2026-08-08T16:03:36Z

+		if operation == "delete" && cpoerrors.IsNotFound(err) {
+			klog.V(2).Infof("%s was already deleted", resourceType)
+		} else {
+			return result, fmt.Errorf("failed to %s %s on load balancer %s: %v", operation, resourceType, lbID, err)


instead of %v, we should use %w

zetaab · 2026-08-08T16:03:43Z

+		}
+	}
+	if _, err := WaitActiveAndGetLoadBalancer(ctx, client, lbID); err != nil {
+		return result, fmt.Errorf("failed to wait for load balancer %s ACTIVE after %s %s: %v",


instead of %v, we should use %w

zetaab · 2026-08-08T16:05:08Z

-		if cpoerrors.IsNotFound(err) {
-			return nil, cpoerrors.ErrNotFound
-		}
+	listener, err := listWithUniqueResult(ctx, "loadbalancer_listener", "list", listeners.List(client, listOpts), listeners.ExtractListeners)


this is actually now hardfail if not found, we are not checking ErrNotFound error at all

kubernetes-prow · 2026-09-04T17:55:08Z

@kayrus: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cloud-provider-openstack-test	`73c265a`	link	true	`/test pull-cloud-provider-openstack-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 26, 2026

k8s-ci-robot requested review from anguslees and stephenfin February 26, 2026 12:26

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 26, 2026

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2026

kayrus force-pushed the os-utils-refactor branch from 3d13181 to ab2defc Compare March 12, 2026 14:42

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2026

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 18, 2026

kubernetes-prow Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 1, 2026

kayrus force-pushed the os-utils-refactor branch from ab2defc to 769ff53 Compare July 7, 2026 09:45

jihyun-huh reviewed Jul 7, 2026

View reviewed changes

Comment thread pkg/util/openstack/loadbalancer.go

Comment thread pkg/util/openstack/loadbalancer.go Outdated

yahor-kurachkin reviewed Jul 8, 2026

View reviewed changes

Comment thread pkg/util/openstack/loadbalancer.go

gouthampacha approved these changes Jul 9, 2026

View reviewed changes

Comment thread pkg/util/openstack/loadbalancer.go Outdated

Comment thread pkg/util/openstack/loadbalancer.go Outdated

kubernetes-prow Bot assigned gouthampacha Jul 9, 2026

kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2026

core: refactor lbaas util functions

b58f193

kayrus force-pushed the os-utils-refactor branch from 769ff53 to 5545247 Compare July 16, 2026 11:15

kubernetes-prow Bot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2026

yahor-kurachkin approved these changes Jul 16, 2026

View reviewed changes

code review

7cee2ef

kayrus force-pushed the os-utils-refactor branch from 5545247 to 7cee2ef Compare July 16, 2026 11:37

valeryia-hurynovich reviewed Jul 16, 2026

View reviewed changes

Comment thread pkg/util/openstack/loadbalancer.go Outdated

kayrus force-pushed the os-utils-refactor branch 2 times, most recently from f88dd2d to 0a21e49 Compare July 16, 2026 12:54

valeryia-hurynovich approved these changes Jul 16, 2026

View reviewed changes

kayrus force-pushed the os-utils-refactor branch from 0a21e49 to 93b8986 Compare July 16, 2026 13:26

kayrus commented Jul 16, 2026

View reviewed changes

Comment thread pkg/util/openstack/loadbalancer.go Outdated

kayrus added 5 commits July 17, 2026 13:34

rework listWithUniqueResult

185176b

rework getSingleResource func

e276116

rework DeleteLoadbalancer func

8b79feb

rework executeExtractAndWaitActive

e554068

add context to wait helpers

73c265a

kayrus force-pushed the os-utils-refactor branch from 93b8986 to 73c265a Compare July 17, 2026 11:34

kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2026

anguslees approved these changes Jul 30, 2026

View reviewed changes

kubernetes-prow Bot assigned anguslees Jul 30, 2026

zetaab requested changes Aug 8, 2026

View reviewed changes

Uh oh!

Conversation

kayrus commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key improvements

Testing

Files changed

Uh oh!

k8s-ci-robot commented Feb 26, 2026

Uh oh!

gouthampacha commented Mar 18, 2026

Uh oh!

stephenfin commented Mar 20, 2026

Uh oh!

k8s-triage-robot commented Jun 18, 2026

Uh oh!

valeryia-hurynovich commented Jul 1, 2026

Uh oh!

valeryia-hurynovich commented Jul 1, 2026

Uh oh!

kubernetes-prow Bot commented Jul 7, 2026

Uh oh!

kayrus commented Jul 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gouthampacha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yahor-kurachkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valeryia-hurynovich left a comment

Choose a reason for hiding this comment

Uh oh!

kayrus commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gouthampacha commented Jul 23, 2026

Uh oh!

kayrus commented Jul 23, 2026

Uh oh!

anguslees left a comment

Choose a reason for hiding this comment

Uh oh!

kayrus commented Jul 30, 2026

Uh oh!

zetaab Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

zetaab Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

zetaab Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

zetaab Aug 8, 2026

Choose a reason for hiding this comment

Uh oh!

kubernetes-prow Bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

kayrus commented Feb 26, 2026 •

edited

Loading

kayrus commented Jul 16, 2026 •

edited

Loading