MULTIARCH-4633: Power VS: Configure load balancers for the private scenario #8331
Conversation
mjturek
force-pushed
the
private-lb
branch
5 times, most recently
from
0dd50bd
to
cde97b4
Compare
mjturek
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mjturek: This pull request references MULTIARCH-4633 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.16.0" version, but no target version was set. In 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. |
|
/lgtm |
|
/assign r4f4 |
| // InfraReady is called once cluster.Status.InfrastructureReady | ||
| // is true, typically after load balancers have been provisioned. It can be used | ||
| // to create DNS records. | ||
| // nolint:gocyclo |
There was a problem hiding this comment.
Maybe the linter has a point here and it's time to start breaking this functionality down into separate functions.
There was a problem hiding this comment.
yep I'm working on it
There was a problem hiding this comment.
@r4f4 are you okay with me refactoring in this PR?
There was a problem hiding this comment.
@mjturek sure. Just please break it down into commits to make the life of a fellow reviewer a bit easier :)
I also wouldn't mind if you add the //nolint: now and do the break down in a follow-up (as long as it's done).
There was a problem hiding this comment.
sounds good, let me address your other comments and I'll finish up the break down in a follow up patch this week that removes the //nolint:
There was a problem hiding this comment.
Cleanup patch is here #8337 it's based on this one so theoretically we could merge it in one go. Up to you!
| existingSubnets, err := installConfig.PowerVS.GetVPCSubnets(context.TODO(), vpc) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, subnet := range existingSubnets { | ||
| vpcSubnets = append(vpcSubnets, *subnet.Name) | ||
| } |
There was a problem hiding this comment.
Aren't these subnets supplied via installConfig.Config.PowerVS.VPCSubnets?
There was a problem hiding this comment.
they are but we've found the provider will fail if you don't add all of the existing subnets. We could require that you specify all the subnets through the install config, but this is a more automated approach.
There was a problem hiding this comment.
Ok. Let's add that explanation as a comment here?
| if err2 == nil { | ||
| return true, nil | ||
| } | ||
| return false, err2 |
There was a problem hiding this comment.
ExponentialBackoff immediately returns an error if the callback returns an error. So this whole backoff call is useless here. You only need it in case you inspect the error and decide to retry the call in certain conditions.
There was a problem hiding this comment.
I believe the intent here is to wrap a function that returns only error to one that returns bool, error as ExponentialBackoffWithContext wants. We can alter the client functions to return bool, error if you'd like.. Is there something else you're suggesting?
There was a problem hiding this comment.
That's not what I mean. ExponentialBackoff is to be used when you want to retry a functional call (with exponential backoff). In your case, you're never retrying anything because you either stop after the first success (return true, nil) or you stop with the first failure (return false, err2). You can remove the whole ExponentialBackoff and just call client.CreateDNSRecord and you'll have the exact same behaviour.
So unless there is an error condition when you want to return false, nil, the ExponentialBackoff is useless.
There was a problem hiding this comment.
https://pkg.go.dev/k8s.io/apimachinery/pkg/util/wait#ExponentialBackoffWithContext
ExponentialBackoffWithContext repeats a condition check with exponential backoff. It immediately returns an error if the condition returns an error, [...]. If an error is returned by the condition the backoff stops immediately.
There was a problem hiding this comment.
ahhhhh I had thought it would retry until returning true
There was a problem hiding this comment.
you indeed are right - I've removed the backoff bits.
mjturek
force-pushed
the
private-lb
branch
2 times, most recently
from
b5d598f
to
6742b73
Compare
|
After talking with @r4f4 we decided it would be better to fix the retry rather than remove it! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: r4f4 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
/test e2e-aws-ovn |
|
@mjturek: The following test failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit def8d5d
into
openshift:master
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-installer-altinfra-container-v4.16.0-202405031950.p0.gdef8d5d.assembly.stream.el9 for distgit ose-installer-altinfra. |
In the private scenario, both
apiandapi-intwill point to the private loadbalancer. Update the DNS creation call back to support this.