🌱 Adding logging verbosity levels for pkg/cloud/services/loadbalancer/loadbalancer.go #1636

asarunova · 2023-08-09T01:31:17Z

What this PR does / why we need it:
This PR is part of the process to improve logging in CAPO (issue #1314). It adds the proper verbosity levels to log messages in loadbalancer.go, as described in these guidelines.

Special notes for your reviewer:
In this file, I used mainly verbosity levels 2, and 3. According to the guidelines,

level 2: Useful steady state information about the service and important log messages that may correlate to significant changes in the system
level 3: Extended information about changes

Thus, I used level 2 for the creation/deletion of important objects (load balancer, listener, pool, monitor).
I used level 3 for 'reconciling' and 'waiting for' messages. I think those are slightly less important for understanding the process flow or debugging, but they do signify changes and deserve the attention of the developers.

/hold

k8s-ci-robot · 2023-08-09T01:31:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anastaruno
Once this PR has been reviewed and has the lgtm label, please assign vincepri for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

netlify · 2023-08-09T01:31:25Z

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

Name	Link
🔨 Latest commit	`d84775f`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64d4dfc620733c0008f05ddb
😎 Deploy Preview	https://deploy-preview-1636--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-09T01:31:26Z

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.

dulek · 2023-08-09T08:23:46Z

pkg/cloud/services/loadbalancer/loadbalancer.go

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

-	s.scope.Logger().Info(fmt.Sprintf("Creating load balancer in subnet: %q", subnetID), "name", loadBalancerName)
+	s.scope.Logger().V(2).Info("Creating load balancer in subnet", "subnetID", subnetID, "name", loadBalancerName)


This can be just "id" here.

But subnet is not the main subject of the message, load balancer is. Hence, no prefixes for load balancer, and "subnetID" for subnet. That was my logic

Yep. We should also update the documentation with the upstream links we used, plus any supplemental conventions we've decided on such as this.

dulek · 2023-08-09T08:23:55Z

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", "name", listenerName, "lb-id", lbID)
+	s.scope.Logger().V(2).Info("Creating load balancer listener", "name", listenerName, "loadBalancerID", lbID)


Just "id" here too.

Same logic here. Load balancer is not the main subject

dulek · 2023-08-09T08:24:55Z

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().V(3).Info("CIDRs do not match, updating listener", "expectedCIDRs", allowedCIDRs, "currentCIDRs", listener.AllowedCIDRs)


If V(2) informs about changes in the system, it's reasonable this should be V(2) too.

Is it important or useful enough to be V(2)? Seems like something that the system does on its own without impacting the user much

I agree with @dulek here. This branch is making an OpenStack API call and reconfiguring the listener (see line 275). This should probably log at V(2).

dulek · 2023-08-09T08:25:41Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -539,7 +539,7 @@ func (s *Service) DeleteLoadBalancerMember(openStackCluster *infrav1.OpenStackCl
 			return err
 		}
 		if pool == nil {
-			s.scope.Logger().Info("Load balancer pool does not exist", "name", lbPortObjectsName)
+			s.scope.Logger().V(2).Info("Load balancer pool does not exist", "name", lbPortObjectsName)


Seems like V(3) or even V(4).

Agreed, missed that one

mdbooth

Nice, this is really thoughtful. I have some queries for @dulek inline as he knows OpenStack load balancers much better than I do.

mdbooth · 2023-08-16T08:04:45Z

pkg/cloud/services/loadbalancer/loadbalancer.go

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

-	s.scope.Logger().Info(fmt.Sprintf("Creating load balancer in subnet: %q", subnetID), "name", loadBalancerName)
+	s.scope.Logger().V(2).Info("Creating load balancer in subnet", "subnetID", subnetID, "name", loadBalancerName)


Yep. We should also update the documentation with the upstream links we used, plus any supplemental conventions we've decided on such as this.

mdbooth · 2023-08-16T08:11:22Z

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().V(3).Info("CIDRs do not match, updating listener", "expectedCIDRs", allowedCIDRs, "currentCIDRs", listener.AllowedCIDRs)


I agree with @dulek here. This branch is making an OpenStack API call and reconfiguring the listener (see line 275). This should probably log at V(2).

mdbooth · 2023-08-16T08:20:19Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -539,7 +539,7 @@ func (s *Service) DeleteLoadBalancerMember(openStackCluster *infrav1.OpenStackCl
 			return err
 		}


@dulek Not related this this PR, but this looks pretty janky. Does this need an issue to track it?

We should probably just make a call and ignore 404's? Is this what you mean here?

mdbooth · 2023-08-16T08:22:45Z

pkg/cloud/services/loadbalancer/loadbalancer.go

@@ -539,7 +539,7 @@ func (s *Service) DeleteLoadBalancerMember(openStackCluster *infrav1.OpenStackCl
 			return err
 		}
 		if pool == nil {
-			s.scope.Logger().Info("Load balancer pool does not exist", "name", lbPortObjectsName)
+			s.scope.Logger().V(4).Info("Load balancer pool does not exist", "name", lbPortObjectsName)


V(4) looks correct to me, but I'm interested to know what @dulek thinks. I don't know this code super well: would it be 'normal' for the lb pool to not exist, i.e. an expected state of affairs, even if only briefly? If so, V(4) is appropriate. If it would be unusual then perhaps V(3) would be more appropriate.

Nah, you're right, in this context the pool not existing at all is pretty unusual.

k8s-ci-robot · 2023-08-16T17:47:07Z

PR needs rebase.

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-30T13:05:03Z

@anastaruno This needs a rebase.

k8s-triage-robot · 2024-01-21T07:16:55Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-02-20T07:30:16Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

EmilienM · 2024-02-21T17:14:02Z

I'm convinced that we need to fix logging, but not just for that file, and do it consistently for all the files.

k8s-triage-robot · 2024-03-22T17:28:56Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-03-22T17:29:00Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

k8s-ci-robot requested review from mdbooth and tobiasgiese August 9, 2023 01:31

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 9, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 9, 2023

dulek suggested changes Aug 9, 2023

View reviewed changes

adding verbosity levels for loadbalancer.go

d84775f

asarunova force-pushed the log_levels branch from d2ef335 to d84775f Compare August 10, 2023 13:01

mdbooth reviewed Aug 16, 2023

View reviewed changes

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 20, 2024

k8s-ci-robot closed this Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Adding logging verbosity levels for pkg/cloud/services/loadbalancer/loadbalancer.go #1636

🌱 Adding logging verbosity levels for pkg/cloud/services/loadbalancer/loadbalancer.go #1636

asarunova commented Aug 9, 2023 •

edited

Loading

k8s-ci-robot commented Aug 9, 2023

netlify bot commented Aug 9, 2023 •

edited

Loading

k8s-ci-robot commented Aug 9, 2023

dulek Aug 9, 2023

asarunova Aug 10, 2023

mdbooth Aug 16, 2023

dulek Aug 9, 2023

asarunova Aug 10, 2023

dulek Aug 9, 2023

asarunova Aug 10, 2023

mdbooth Aug 16, 2023

dulek Aug 9, 2023

asarunova Aug 10, 2023

mdbooth left a comment

mdbooth Aug 16, 2023

mdbooth Aug 16, 2023

mdbooth Aug 16, 2023

dulek Aug 17, 2023

mdbooth Aug 16, 2023

dulek Aug 17, 2023

k8s-ci-robot commented Aug 16, 2023

mdbooth commented Aug 30, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-triage-robot commented Feb 20, 2024

EmilienM commented Feb 21, 2024

k8s-triage-robot commented Mar 22, 2024

k8s-ci-robot commented Mar 22, 2024

		@@ -539,7 +539,7 @@ func (s Service) DeleteLoadBalancerMember(openStackCluster infrav1.OpenStackCl
		return err
		}

🌱 Adding logging verbosity levels for pkg/cloud/services/loadbalancer/loadbalancer.go #1636

🌱 Adding logging verbosity levels for pkg/cloud/services/loadbalancer/loadbalancer.go #1636

Conversation

asarunova commented Aug 9, 2023 • edited Loading

k8s-ci-robot commented Aug 9, 2023

netlify bot commented Aug 9, 2023 • edited Loading

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

k8s-ci-robot commented Aug 9, 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

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

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

k8s-ci-robot commented Aug 16, 2023

mdbooth commented Aug 30, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-triage-robot commented Feb 20, 2024

EmilienM commented Feb 21, 2024

k8s-triage-robot commented Mar 22, 2024

k8s-ci-robot commented Mar 22, 2024

asarunova commented Aug 9, 2023 •

edited

Loading

netlify bot commented Aug 9, 2023 •

edited

Loading