🌱Structured logging migration of instance.go, service.go, floatingip.go, and securitygroups.go #1631

skylerxhu · 2023-08-02T05:27:40Z

What this PR does / why we need it:
Fix up the string formatting log messages so that they use structured logging as documented here: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

netlify · 2023-08-02T05:27:49Z

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

Name	Link
🔨 Latest commit	`4e7e0fe`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64fec0548bd5d100083d969a
😎 Deploy Preview	https://deploy-preview-1631--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-08-02T05:27:49Z

Hi @skylerxhu. 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-08-02T12:55:21Z

/ok-to-test

mdbooth

/approve

I think it's good with or without a change of "err" to "error" (maybe), but if you could have a quick look for precedent I'd appreciate it. Thanks!

mdbooth · 2023-08-02T12:57:05Z

pkg/cloud/services/compute/instance.go

@@ -95,7 +95,7 @@ func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster *
 				if err != nil {
 					// Multiple matches might be ok later when we restrict matches to a single network
 					if errors.Is(err, networking.ErrMultipleMatches) {
-						s.scope.Logger().V(4).Info("Can't infer network from subnet %d: %s", i, err)
+						s.scope.Logger().V(4).Info("Couldn't infer network from subnet", "subnet", i, "err", err)


Do we have any other instances where we're passing an error in a structured log? Do we have a standard name for them already?

Also it doesn't seem like i is the subnet here. It's the index of the Port's FixedIPs, we should rather use fixedIP.Subnet here.

I think we should use "err" instead of "error" according to "With the exception of acronyms like "IP" and the standard "err", don't shorten names. " from this structured logging migration guideline(https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md).

There is another instance like this in this file. On line 261: s.scope.Logger().V(4).Error(err, "Failed to clean up ports after failure"). I'm not sure if we already have a standard name for them.

For the naming issue, I had a discussion with @mdbooth last week. We think subnetIndex is a proper choice. Sorry for not updating it in this PR. FixedIPs is a weird naming thing from OpenStack which really means subnets.

dulek · 2023-08-02T14:30:34Z

pkg/cloud/services/compute/instance.go

@@ -95,7 +95,7 @@ func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster *
 				if err != nil {
 					// Multiple matches might be ok later when we restrict matches to a single network
 					if errors.Is(err, networking.ErrMultipleMatches) {
-						s.scope.Logger().V(4).Info("Can't infer network from subnet %d: %s", i, err)
+						s.scope.Logger().V(4).Info("Couldn't infer network from subnet", "subnet", i, "err", err)


Also it doesn't seem like i is the subnet here. It's the index of the Port's FixedIPs, we should rather use fixedIP.Subnet here.

dulek · 2023-08-02T14:32:02Z

pkg/cloud/services/networking/floatingip.go

@@ -179,7 +179,7 @@ func (s *Service) DisassociateFloatingIP(eventObject runtime.Object, ip string)
 }

 func (s *Service) waitForFloatingIP(id, target string) error {
-	s.scope.Logger().Info("Waiting for floating IP", "id", id, "targetStatus", target)
+	s.scope.Logger().Info("Waiting for floating IP", "ID", id, "status", target)


status is vague here, it doesn't say if it's the current or expected status. How about expectedStatus?

The guideline(https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md) mentioned "Do not provide additional context for how value is used. Don't use podIP, do use IP." And "Context in name is acceptable to distinguish between values that normally go under same key. For example using both status and oldStatus in log that needs to show the change between statuses."

I still disagree, status here can be understood both ways.

This could go in the log message: "Waiting for floating IP status", and then just "status" in the parameters. I think that would be more explicit. We're "Waiting for floating IP", but what are we waiting for it to do? We're waiting for it to reach a target status.

stephenfin · 2023-08-04T10:01:36Z

pkg/cloud/services/networking/securitygroups.go

@@ -321,7 +321,7 @@ func (s *Service) createSecurityGroupIfNotExists(openStackCluster *infrav1.OpenS
 		return err
 	}
 	if secGroup == nil || secGroup.ID == "" {
-		s.scope.Logger().V(6).Info("Group doesn't exist, creating it.", "name", groupName)
+		s.scope.Logger().V(6).Info("Group doesn't exist, creating it", "name", groupName)


Somewhat unrelated, but I assume there are no issues with using contractions (doesn't rather than does not) in logs?

Yeah, the guideline didn't mention the contraction issue.

stephenfin · 2023-08-04T10:04:41Z

One question inline but it's more out of curiosity and not really related to this change.

/lgtm

k8s-ci-robot · 2023-08-04T10:04:44Z

@stephenfin: changing LGTM is restricted to collaborators

In response to this:

One question inline but it's more out of curiosity and not really related to this change.

/lgtm

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

/approve

I'll leave @dulek to lgtm when he's happy as discussed in the comments.

stephenfin · 2023-08-16T10:56:28Z

/lgtm

now that I have the appropriate powers

mdbooth · 2023-08-30T13:12:31Z

/lgtm
/hold cancel

mdbooth · 2023-08-30T16:18:40Z

Bah, needs a rebase.

…ce.go, and floatingip.go (rebase)

mdbooth

/approve
/lgtm

k8s-ci-robot · 2023-09-11T08:24:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2023

k8s-ci-robot requested review from mdbooth and tobiasgiese August 2, 2023 05:27

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 2, 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 Aug 2, 2023

mdbooth reviewed Aug 2, 2023

View reviewed changes

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

dulek suggested changes Aug 2, 2023

View reviewed changes

stephenfin reviewed Aug 4, 2023

View reviewed changes

skylerxhu force-pushed the structured-logging-migration branch from d6d9023 to a8f157a Compare August 7, 2023 17:55

mdbooth reviewed Aug 16, 2023

View reviewed changes

k8s-ci-robot assigned stephenfin Aug 16, 2023

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

skylerxhu force-pushed the structured-logging-migration branch 2 times, most recently from 373a76c to a8f157a Compare August 16, 2023 21:11

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023

k8s-ci-robot assigned mdbooth Aug 30, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 30, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2023

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 11, 2023

structured logging migration of securitygroups.go, instance.go, servi…

4e7e0fe

…ce.go, and floatingip.go (rebase)

skylerxhu force-pushed the structured-logging-migration branch from 5b78cc0 to 4e7e0fe Compare September 11, 2023 07:22

mdbooth reviewed Sep 11, 2023

View reviewed changes

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

k8s-ci-robot merged commit ccbaef6 into kubernetes-sigs:main Sep 11, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱Structured logging migration of instance.go, service.go, floatingip.go, and securitygroups.go #1631

🌱Structured logging migration of instance.go, service.go, floatingip.go, and securitygroups.go #1631

skylerxhu commented Aug 2, 2023 •

edited

netlify bot commented Aug 2, 2023 •

edited

k8s-ci-robot commented Aug 2, 2023

mdbooth commented Aug 2, 2023

mdbooth left a comment

mdbooth Aug 2, 2023

dulek Aug 2, 2023

skylerxhu Aug 3, 2023

skylerxhu Aug 3, 2023

skylerxhu Aug 3, 2023

dulek Aug 2, 2023

dulek Aug 2, 2023

skylerxhu Aug 3, 2023

dulek Aug 7, 2023

mdbooth Aug 16, 2023 •

edited

stephenfin Aug 4, 2023

skylerxhu Aug 4, 2023

stephenfin commented Aug 4, 2023

k8s-ci-robot commented Aug 4, 2023

mdbooth left a comment

stephenfin commented Aug 16, 2023

mdbooth commented Aug 30, 2023

mdbooth commented Aug 30, 2023

mdbooth left a comment

k8s-ci-robot commented Sep 11, 2023

🌱Structured logging migration of instance.go, service.go, floatingip.go, and securitygroups.go #1631

🌱Structured logging migration of instance.go, service.go, floatingip.go, and securitygroups.go #1631

Conversation

skylerxhu commented Aug 2, 2023 • edited

netlify bot commented Aug 2, 2023 • edited

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

k8s-ci-robot commented Aug 2, 2023

mdbooth commented Aug 2, 2023

mdbooth left a 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

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 Aug 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenfin commented Aug 4, 2023

k8s-ci-robot commented Aug 4, 2023

mdbooth left a comment

Choose a reason for hiding this comment

stephenfin commented Aug 16, 2023

mdbooth commented Aug 30, 2023

mdbooth commented Aug 30, 2023

mdbooth left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 11, 2023

skylerxhu commented Aug 2, 2023 •

edited

netlify bot commented Aug 2, 2023 •

edited

mdbooth Aug 16, 2023 •

edited