Add more logs to L4 NetLB #1845
Conversation
- Remove unused "version" argument - Remove no-op type conversions
Add more logging in deletion with measuring time taken
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
panslava
force-pushed
the
debug-logs
branch
2 times, most recently
from
91636e2 to
4bf99f8
Compare
|
/assign cezarygerard |
|
@panslava: GitHub didn't allow me to request PR reviews from the following users: code-elinka. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. 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/test-infra repository. |
| @@ -216,26 +204,52 @@ func (l4hc *l4HealthChecks) ensureHealthCheck(hcName string, svcName types.Names | |||
| // | |||
| // L4 ILB and L4 NetLB Services with ExternalTrafficPolicy=Cluster use the same firewall | |||
| // rule at global scope. | |||
| func (l4hc *l4HealthChecks) ensureIPv4Firewall(svc *corev1.Service, hcFwName string, hcPort int32, sharedHC bool, nodeNames []string) error { | |||
| func (l4hc *l4HealthChecks) ensureIPv4Firewall(svc *corev1.Service, namer namer.L4ResourcesNamer, hcPort int32, isSharedHC bool, nodeNames []string, hcResult *EnsureHealthCheckResult) { | |||
There was a problem hiding this comment.
I like the old way of returning the error more than not returning it.
Pros of the old approach:
- easier to test
- you can customize log messages based on error
Cons:
- you need to check for an error everywhere, where you use this function
There was a problem hiding this comment.
I agree, I also like returning errors...
I find the main problem is -- we use single EnsureHealthCheckResult for storing too much stuff. We should not store error there. Even if we want more context about errors (like now we use GceResourceInError), we should just create custom error type
But changing this will require changing too much logic...
hcResult also stores HCFirewallRuleName, that's why I passed it here, cause I don't want to create firewallName on the level above...
There was a problem hiding this comment.
I see. Yes, I tried to find a nice way to handle it and I would rather keep the error logic inside of the function. Thank you for the reasoning 👍
|
LGTM. Thank you for this PR. I really like the last 2 commits. For first 2 refactoring commits: I think we could have much cleaner code with more linters :D |
|
/cc aojea |
| start := time.Now() | ||
| klog.V(2).Infof("EnsureL4BackendService(%v, %v, %v): started", name, scheme, protocol) | ||
| defer klog.V(2).Infof("EnsureL4BackendService(%v, %v, %v): finished, time taken: %v", name, scheme, protocol, time.Since(start)) | ||
|
|
||
| klog.V(2).Infof("EnsureL4BackendService(%v, %v, %v): checking existing backend service", name, scheme, protocol) |
There was a problem hiding this comment.
this log is now redundant
There was a problem hiding this comment.
Not really, we can remove, but it gives info about what step is happening (checking existing backend service)
|
@cezarygerard thanks for review, I fixed everything |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cezarygerard, panslava 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 |
This should not introduce any change in logic, but involves some refactoring
I've tried to split it by multiple commits