add logging in NEG syncer #1155
Conversation
| @@ -244,6 +245,9 @@ func toZoneNetworkEndpointMap(endpoints *apiv1.Endpoints, zoneGetter negtypes.Zo | |||
| return nil, nil, err | |||
| } | |||
| } | |||
| if !foundMatchingPort { | |||
There was a problem hiding this comment.
Can we add a log at line 209 as well?
There was a problem hiding this comment.
+1 to that. Also, we can move this log line to line 194? At that point, we know foundMatchingPort is false right? Or if we want this approach, we need to reset foundMatchingPort to false in line 250.
There was a problem hiding this comment.
Please disregard my comment about moving the log to a different line. It makes sense to keep it the way you have it - we only want this log if no endpoints had the matching port.
There was a problem hiding this comment.
Added another log line when the output is empty.
pkg/neg/syncers/transaction.go
Outdated
|
|
||
| targetMap, endpointPodMap, err := s.endpointsCalculator.CalculateEndpoints(ep.(*apiv1.Endpoints), currentMap) | ||
| s.logStats(currentMap, "desired NEG endpoints") |
| @@ -244,6 +245,9 @@ func toZoneNetworkEndpointMap(endpoints *apiv1.Endpoints, zoneGetter negtypes.Zo | |||
| return nil, nil, err | |||
| } | |||
| } | |||
| if !foundMatchingPort { | |||
There was a problem hiding this comment.
+1 to that. Also, we can move this log line to line 194? At that point, we know foundMatchingPort is false right? Or if we want this approach, we need to reset foundMatchingPort to false in line 250.
| @@ -185,7 +189,8 @@ func (s *transactionSyncer) syncInternal() error { | |||
| klog.V(4).Infof("No endpoint change for %s/%s, skip syncing NEG. ", s.Namespace, s.Name) | |||
| return nil | |||
| } | |||
|
|
|||
| s.logEndpoints(addEndpoints, "adding endpoint") | |||
| s.logEndpoints(removeEndpoints, "removing endpoint") | |||
There was a problem hiding this comment.
This would log endpoints twice in case of VM_IP NEGs, see line 172. Maybe we can remove line 172 and log include s.NegType in this function.
pkg/neg/syncers/transaction.go
Outdated
| func (s *transactionSyncer) logEndpoints(endpointMap map[string]negtypes.NetworkEndpointSet, desc string) { | ||
| for zone, endpointSet := range endpointMap { | ||
| for _, endpoint := range endpointSet.List() { | ||
| klog.V(3).Infof("For NEG %q, %s: %+v to zone %q", s.negName, desc, endpoint, zone) |
There was a problem hiding this comment.
Can we log the entire map instead of each endpoint? The map key will have the zone and the value will be the list of endpoints?
pkg/neg/syncers/transaction.go
Outdated
|
|
||
| // logEndpoints logs individual endpoint in the input endpointMap | ||
| func (s *transactionSyncer) logEndpoints(endpointMap map[string]negtypes.NetworkEndpointSet, desc string) { | ||
| klog.V(3).Infof("For NEG %q, %s: %+v", endpointMap) |
There was a problem hiding this comment.
Let's include s.negName and s.NegType
There was a problem hiding this comment.
The log line when the syncerInternal starts includes everything. I feel like you can reference it by searching the neg name?
There was a problem hiding this comment.
true.. this log line needs more params though. Maybe you meant to include "desc" and s.negName? It might be nice to see the negType here too, but we can lookup from name as well.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, 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 |
Cherrypicking #1155 into release 1.9
No description provided.