🌱 Migrating /pkg/cloud/services/loadbalancer/loadbalancer.go and /controllers/openstackmachine_controller.go to structured logging #1621

asarunova · 2023-07-24T20:23:46Z

What this PR does / why we need it:
It fixes the format of the log messages and log parameters. Following the guidelines here and here. Part of the process to fix issue #1314.

netlify · 2023-07-24T20:23:51Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`6915c37`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64d2ad2af5b4f50008bb11f7
😎 Deploy Preview	https://deploy-preview-1621--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

k8s-ci-robot · 2023-07-24T20:23:54Z

Hi @anastaruno. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

mdbooth · 2023-07-25T09:11:36Z

/ok-to-test

mdbooth · 2023-07-25T09:15:04Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -48,7 +48,7 @@ const loadBalancerProvisioningStatusActive = "ACTIVE"

 func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackCluster, clusterName string, apiServerPort int) (bool, error) {
 	loadBalancerName := getLoadBalancerName(clusterName)
-	s.scope.Logger().Info("Reconciling load balancer", "name", loadBalancerName)
+	s.scope.Logger().Info("Reconciling load balancer", "loadBalancerName", loadBalancerName)


Lets hold off on this change until we have agreement in #1617 (comment)

I suspect that we may decide we're going to stick with name here.

mdbooth · 2023-07-25T10:18:22Z

This is fun: you have a test failure!

If you click on details you'll see it's a linter failure:

pkg/cloud/services/loadbalancer/loadbalancer.go:657: File is not `gci`-ed with --skip-generated -s standard,default (gci)

Honestly I still haven't worked out exactly what 'gci'-ed means, but in this case I'm guessing it's referring to the lack of a newline at the end of the file. Normally it's complaining about the ordering of imports, but not today!

If you want to run the linter tests locally you can do make lint, which should give you the same error. You can fix it manually, but frequently you can fix it automatically by running the linter in update mode. In this repo you can do that with make lint-update.

mdbooth · 2023-07-26T08:34:48Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -169,7 +169,7 @@ func (s *Service) getOrCreateLoadBalancer(openStackCluster *infrav1.OpenStackClu
 		return lb, nil
 	}

-	s.scope.Logger().Info("Creating load balancer in subnet", "subnetID", subnetID, "loadBalancerName", loadBalancerName)
+	s.scope.Logger().Info("Creating load balancer in subnet", "id", subnetID, "name", loadBalancerName)


This is tricky! I think we have to prefix at least one of id and name here, as they refer to different objects. Which one is honestly debatable, but I might argue that the 'subject' of the log message is the load balancer, therefore name and subnetID.

Agree! Subnet is a secondary resource here.

mdbooth · 2023-07-26T08:40:28Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -199,7 +199,7 @@ func (s *Service) getOrCreateListener(openStackCluster *infrav1.OpenStackCluster
 		return listener, nil
 	}

-	s.scope.Logger().Info("Creating load balancer listener", "listenerName", listenerName, "loadBalancerID", lbID)
+	s.scope.Logger().Info("Creating load balancer listener", "name", listenerName, "id", lbID)


2 objects again. I'm thinking name and loadBalancerID.

mdbooth · 2023-07-26T09:10:08Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -267,7 +267,7 @@ func (s *Service) getOrUpdateAllowedCIDRS(openStackCluster *infrav1.OpenStackClu
 	listener.AllowedCIDRs = capostrings.Unique(listener.AllowedCIDRs)

 	if !reflect.DeepEqual(allowedCIDRs, listener.AllowedCIDRs) {
-		s.scope.Logger().Info("CIDRs do not match, start to update listener", "expected CIDRs", allowedCIDRs, "load balancer existing CIDR", listener.AllowedCIDRs)
+		s.scope.Logger().Info("CIDRs do not match, start to update listener", "expected CIDRs", allowedCIDRs, "existing CIDR", listener.AllowedCIDRs)


This is basically a trace level log message. When we get around to it, I'm pretty sure we'll put this at V(5). However...

I'm not 100% sure, but I don't think we want spaces in parameter names. Perhaps expected and current?

We also probably want ..., updating listener.

And lastly: are you able to find out how allowedCIDRs renders in this log message in practise? I wonder if we should explicitly format it somehow. However, don't spend much time on this given that it's most likely a trace level log.

mdbooth · 2023-07-26T09:11:07Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -316,7 +316,7 @@ func (s *Service) getOrCreatePool(openStackCluster *infrav1.OpenStackCluster, po
 		return pool, nil
 	}

-	s.scope.Logger().Info("Creating load balancer pool for listener", "listenerID", listenerID, "poolName", poolName, "loadBalancerID", lbID)
+	s.scope.Logger().Info("Creating load balancer pool for listener", "id", lbID, "listenerID", listenerID, "name", poolName)


I think 'pool' is the subject here, so loadBalancerID, listenerID, name?

mdbooth · 2023-07-26T09:12:14Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -356,7 +356,7 @@ func (s *Service) getOrCreateMonitor(openStackCluster *infrav1.OpenStackCluster,
 		return nil
 	}

-	s.scope.Logger().Info("Creating load balancer monitor for pool", "poolID", poolID, "monitorName", monitorName, "loadBalancerID", lbID)
+	s.scope.Logger().Info("Creating load balancer monitor for pool", "id", lbID, "name", monitorName, "poolID", poolID)


Ditto, but for 'monitor' instead of 'pool'.

mdbooth · 2023-07-26T09:14:26Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -400,7 +400,7 @@ func (s *Service) ReconcileLoadBalancerMember(openStackCluster *infrav1.OpenStac
 	}

 	loadBalancerName := getLoadBalancerName(clusterName)
-	s.scope.Logger().Info("Reconciling load balancer member", "loadBalancerName", loadBalancerName)
+	s.scope.Logger().Info("Reconciling load balancer member", "name", loadBalancerName)


Tricky. The only parameter doesn't refer to the subject of the log message. i.e. We're creating a 'member', but name refers to the load balancer, not the member. I'd probably use loadBalancerName here.

…tured logging

mdbooth

/approve
/lgtm

k8s-ci-robot · 2023-08-16T09:44:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anastaruno, mdbooth

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~~ [mdbooth]

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

k8s-ci-robot requested review from mdbooth and tobiasgiese July 24, 2023 20:23

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 24, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 24, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 25, 2023

mdbooth reviewed Jul 25, 2023

View reviewed changes

asarunova changed the title ~~🌱 Migrating /pkg/cloud/services/loadbalancer/loadbalancer.go to structured logging~~ 🌱 Migrating /pkg/cloud/services/loadbalancer/loadbalancer.go and /controllers/openstackmachine_controller.go to structured logging Jul 25, 2023

asarunova requested a review from mdbooth July 25, 2023 20:00

mdbooth reviewed Jul 26, 2023

View reviewed changes

asarunova force-pushed the main branch from 8f80ed0 to 2351d11 Compare August 8, 2023 02:31

asarunova requested a review from mdbooth August 8, 2023 02:32

asarunova force-pushed the main branch from 920dc46 to 2351d11 Compare August 8, 2023 19:20

migrating loadbalancer.go and openstackmachine_controller.go to struc…

6915c37

…tured logging

asarunova force-pushed the main branch from 38d1472 to 6915c37 Compare August 8, 2023 21:01

mdbooth reviewed Aug 16, 2023

View reviewed changes

k8s-ci-robot assigned mdbooth Aug 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2023

k8s-ci-robot merged commit e0b2279 into kubernetes-sigs:main Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Migrating /pkg/cloud/services/loadbalancer/loadbalancer.go and /controllers/openstackmachine_controller.go to structured logging #1621

🌱 Migrating /pkg/cloud/services/loadbalancer/loadbalancer.go and /controllers/openstackmachine_controller.go to structured logging #1621

asarunova commented Jul 24, 2023 •

edited

Loading

netlify bot commented Jul 24, 2023 •

edited

Loading

k8s-ci-robot commented Jul 24, 2023

mdbooth commented Jul 25, 2023

mdbooth Jul 25, 2023

mdbooth commented Jul 25, 2023

mdbooth Jul 26, 2023

dulek Jul 26, 2023

mdbooth Jul 26, 2023

mdbooth Jul 26, 2023

mdbooth Jul 26, 2023

mdbooth Jul 26, 2023

mdbooth Jul 26, 2023

mdbooth left a comment

k8s-ci-robot commented Aug 16, 2023

🌱 Migrating /pkg/cloud/services/loadbalancer/loadbalancer.go and /controllers/openstackmachine_controller.go to structured logging #1621

🌱 Migrating /pkg/cloud/services/loadbalancer/loadbalancer.go and /controllers/openstackmachine_controller.go to structured logging #1621

Conversation

asarunova commented Jul 24, 2023 • edited Loading

netlify bot commented Jul 24, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

k8s-ci-robot commented Jul 24, 2023

mdbooth commented Jul 25, 2023

Choose a reason for hiding this comment

mdbooth commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 16, 2023

asarunova commented Jul 24, 2023 •

edited

Loading

netlify bot commented Jul 24, 2023 •

edited

Loading