Skip to content

Conversation

@Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Sep 12, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3626

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 12, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 12, 2022
@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 12, 2022

Hmmm, not sure if I'm done yet. CAPA does already some nice logging practices such as using ctx logger in reconcile loops.

But I feel like I'm missing something still... I will look at V(n) logs as well later on.

@Skarlso Skarlso force-pushed the implement_logging_guidelines branch from 18cdeec to 2bfb29e Compare September 12, 2022 08:22
@Ankitasw
Copy link
Member

This looks good 👍
@Skarlso V(n) logging is also not consistent, so I think we can handle that as part of this PR.
Wdyt?

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 12, 2022

@Ankitasw True. It might warrant a separate PR to handle V(n) log numbers. :)

@Ankitasw
Copy link
Member

Ok, makes sense.
I think this issue is covering V(n) logging consistency part.

@Ankitasw
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2022
@sedefsavas
Copy link
Contributor

There are few other changes done in CAPV like using "sigs.k8s.io/controller-runtime instead of "sigs.k8s.io/controller-runtime/pkg/log".

kubernetes-sigs/cluster-api-provider-vsphere@df945ea

Also, does this PR cover the https://cluster-api.sigs.k8s.io/developer/providers/v1.1-to-v1.2.html logging section, there are bunch of changes in main in this doc.

/hold
until #3720 is merged.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2022
@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 13, 2022

Ah, thanks, @sedefsavas!

I shall take a look and incorporate those changes. :)

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 13, 2022
@Skarlso Skarlso force-pushed the implement_logging_guidelines branch from 4d9f9ab to 08b1db3 Compare September 13, 2022 06:55
@Skarlso Skarlso force-pushed the implement_logging_guidelines branch 2 times, most recently from c21dd6a to cf69fc8 Compare September 13, 2022 08:32
Copy link
Contributor Author

@Skarlso Skarlso Sep 13, 2022

Choose a reason for hiding this comment

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

I don't think this was ever working before properly. :D
This was trying to use the package local variable but created a new one instead.

@Skarlso Skarlso force-pushed the implement_logging_guidelines branch from ad0e277 to 2d3f98b Compare September 13, 2022 09:59
@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 13, 2022

I0913 09:56:33.117909     373 addons.go:164] "describe output" output=<
	{
	  AddonArn: "arn:aws:eks:eu-central-1:<NOPE>addon/default_managed-cluster-control-plane/vpc-cni/aac19b19-02bc-2d31-5eee-b48ef0cfe5ce",
	  AddonName: "vpc-cni",
	  AddonVersion: "v1.11.0-eksbuild.1",
	  ClusterName: "default_managed-cluster-control-plane",
	  CreatedAt: 2022-09-13 09:33:11.631 +0000 UTC,
	  Health: {
	    Issues: []
	  },
	  ModifiedAt: 2022-09-13 09:33:12.697 +0000 UTC,
	  Status: "ACTIVE",
	  Tags: {
	    kubernetes.io/cluster/managed-cluster: "owned"
	  }
	}
 >
I0913 09:56:33.119742     373 addons.go:95] "Reconcile EKS addons completed successfully"
I0913 09:56:33.119784     373 identity_provider.go:33] "reconciling oidc identity provider"
I0913 09:56:33.119814     373 eks.go:64] "Reconcile EKS control plane completed successfully"
I0913 09:56:33.119846     373 cni.go:45] "Reconciling aws-node DaemonSet in cluster" cluster="default/managed-cluster"
I0913 09:56:33.120667     373 recorder.go:103] "events: Normal" object={Kind:AWSManagedControlPlane Namespace:default Name:managed-cluster-control-plane UID:2fa136ba-67ff-4b1d-9d4f-0e375b6b7fa7 APIVersion:controlplane.cluster.x-k8s.io/v1beta1 ResourceVersion:12502 FieldPath:} reason="SuccessfulReconcileEKSClusterAddons" message="Reconciled addons for EKS Cluster default_managed-cluster-control-plane"
I0913 09:56:33.121198     373 recorder.go:103] "events: Normal" object={Kind:AWSManagedControlPlane Namespace:default Name:managed-cluster-control-plane UID:2fa136ba-67ff-4b1d-9d4f-0e375b6b7fa7 APIVersion:controlplane.cluster.x-k8s.io/v1beta1 ResourceVersion:12502 FieldPath:} reason="SuccessfulReconcileEKSClusterAddons" message="Reconciled addons for EKS Cluster default_managed-cluster-control-plane"
I0913 09:56:33.777756     373 reconcile.go:39] "Reconciling kube-proxy DaemonSet in cluster" cluster="default/managed-cluster"
I0913 09:56:33.903582     373 reconcile.go:34] "Reconciling aws-iam-authenticator configuration" cluster="default/managed-cluster"
I0913 09:56:34.859719     373 reconcile.go:60] "Mapping node IAM role" iam-role="arn:aws:iam::542654844075:role/nodes.cluster-api-provider-aws.sigs.k8s.io" user="system:node:{{EC2PrivateDNSName}}"
I0913 09:56:34.891112     373 reconcile.go:65] "Mapping additional IAM roles and users"
I0913 09:56:34.891191     373 reconcile.go:81] "Reconciled aws-iam-authenticator configuration" cluster-name="default_managed-cluster-control-plane"

Logs are looking good. :)

@Skarlso Skarlso force-pushed the implement_logging_guidelines branch from 2d3f98b to e4ea6e3 Compare September 13, 2022 10:01
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2022
@sedefsavas
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2022
@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 22, 2022

Oh Gods.... :D

Turned out to be simple. :)

@Skarlso Skarlso force-pushed the implement_logging_guidelines branch from e4ea6e3 to d0d2fc3 Compare September 22, 2022 10:54
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2022
@sedefsavas
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2022
@Skarlso Skarlso force-pushed the implement_logging_guidelines branch from d0d2fc3 to a867db9 Compare September 23, 2022 06:59
@richardcase
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2022
@k8s-ci-robot k8s-ci-robot merged commit d52c10c into kubernetes-sigs:main Sep 23, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.x milestone Sep 23, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase, sedefsavas

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 [richardcase,sedefsavas]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve logging with following CAPI guideline

5 participants