confused SharedMountPoint with StorPool information in log files

[levindecaro]

ISSUE TYPE

• Improvement Request

COMPONENT NAME

Logging

CLOUDSTACK VERSION

4.17.1

CONFIGURATION

kvm
SharedMountPoint (CephFS)

OS / ENVIRONMENT

CentOS Linux release 7.9.2009 (Core) 3.10.0-1160.45.1.el7.x86_64
libvirt-4.5.0-36.el7_9.5.x86_64
qemu-kvm-ev-2.12.0-44.1.el7_8.1.x86_64
qemu-img-ev-2.12.0-44.1.el7_8.1.x86_64
qemu-kvm-common-ev-2.12.0-44.1.el7_8.1.x86_64
libvirt-daemon-driver-qemu-4.5.0-36.el7_9.5.x86_64

SUMMARY

Having upgrade from 4.16.1 to 4.17.1 recently, "Storpool" class/component name throwing around in the log, which is inaccurate for the setup are NOT involving StorPool.

eg:

1. new agent logs
   /var/log/cloudstack/agent/storpool-agent.log
   [excerpt]

2022-10-04 15:00:48,048 StorpooolStorageAdaptor.disconnectPhysicalDiskByPath: localPath=/cloudstack/sg1/d0126a5a-62c9-4935-b6f8-bba159d6fbae
2022-10-04 16:19:00,190 StorpooolStorageAdaptor.disconnectPhysicalDiskByPath: localPath=/cloudstack/sg1/d0126a5a-62c9-4935-b6f8-bba159d6fbae

2. managemnet log
   [excerpt]

snapshot=worker-2_ROOT-1136_20211122045039, uuid=bdc1c054-f455-46cb-a457-d3cf912bedba, op=REVERT
2022-10-04 17:05:47,664 DEBUG [o.a.c.s.s.StorPoolSnapshotStrategy] (qtp775386112-1654:ctx-5092e9c6 ctx-b3a05d56) (logid:bdc75a7c) StorpoolSnapshotStrategy.canHandle: snapshot=worker-1_ROOT-821_20211122045029, uuid=614277a3-980c-444f-b3bc-753e82a63e19, op=REVERT
2022-10-04 17:05:47,671 DEBUG [o.a.c.s.s.StorPoolSnapshotStrategy] (qtp775386112-1654:ctx-5092e9c6 ctx-b3a05d56) (logid:bdc75a7c) StorpoolSnapshotStrategy.canHandle: snapshot=worker-0_ROOT-820_20211122045019, uuid=9b142e93-52a0-40e0-ba70-9a8f08628821, op=REVERT
2022-10-04 17:05:47,688 DEBUG [o.a.c.s.s.StorPoolSnapshotStrategy] (qtp775386112-1654:ctx-5092e9c6 ctx-b3a05d56) (logid:bdc75a7c) StorpoolSnapshotStrategy.canHandle: snapshot=master-2_ROOT-819_20211122044950, uuid=21e94baa-7487-4b9b-8e36-cbbc65b34a5d, op=REVERT

STEPS TO REPRODUCE

1. Upgrade to 4.17.1
2. Examine the logs

EXPECTED RESULTS

Should report accurate log information of current component 

ACTUAL RESULTS

Mixed up SharedMountPoint with StorPool

Besides, classname StorpooolStorageAdaptor should be StorpoolStorageAdaptor

[yadvr]

cc @slavkap can you help us triage/investigate this? Thanks.

[slavkap]

yes, @rohityadavcloud, I did some investigation.

Actually, we do not mix anymore with the SharedMoutPoint.

@levindecaro, the disconnectPhysicalDiskByPath is invoked for all StorageAdaptors till one returns true:

    public boolean disconnectPhysicalDiskByPath(String path) {
        s_logger.debug(String.format("Disconnect physical disk by path: %s", path));
        for (Map.Entry<String, StorageAdaptor> set : _storageMapper.entrySet()) {
            StorageAdaptor adaptor = set.getValue();

            if (adaptor.disconnectPhysicalDiskByPath(path)) {
                s_logger.debug(String.format("Disconnected physical disk by local path: %s, using the storage adaptor for pool type: %s", path, set.getKey()));
                return true;
            }
        }

        return false;
    }

Which is not the best option instead of providing the correct StorageAdaptor.

Because we are logging everything which invokes our functionality, the storpool-agent.log is created in this case. I'll change this to be logged in the agent.log

For the management.log the situation is the same - the canHandle method is invoked for all Storage plugins that implement the SnapshotStrategy. You don't see logs for the rest because there aren't any.

For me, the more logs, the better. This is StorPool functionality invoked for some reason by CS. And as I said, we log everything which invokes that functionality. But to avoid any confusion, I could remove it. Do you agree?
Thanks!

[levindecaro]

@slavkap
Thanks for your explanation, I agree more verbose logging help for troubleshooting, for this case, it was leading myself to dig into source code for an error which is not relate to my current infrastructure. Since upgrade 4.17.1, I'd hard time to verify every component functionality, especially on kvm snapshot change.

[DaanHoogland]

@slavkap are you creating PR(s) for this?

[DaanHoogland]

and is only extra logging needed, or should/can we make the dismount more intelligent?
BTW this is true for more storage/network/hypervisorguru operations.

[slavkap]

@DaanHoogland, yes, I will prepare a PR that removes the logging to a StorPool's file when CS invokes our functionality, and it's not related to a StorPool storage

[DaanHoogland]

@slavkap is this issue still relevant?

[slavkap]

hey @DaanHoogland, the creation of the storpool-agent.log file is removed if there isn't a StorPool storage in PR 6661

[DaanHoogland]

hey @DaanHoogland, the creation of the storpool-agent.log file is removed if there isn't a StorPool storage in PR 6661

So it can be closed?

[slavkap]

sorry @DaanHoogland, I don't know how I've missed this. I think you can close it. The storpool log file won't be created if there aren't any StorPool primary storages

[DaanHoogland]

fixed in #6661