Skip to content

Conversation

@vijtrip2
Copy link
Contributor

@vijtrip2 vijtrip2 commented Oct 7, 2021

Issue #, if available: aws-controllers-k8s/community#1001

Description of changes:

  • "Kind" value in logs was empty because Get from K8s typed client does not unmarshall GVK fields. We need to use dynamic client OR deduce GVK from scheme object. However to set "Kind" field in logs, resource-descriptor can be used and easily fix this bug.
  • Other resource fields(name, namespace, generation) were getting duped in logs because resourceLogger constructor was deducing them and setting them in r.log values. When INFO or DEBUG method was called, they were getting deduced again and added as additional values. To fix this, during construction, we do not deduce fields from resource.

Additional changes:

  • Fields like name, namespace, kind do not change during reconciliation of the object so we can set them once during construction of resource logger
  • Only generation field is calculated on INFO/DEBUG methods
  • Adoption reconciler logger did not have these bugs. Adoption reconciler finds Kind from "adoptedResource.Spec" and does not use constructor for resource logger.

Tested locally and validated the logs.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

👍

"role", roleARN,
"region", region,
// All the fields for a resource that do not change during reconciliation
// can be initialized during resourceLogger creation
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this comment also apply to the three value pairs above it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but they are constant but at controller level.
I made the comment for fields we were deriving from the resource every time earlier

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually account, role and region can all change based on the namespace for a resource :)

@RedbackThomson
Copy link
Contributor

"Kind" value in logs was empty because Get from K8s typed client does not unmarshall GVK fields

Super weird. Would love to figure out that one, but some other day.

Great detective work!
/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Oct 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, RedbackThomson, vijtrip2

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:
  • OWNERS [RedbackThomson,jaypipes,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit 93d4146 into aws-controllers-k8s:main Oct 7, 2021
@vijtrip2 vijtrip2 deleted the dupe-logs branch October 8, 2021 01:54
acornett21 pushed a commit to acornett21/ack-runtime that referenced this pull request Oct 13, 2021
Issue #, if available: aws-controllers-k8s/community#1001

Description of changes:
* "Kind" value in logs was empty because `Get` from K8s typed client does not unmarshall GVK fields. We need to use dynamic client OR deduce GVK from `scheme` object. However to set "Kind" field in logs, resource-descriptor can be used and easily fix this bug.
* Other resource fields(name, namespace, generation) were getting duped in logs because resourceLogger constructor was deducing them and setting them in `r.log` values. When INFO or DEBUG method was called, they were getting deduced again and added as additional values. To fix this, during construction, we do not deduce fields from resource.

Additional changes:
* Fields like name, namespace, kind do not change during reconciliation of the object so we can set them once during construction of resource logger
* Only generation field is calculated on INFO/DEBUG methods
* Adoption reconciler logger did not have these bugs. Adoption reconciler finds Kind from "adoptedResource.Spec" and does not use constructor for resource logger.

Tested locally and validated the logs.  

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants