Conversation
|
/retest |
eshulman2
force-pushed
the
add_lb
branch
8 times, most recently
from
d616eb6 to
4a22540
Compare
winiciusallan
left a comment
winiciusallan
left a comment
There was a problem hiding this comment.
Hi @eshulman2, that was a huge job, huh? I've just made a small round of review, and I'll take a look at the missing files this week.
Let me know what you think about the comments.
| return progress.WrapError(err) | ||
| } | ||
|
|
||
| func (actuator loadbalancerActuator) updateResource(ctx context.Context, obj orcObjectPT, osResource *osResourceT) progress.ReconcileStatus { |
There was a problem hiding this comment.
I'm wondering if we need to check for the Loadbalancer status before performing an update operation, since we can't update the load balancer if it has a PENDING_UPDATE status. I think the controller itself will retry the update when checking the NeedsRefresh. Am I missing something?
There was a problem hiding this comment.
As far as I understand the code it will be a terminal error on the 409 (conflict), I'll add a similar check to the one in the delete to avoid going into terminal error when getting conflict. Additionally I believe we should consider if 409 should be a terminal error for every 429 or if we should add a mechanism to respond differently to different 409 errors.
There was a problem hiding this comment.
IIRC, this topic about handling different 409 errors was discussed in #667, and I like the "resync" solution.
As long as I have experienced, load balancers, especially using the amphora driver, tend to break at a certain frequency, and sometimes they get stuck in PENDING_UPDATE, for example. This likely needs external intervention and a resync will be very useful, since we can't solve this via a change in the spec, consider this as a terminal would not be good, I believe.
There was a problem hiding this comment.
I added a similar section to the delete section to make sure we are waiting intermediate status'es. in this case (unlike the quota issue) I believe the rsync is not the right solution here as it is a real "waiting on openstack" scenario so returning waiting on openstack status I believe is a fair approach.
internal/controllers/loadbalancer/tests/loadbalancer-create-minimal/00-assert.yaml
Show resolved
Hide resolved
internal/controllers/loadbalancer/tests/loadbalancer-create-minimal/00-assert.yaml
Show resolved
Hide resolved
eshulman2
force-pushed
the
add_lb
branch
2 times, most recently
from
2e5cc7e to
c0196ef
Compare
|
Failed to assess the semver bump. See logs for details. |
- allow testing octavia by enabling it in the e2e job - increase vm qouta to allow e2e to run
$ go run ./cmd/scaffold-controller -interactive=false -kind=LoadBalancer -gophercloud-client=NewLoadBalancerV2 -gophercloud-module=github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers -optional-create-dependency Subnet -optional-create-dependency Network -optional-create-dependency Port -optional-create-dependency Flavor -optional-create-dependency Project
Add support for OpenStack Octavia Load Balancer resources. This includes: - LoadBalancer CRD with support for VIP subnet/network/port references - Controller with create, update, delete, and import capabilities - Status reporting with provisioning and operating status - Dependency resolution for Subnet, Network, Port, and Project references - Kuttl tests for create, update, import, and dependency scenarios Closes k-orc#619
|
@mandre can you please re-trigger the failed job? |
2 participants
Add support for OpenStack Octavia Load Balancer resources. This includes: