SDK2: Ensure service endpoints track2 #3885
Conversation
bitoku
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
SrinivasAtmakuri,
mociarain,
kimorris27,
tsatam and
fahlmant
as code owners
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
One question though: how does this address the cluster install failure? |
|
We have seen some install failures about authorization in this step. We can wrap the step with By updating the old SDK to the new one, we can use built-in retry strategy in the new SDK. Aside from the CIF, we should update old SDKs anyway. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
We have seen some install failures about authorization in this step. It should be caused by the AAD propagation delay.
We can wrap the step with
AuthorizationRetryingAction, but it will retry the whole step again. Because we don't make steps idempotent, retrying the whole step might cause different issues.By updating the old SDK to the new one, we can use built-in retry strategy in the new SDK. This will retry just the failed API, so it's a safer way than
AuthorizationRetryingAction.Aside from the CIF, we should update old SDKs anyway.
Oh, cool. I didn't realize the new SDK clients had that built-in retry strategy. That makes sense!
|
Please rebase pull request. |
…nts-track2 # Conflicts: # pkg/cluster/cluster.go
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| userAssignedIdentities armmsi.UserAssignedIdentitiesClient | ||
|
|
||
| dns dns.Manager | ||
| storage storage.Manager | ||
| subnet subnet.Manager | ||
| subnet subnet.Manager // TODO: use armSubnet instead. |
| for _, endpoint := range endpoints { | ||
| endpointFound, serviceEndpointPtr := subnetContainsEndpoint(subnet, endpoint) | ||
|
|
||
| if !endpointFound || *serviceEndpointPtr.ProvisioningState != armnetwork.ProvisioningStateSucceeded { |
There was a problem hiding this comment.
Can ProvisioningState ever be in an error state? We wouldn't want to add it then I'm guessing
Which issue this PR addresses:
Fixes https://issues.redhat.com/browse/ARO-4665 and https://issues.redhat.com/browse/ARO-7316
What this PR does / why we need it:
This PR replaces old SDK to the new one.
Test plan for issue:
ci, e2e
Is there any documentation that needs to be updated for this PR?
no
How do you know this will function as expected in production?
Creating a cluster can verify the change.
It can be checked in e2e.