metrics: refactor volume collector

[derekbit]

Longhorn 8098

Which issue(s) this PR fixes:

Issue longhorn/longhorn#8098

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

[innobead]

Which part caused the nil exception?

note: The current code will not cause the Longhorn manager pod to crash, as the panic will be intendedly handled.

[innobead]

@c3y1huang Why we did recover part in each collection method? It looks intendely.

[derekbit]

Which part caused the nil exception?

				engineClientProxy, err = vc.getEngineClientProxy(e)
				if err == nil {
					defer engineClientProxy.Close()

					metrics, err = engineClientProxy.MetricsGet(e)
					if err != nil {
						vc.logger.WithError(err).Warnf("Failed to get metrics from volume %v from engine %v", e.Spec.VolumeName, e.Name)
					}
				} else {
					vc.logger.WithError(err).Warnf("Failed to get engine proxy of %v for volume %v", e.Name, v.Name)
				}

defer engineClientProxy.Close() causes nil pointer dereference.

note: The current code will not cause the Longhorn manager pod to crash, as the panic will be intendedly handled.

[innobead]

Which part caused the nil exception?

				engineClientProxy, err = vc.getEngineClientProxy(e)
				if err == nil {
					defer engineClientProxy.Close()

But from the log below. getEngineClientProxy returned an error and nil engineClientProxy. Why defer engineClientProxy.Close() will be invoked?

2024-03-01T10:28:17.583228766+01:00 time="2024-03-01T09:28:17Z" level=warning msg="Failed to get engine proxy of pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed-e-0 for volume pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed" func="metrics_collector.(*VolumeCollector).Collect" file="volume_collector.go:192" collector=volume error="failed to get binary client for engine pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed-e-0: cannot get client for engine pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed-e-0: engine is not running" node=core8
2024-03-01T10:28:17.583267365+01:00 time="2024-03-01T09:28:17Z" level=warning msg="Panic during collecting metrics" func="metrics_collector.(*VolumeCollector).Collect.func1" file="volume_collector.go:164" collector=volume error="runtime error: invalid memory address or nil pointer dereference" node=core8

/metrics_collector/volume_collector.go#L212-L219

func (vc *VolumeCollector) getEngineClientProxy(engine *longhorn.Engine) (c engineapi.EngineClientProxy, err error) {
	engineCliClient, err := controller.GetBinaryClientForEngine(engine, &engineapi.EngineCollection{}, engine.Status.CurrentImage)
	if err != nil {
		return nil, errors.Wrapf(err, "failed to get binary client for engine %v", engine.Name)
	}

	return engineapi.GetCompatibleClient(engine, engineCliClient, vc.ds, nil, vc.proxyConnCounter)
}

[derekbit]

Which part caused the nil exception?

				engineClientProxy, err = vc.getEngineClientProxy(e)
				if err == nil {
					defer engineClientProxy.Close()

But from the log below. getEngineClientProxy returned an error and nil engineClientProxy. Why defer engineClientProxy.Close() will be invoked?

2024-03-01T10:28:17.583228766+01:00 time="2024-03-01T09:28:17Z" level=warning msg="Failed to get engine proxy of pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed-e-0 for volume pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed" func="metrics_collector.(*VolumeCollector).Collect" file="volume_collector.go:192" collector=volume error="failed to get binary client for engine pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed-e-0: cannot get client for engine pvc-4c4da4f6-6585-4afe-8c2c-23b6624573ed-e-0: engine is not running" node=core8
2024-03-01T10:28:17.583267365+01:00 time="2024-03-01T09:28:17Z" level=warning msg="Panic during collecting metrics" func="metrics_collector.(*VolumeCollector).Collect.func1" file="volume_collector.go:164" collector=volume error="runtime error: invalid memory address or nil pointer dereference" node=core8

/metrics_collector/volume_collector.go#L212-L219

func (vc *VolumeCollector) getEngineClientProxy(engine *longhorn.Engine) (c engineapi.EngineClientProxy, err error) {
	engineCliClient, err := controller.GetBinaryClientForEngine(engine, &engineapi.EngineCollection{}, engine.Status.CurrentImage)
	if err != nil {
		return nil, errors.Wrapf(err, "failed to get binary client for engine %v", engine.Name)
	}

	return engineapi.GetCompatibleClient(engine, engineCliClient, vc.ds, nil, vc.proxyConnCounter)
}

You're right.
Let's close the PR. BTW, I don't see any issue that can cause the exception in the remaining part.
Do you have idea?

[innobead]

It looks normal to me as well, but it could be a nested error, possibly due to nil except for a property of any nested property of an object.

@c3y1huang any ideas?

[derekbit]

It looks normal to me as well, but it could be a nested error, possibly due to nil except for a property of any nested property of an object.

The refactored codes are better and clear. Do you think we can merge it?

[innobead]

Sounds good to me. Refactoring if having chances always. Of course, need to be careful of any regression.

[innobead]

LGTM.

Please update the commit message and title of this PR, as the change here doesn't really fix the nil exception issue.

[derekbit]

LGTM.

Please update the commit message and title of this PR, as the change here doesn't really fix the nil exception issue.

Done.

[innobead]

@derekbit the commit message is not changed yet.