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
Handle NotImplemented error in service_controller. #80660
Conversation
/assign @bowei |
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.
/lgtm
/retest |
@@ -339,6 +339,10 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st | |||
} | |||
newStatus, err = s.ensureLoadBalancer(service) | |||
if err != nil { | |||
if err == cloudprovider.NotImplemented { |
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 NotImplemented error is an already existing error which cloud providers outside of tree may be using. I do not think we should ignore it here. If we want an error to indicate the functionality is implemented elsewhere lets generate a new error for that case.
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.
ack, will add a new error.
Adding a new error "ImplementedElsewhere" to cloud.go. This error indicates that implementation for a particular service/loadbalancer spec is handled by a different controller. The caller can ignore the error and skip modifying service status upon receiving this error.
Added error check to EnsureLoadBalancerDeleted.
Depending on how the cluster is created. Test clusters set a default level of 4.
Removed handling ImplementedElsewhere error in call to EnsureLoadBalancerDeleted.
@@ -339,6 +339,11 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st | |||
} | |||
newStatus, err = s.ensureLoadBalancer(service) | |||
if err != nil { | |||
if err == cloudprovider.ImplementedElsewhere { | |||
klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", |
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.
minor: we usually don't break newline inside statement.
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.
done
@@ -703,7 +708,12 @@ func (s *ServiceController) lockedUpdateLoadBalancerHosts(service *v1.Service, h | |||
} | |||
return nil | |||
} | |||
|
|||
if err == cloudprovider.ImplementedElsewhere { | |||
// ImplementedElsewhere indicates that the UpdateLoadBalancer is a nop and the |
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.
Don't you need the comment above as well.
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.
done
/ok-to-test |
/priority important-soon |
/approve |
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, cheftako, prameshj 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 |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
What this PR does / why we need it:
This PR adds support in service_controller to handle NotImplemented errors when creating a LoadBalancer. CloudProviders using alternate controllers to implement LoadBalancer services or choosing to skip creation of LoadBalancer based on service annotations can now return a NotImplemented error. service_controller will ignore the error without generating an event and will also not modify the Service status in any way.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: