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.
internal/controllers/loadbalancer/tests/loadbalancer-create-minimal/00-assert.yaml
Show resolved
Hide resolved
| kind: Subnet | ||
| name: loadbalancer-create-minimal | ||
| ref: subnet | ||
| assertAll: |
There was a problem hiding this comment.
You could exercise more dynamic checks here, like having an empty description. Is there a default availability zone?
There was a problem hiding this comment.
Availability zone is optional in Octavia and its value is cloud-specific - some deployments don't have LB availability zones configured at all, and when they do, the default depends on the cloud configuration. Since this is a "minimal" test that doesn't specify an availability zone, I don't think we can reliably assert a specific value here. I've added checks for description: "" and adminStateUp: true which have predictable defaults.
| [[post-config|/etc/nova/nova.conf]] | ||
| [quota] | ||
| instances = 50 |
There was a problem hiding this comment.
I didn't get why we need to change the instance quota here. Could you please explain?
There was a problem hiding this comment.
when e2e tests run in parallel in some cases the amount of created vms (lb's are also vms) exceeds the default quota so I increased it from the default to what seems to me a reasonable number
There was a problem hiding this comment.
oh, yeah, it's true. thanks for clarifying.
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: