Skip to content
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

add logging in NEG syncer #1155

Merged
merged 1 commit into from
Jun 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions pkg/neg/syncers/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,18 +155,18 @@ func (s *transactionSyncer) syncInternal() error {
if err != nil {
return err
}
s.logStats(currentMap, "current NEG endpoints")

// Merge the current state from cloud with the transaction table together
// The combined state represents the eventual result when all transactions completed
mergeTransactionIntoZoneEndpointMap(currentMap, s.transactions)
s.logStats(currentMap, "after in-progress operations have completed, NEG endpoints")

targetMap, endpointPodMap, err := s.endpointsCalculator.CalculateEndpoints(ep.(*apiv1.Endpoints), currentMap)
s.logStats(targetMap, "desired NEG endpoints")

// Calculate the endpoints to add and delete to transform the current state to desire state
addEndpoints, removeEndpoints := calculateNetworkEndpointDifference(targetMap, currentMap)
if s.NegType == negtypes.VmIpEndpointType && len(removeEndpoints) > 0 {
// Make removals minimum since the traffic will be abruptly stopped. Log removals
klog.V(3).Infof("Removing endpoints %+v from GCE_VM_IP NEG %s", removeEndpoints, s.negName)
}
// Calculate Pods that are already in the NEG
_, committedEndpoints := calculateNetworkEndpointDifference(addEndpoints, targetMap)
// Filter out the endpoints with existing transaction
Expand All @@ -185,7 +185,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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return s.syncNetworkEndpoints(addEndpoints, removeEndpoints)
}

Expand Down Expand Up @@ -391,3 +392,17 @@ func mergeTransactionIntoZoneEndpointMap(endpointMap map[string]negtypes.Network
}
return
}

// logStats logs aggregated stats of the input endpointMap
func (s *transactionSyncer) logStats(endpointMap map[string]negtypes.NetworkEndpointSet, desc string) {
stats := []string{}
for zone, endpointSet := range endpointMap {
stats = append(stats, fmt.Sprintf("%d endpoints in zone %q", endpointSet.Len(), zone))
}
klog.V(3).Infof("For NEG %q, %s: %s.", s.negName, desc, strings.Join(stats, ","))
}

// 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", s.negName, desc, endpointMap)
}
10 changes: 9 additions & 1 deletion pkg/neg/syncers/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ func toZoneNetworkEndpointMap(endpoints *apiv1.Endpoints, zoneGetter negtypes.Zo
klog.Errorf("Endpoint object is nil")
return zoneNetworkEndpointMap, networkEndpointPodMap, nil
}

var foundMatchingPort bool
for _, subset := range endpoints.Subsets {
matchPort := ""
// service spec allows target Port to be a named Port.
Expand All @@ -193,6 +193,7 @@ func toZoneNetworkEndpointMap(endpoints *apiv1.Endpoints, zoneGetter negtypes.Zo
if len(matchPort) == 0 {
continue
}
foundMatchingPort = true

// processAddressFunc adds the qualified endpoints from the input list into the endpointSet group by zone
processAddressFunc := func(addresses []v1.EndpointAddress, includeAllEndpoints bool) error {
Expand Down Expand Up @@ -244,6 +245,13 @@ func toZoneNetworkEndpointMap(endpoints *apiv1.Endpoints, zoneGetter negtypes.Zo
return nil, nil, err
}
}
if !foundMatchingPort {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a log at line 209 as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added another log line when the output is empty.

klog.Errorf("Service port name %q was not found in the endpoints object %+v", servicePortName, endpoints)
}

if len(zoneNetworkEndpointMap) == 0 || len(networkEndpointPodMap) == 0 {
klog.V(3).Infof("Generated empty endpoint maps (zoneNetworkEndpointMap: %+v, networkEndpointPodMap: %v) from Endpoints object: %+v", zoneNetworkEndpointMap, networkEndpointPodMap, endpoints)
}
return zoneNetworkEndpointMap, networkEndpointPodMap, nil
}

Expand Down