-
Notifications
You must be signed in to change notification settings - Fork 299
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
Compute number of L4 ILBs in error and success state #1176
Conversation
Hi @skmatti. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/assign @prameshj |
/ok-to-test |
pkg/loadbalancers/l4.go
Outdated
// Mark the service InSuccess state as false to begin with. | ||
// This will be updated to true if the VIP is configured successfully. | ||
serviceState.InSuccess = false | ||
defer func() { |
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 should be very careful what uses defer, as it's behavior is somewhat non-obvious. Is there a good reason to do this using defer?
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.
This is to update service state for metrics if this function returns an error. I added defer for code compactness.
Would it be better to add this line at each return statement?.
242d521
to
40bf2c9
Compare
/lgtm |
klog.V(6).Infof("ILB Service %s has EnabledGlobalAccess: %t, EnabledCustomSubnet: %t, InSuccess: %t", key, state.EnabledGlobalAccess, state.EnabledCustomSubnet, state.InSuccess) | ||
counts[l4ILBService]++ | ||
if !state.InSuccess { | ||
counts[l4ILBInError]++ |
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 would be useful to expand this in the future to indicate the reason of the error. So we could have "l4ILBErrorFirewall", "l4ILBErrorBackendService" etc.
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 can add new metric features and modify service state in order to achieve that.
pkg/loadbalancers/l4.go
Outdated
@@ -234,18 +242,17 @@ func (l *L4) EnsureInternalLoadBalancer(nodeNames []string, svc *corev1.Service) | |||
return nil, err | |||
} | |||
|
|||
var serviceState metrics.L4ILBServiceState | |||
serviceState.InSuccess = true |
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 we also set the service state to false in
ingress-gce/pkg/l4/l4controller.go
Line 161 in c96b1d8
return err |
It would be worth setting it in each of those error cases. Maybe we pass in a pointer to ServiceState in EnsureInternalLoadBalancer and always set it in the processServiceCreateOrUpdate function?
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, prameshj, skmatti 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 |
No description provided.