-
Notifications
You must be signed in to change notification settings - Fork 302
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
Replace klog with context logging for NEG Controller #2297
Replace klog with context logging for NEG Controller #2297
Conversation
Hi @sawsa307. 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. |
for endpoint, namespacedName := range endpointMap { | ||
pod, exists, err := getPodFromStore(podLister, namespacedName.Namespace, namespacedName.Name) | ||
if err != nil { | ||
klog.Warningf("Failed to retrieve pod %q from store: %v", namespacedName.String(), err) |
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.
Based on discussion in #1746, I think we can use Info
or Error
. In this case, Error makes more sense to me.
f5be8ec
to
aa061ae
Compare
/assign @swetharepakula |
aa061ae
to
3938758
Compare
pkg/neg/syncers/utils.go
Outdated
return negRef, err | ||
} | ||
klog.V(4).Infof("Neg %q in zone %q was not found: %s", negName, zone, err) | ||
logger.V(4).Info("Neg in zone was not found", "negName", negName, "zone", zone, "err", err) |
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.
nit: I don't think we need to do V(4)
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.
Updated!
pkg/neg/syncers/utils.go
Outdated
recorder.Eventf(svc, apiv1.EventTypeNormal, "Delete", "Deleted NEG %q for %s in %q.", negName, negServicePortName, zone) | ||
} | ||
} | ||
} | ||
} | ||
|
||
if needToCreate { | ||
klog.V(2).Infof("Creating NEG %q for %s in %q.", negName, negServicePortName, zone) | ||
logger.V(2).Info("Creating NEG", "negName", negName, "negServicePortName", negServicePortName, "zone", zone) |
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.
nit: remove V(2)
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.
Updated!
3938758
to
60f37bd
Compare
pkg/neg/syncers/utils.go
Outdated
var negRef negv1beta1.NegObjectReference | ||
neg, err := cloud.GetNetworkEndpointGroup(negName, zone, version) | ||
if err != nil { | ||
if !utils.IsNotFoundError(err) { | ||
klog.Errorf("Failed to get Neg %q in zone %q: %s", negName, zone, err) | ||
logger.Error(err, "Failed to get Neg in zone", "negName", negName, "zone", zone) |
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.
nit: "Failed to get NEG"
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.
One optimization we can do is:
since this function is processing a given NEG, and we need to log the name and zone of this NEG in all log message in this function, we first create a logger with addition key/value pair like:
negLogger := logger.WithValues("negName", negName, "zone", zone)
And we use this as the logger of this function, which should increase readability and shorten the log message.
Same applied to toZoneNetworkEndpointMap and toZoneNetworkEndpointMapDegradedMode. Please take a look!
60f37bd
to
495db5a
Compare
/ok-to-test |
c7fe1b4
to
58d508d
Compare
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.
A few small comments, otherwise generally looks good.
Are we able to pass the logger into the API calls? specifically the GCE ones?
pkg/neg/syncers/utils.go
Outdated
return negRef, err | ||
} | ||
klog.V(4).Infof("Neg %q in zone %q was not found: %s", negName, zone, err) | ||
logger.Info("Neg in zone was not found", "negName", negName, "zone", zone, "err", err) |
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.
nit: "Neg was not found"
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.
Updated.
pkg/neg/syncers/utils.go
Outdated
@@ -161,21 +161,21 @@ func ensureNetworkEndpointGroup(svcNamespace, svcName, negName, zone, negService | |||
!utils.EqualResourceIDs(neg.Subnetwork, networkInfo.SubnetworkURL)) { | |||
|
|||
needToCreate = true | |||
klog.V(2).Infof("NEG %q in %q does not match network and subnetwork of the cluster. Deleting NEG.", negName, zone) | |||
logger.V(2).Info("NEG does not match network and subnetwork of the cluster. Deleting NEG", "negName", negName, "zone", zone) |
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.
nit: remove V(2)
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.
Updated.
pkg/neg/syncers/utils.go
Outdated
@@ -215,7 +215,7 @@ func ensureNetworkEndpointGroup(svcNamespace, svcName, negName, zone, negService | |||
var err error | |||
neg, err = cloud.GetNetworkEndpointGroup(negName, zone, version) | |||
if err != nil { | |||
klog.Errorf("Error while retrieving %q in zone %q: %v after initialization", negName, zone, err) | |||
logger.Error(err, "Error while retrieving in zone after initialization", "negName", negName, "zone", zone) |
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.
"Error while retrieving NEG after initialization"
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.
Updated.
pkg/neg/syncers/utils.go
Outdated
@@ -303,6 +309,13 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. | |||
// accidental diffs resulting from different formats. | |||
networkEndpoint.IPv6 = parseIPAddress(podIPs.IPv6) | |||
} | |||
neLogger := logger.WithValues( |
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.
should we do epLogger.WithValues. Then you don't need to add the EPS specific information.
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.
Updated. Thanks!
pkg/neg/syncers/utils.go
Outdated
@@ -313,7 +326,7 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. | |||
if existingPod, contains := networkEndpointPodMap[networkEndpoint]; contains { | |||
localEPCount[negtypes.Duplicate] += 1 | |||
if existingPod.Name < endpointAddress.TargetRef.Name { | |||
klog.Infof("Found duplicate endpoints [%v, %v] when processing endpoint slice %s/%s, save the pod information from the alphabetically higher pod", networkEndpoint.IP, networkEndpoint.IPv6, ed.Meta.Namespace, ed.Meta.Name) | |||
neLogger.Info("Found duplicate endpoints when processing endpoint slice, save the pod information from the alphabetically higher pod") |
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 you add the pod information to the log line too?
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.
Added.
pkg/neg/syncers/utils.go
Outdated
@@ -458,17 +476,24 @@ func toZoneNetworkEndpointMapDegradedMode(eds []negtypes.EndpointsData, zoneGett | |||
// accidental diffs resulting from different formats. | |||
networkEndpoint.IPv6 = parseIPAddress(podIPs.IPv6) | |||
} | |||
neLogger := logger.WithValues( |
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.
same comment as in the other function.
I think the diff is the EPS addresses field
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.
Updated.
58d508d
to
b3499ff
Compare
Yes that should be possible. I'll update those as well. |
Added contextual logging in all API call function signature in #2320, and created another commit to replace all placeholder in NEG related API calls to use the logger with context. |
/retest |
9dd254a
to
18280d7
Compare
18280d7
to
4d09276
Compare
pkg/neg/types/fakes.go
Outdated
@@ -243,7 +244,7 @@ func (f *FakeNetworkEndpointGroupCloud) DetachNetworkEndpoints(name, zone string | |||
return nil | |||
} | |||
|
|||
func (f *FakeNetworkEndpointGroupCloud) ListNetworkEndpoints(name, zone string, showHealthStatus bool, version meta.Version) ([]*composite.NetworkEndpointWithHealthStatus, error) { | |||
func (f *FakeNetworkEndpointGroupCloud) ListNetworkEndpoints(name, zone string, showHealthStatus bool, version meta.Version, logger klog.Logger) ([]*composite.NetworkEndpointWithHealthStatus, error) { |
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.
nit, should we ignore this parameter like the other functions in this fake?
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.
Thanks for catching this! Updated all instances of logger.
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.
Small nits. This LG and we can merge after the preceding PR goes in
4d09276
to
5890220
Compare
This PR is the continuation of kubernetes#1746(contextual logging for components in NEG controller). * Functions will accept a logger object from its caller, so the prefix will be determined based on the caller objects.
* Pass the logger to API call adapter and update the function interface. * Updates in the pkg/composite will be created in a separate PR when all components are done with contextual logging migration.
5890220
to
09e84a1
Compare
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sawsa307, swetharepakula 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 PR is the continuation of #1746 logging for components
in NEG controller).