Compute number of L4 ILBs in error and success state#1176
Compute number of L4 ILBs in error and success state#1176k8s-ci-robot merged 1 commit intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
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. DetailsInstructions 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/assign @prameshj |
|
/ok-to-test |
k8s-ci-robot
added
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.
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.
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?.
skmatti
force-pushed
the
ilb-metrics
branch
2 times, most recently
from
242d521 to
40bf2c9
Compare
|
/lgtm |
k8s-ci-robot
added
the
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.
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.
We can add new metric features and modify service state in order to achieve that.
k8s-ci-robot
removed
the
lgtm
pkg/loadbalancers/l4.go
Outdated
| } | ||
|
|
||
| var serviceState metrics.L4ILBServiceState | ||
| serviceState.InSuccess = true |
There was a problem hiding this comment.
Can we also set the service state to false in
ingress-gce/pkg/l4/l4controller.go
Line 161 in c96b1d8
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?
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/lgtm |
k8s-ci-robot
added
the
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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
5 participants
No description provided.