-
Notifications
You must be signed in to change notification settings - Fork 181
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
ARO-13789 Add support for deleting LB probes #4111
base: master
Are you sure you want to change the base?
Conversation
@dmichellis please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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.
Deleting line 96 should fix the error.
Running make lint-go
may help test locally to brush off before PR opening.
|
||
func (a *azureActions) deleteLoadbalancerProbeConfiguration(ctx context.Context, resourceID string) error { | ||
// For further details on why this was added, please see JIRA ticket [ARO-13789](https://issues.redhat.com/browse/ARO-13789) | ||
idParts := strings.Split(resourceID, "/") |
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.
Maybe add a check for nil/empty vars here?
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.
The Azure SDK for Go has a function that will make this easier and safer: https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm#ParseResourceID
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.
I found a bug in the unit tests and made some other small recommendations, but the core functionality LGTM 👍🏻
|
||
func (a *azureActions) deleteLoadbalancerProbeConfiguration(ctx context.Context, resourceID string) error { | ||
// For further details on why this was added, please see JIRA ticket [ARO-13789](https://issues.redhat.com/browse/ARO-13789) | ||
idParts := strings.Split(resourceID, "/") |
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.
The Azure SDK for Go has a function that will make this easier and safer: https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm#ParseResourceID
@@ -24,6 +24,24 @@ func RemoveFrontendIPConfiguration(lb *mgmtnetwork.LoadBalancer, resourceID stri | |||
lb.LoadBalancerPropertiesFormat.FrontendIPConfigurations = &newFrontendIPConfig | |||
return nil | |||
} | |||
func RemoveLoadbalancerProbeConfiguration(lb *mgmtnetwork.LoadBalancer, resourceID string) error { | |||
newProbeConfiguration := make([]mgmtnetwork.Probe, 0) | |||
for _, probe := range *lb.LoadBalancerPropertiesFormat.Probes { |
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.
Nit: Although it's very unlikely for this to be nil, I think it would be worth checking to avoid panics.
name: "Fail to remove probe-2 because it still has references", | ||
probeConfig: "/subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/clusterRG/providers/Microsoft.Network/loadBalancers/infraID/probes/probe-2", | ||
currentLB: originalLB, | ||
expectedLB: expectedLB, |
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.
We would actually want this to be the original LB - if we try and fail to delete probe 2, both probes should still be left on the load balancer.
The reason the test cases pass as-is is because the originalLB
's probes are stored in a pointer, so when we specify currentLB: originalLB
in both test cases, the same underlying slice of probes is being used in both test cases. Thus, the first test case removes probe 1, and this change carries over to the second test case. You can validate that this is the case by replacing originalLB
with a hard-coded load balancer struct in both test cases - the second test case fails.
I recommend hard-coding the Azure SDK for Go resource structs in each test case to avoid these types of issues. I know it's verbose, but it avoids confusing bugs and makes each test case clearer.
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-13789
Adds deletion for LB probes
What this PR does / why we need it:
This addresses an issue that happened during an incident (TODO: more info pending) where upgrades were stuck, and some restarts were required to complete the upgrade. Resources were left orphaned and couldn't be deleted.
Test plan for issue:
There are unit tests for both API calls and the loadbalancer logic in the RP
Is there any documentation that needs to be updated for this PR?
TODO: organise the information about the incident
How do you know this will function as expected in production?
Adding new special-case resource deletion, like FrontendIP, that only exists as references in the loadbalancer configuration.