-
Notifications
You must be signed in to change notification settings - Fork 265
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
[Fix] Continue reconcile LB if error is 403/404 #4260
[Fix] Continue reconcile LB if error is 403/404 #4260
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lzhecheng 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 |
pkg/provider/azure_loadbalancer.go
Outdated
@@ -1660,7 +1660,7 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service, | |||
for i := range toDeleteConfigs { | |||
fipConfigToDel := toDeleteConfigs[i] | |||
err := az.reconcilePrivateLinkService(clusterName, service, &fipConfigToDel, false /* wantPLS */) | |||
if err != nil { | |||
if err != nil && !retry.HasStatusForbiddenOrIgnoredError(err) { |
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.
Could we skip 403/404 only for cleanup operation? For create operation, 403/404 should not be skipped.
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.
Thanks for reminding me.
Compared to last commit, I use HasStatusForbiddenOrIgnoredError
only for PLS and backend pool because I think the issue is about these resources instead of those LB's resources like FIP.
Code updated.
4bb55d0
to
00ade9c
Compare
When deleting an LB, error like 403/404 should not stop the reconcile process. Signed-off-by: Zhecheng Li <[email protected]>
00ade9c
to
4e985a9
Compare
/retest |
Why do we want to ignore 403? |
@@ -614,8 +618,11 @@ func (bi *backendPoolTypeNodeIP) ReconcileBackendPools(clusterName string, servi | |||
klog.V(2).Infof("bi.ReconcileBackendPools for service (%s) and vmSet (%s): ensuring the LB is decoupled from the VMSet", serviceName, vmSetName) | |||
shouldRefreshLB, err = bi.VMSet.EnsureBackendPoolDeleted(service, lbBackendPoolIDsSlice, vmSetName, lb.BackendAddressPools, true) | |||
if err != nil { | |||
klog.Errorf("bi.ReconcileBackendPools for service (%s): failed to EnsureBackendPoolDeleted: %s", serviceName, err.Error()) | |||
return false, false, false, err | |||
if !retry.HasStatusForbiddenOrIgnoredError(err) { |
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.
ensureBackendPoolDeleted decouples vmss and vm from backend pool instead of deleting the backend pool. I am not sure if it will return 404 when updating an non-existent vmss/vmssvm.
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 think it will return 404 when it vmssCache.Get() with non-existing vmss.
cloud-provider-azure/pkg/provider/azure_vmss.go
Line 1645 in 5d65ed3
cachedUniform, err := ss.vmssCache.Get(consts.VMSSKey, azcache.CacheReadTypeDefault) |
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.
Can you point out where the 404 can be returned? I think the LIST calls should not generate a 404 error.
if deleteErr != nil { | ||
klog.Errorf("reconcilePrivateLinkService for service(%s): deletePLS for frontEnd(%s) failed: %v", serviceName, pointer.StringDeref(fipConfigID, ""), err) | ||
return deleteErr.Error() | ||
if !retry.HasStatusForbiddenOrIgnoredError(err) { |
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.
It seems it will not return not found error if failed to get the pls by fipConfigID?
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.
Yes, it only leaves a warning. I think it is ok because it is in if !wantPLS
condition.
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.
If the condition will not met, why bother to add so many lines of code for a scenario that will never happen?
I see 404 is handled with 403, "when AuthorizationFailed is reported from Azure API", so I followed the logic. cloud-provider-azure/pkg/retry/azure_error.go Lines 305 to 308 in 3ea02d9
|
The function is designed for delete operations and is not being used anymore. |
PR needs rebase. 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. |
@lzhecheng: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. 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 kubernetes-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
[Fix] Continue reconcileLB if error is 403/404. When deleting an LB, error like 403/404 should not stop the reconcile process.
Which issue(s) this PR fixes:
Fixes #933
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: