Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions kvmagent/kvmagent/plugins/shared_block_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -444,10 +444,10 @@ def get_disk_paths(disks):

return diskPaths

def create_vg_if_not_found(self, vgUuid, disks, hostUuid, allDisks, forceWipe=False):
def create_vg_if_not_found(self, vgUuid, disks, hostUuid, allDisks, forceWipe=False, is_first_create_vg=False):
# type: (str, set([CheckDisk]), str, set([CheckDisk]), bool) -> bool
@linux.retry(times=5, sleep_time=random.uniform(0.1, 3))
def find_vg(vgUuid, raise_exception = True):
def find_vg(vgUuid, raise_exception=True):
cmd = shell.ShellCmd("timeout 5 vgscan --ignorelockingfailure; vgs --nolocking -t %s -otags | grep %s" % (vgUuid, INIT_TAG))
cmd(is_exception=False)
if cmd.return_code != 0 and raise_exception:
Comment on lines 447 to 456
Copy link

@coderabbitai coderabbitai bot Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [447-463]

create_vg_if_not_found方法中,当is_first_create_vgFalse时,抛出异常以防止数据丢失,这与PR目标一致。但是,在add_disk方法中调用create_vg_if_not_found时,没有传递is_first_create_vg参数,这可能会导致在不应该创建vg时创建vg,从而可能导致数据丢失。建议在add_disk方法中添加对is_first_create_vg参数的处理。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我在 create_vg_if_not_found 的 is_first_create_vg 参数设置为默认 false,是不是这样就比较安全,就不需要在 add_disk 里加了

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatheMatrix 您的建议将is_first_create_vg参数的默认值设置为False是一个安全的做法,这样可以确保在默认情况下不会无意中重新创建卷组,从而避免数据丢失。如果您的代码逻辑中确实需要在某些情况下创建新的卷组,您可以显式地将该参数设置为True。这样,就不需要在add_disk方法中添加额外的处理了。🐰

Expand All @@ -457,14 +457,17 @@ def find_vg(vgUuid, raise_exception = True):
return True

@linux.retry(times=3, sleep_time=random.uniform(0.1, 3))
def create_vg(hostUuid, vgUuid, diskPaths, raise_excption = True):
def create_vg(hostUuid, vgUuid, diskPaths, raise_exception=True):
if not is_first_create_vg:
raise Exception("vg %s has already been created before, and there may be a risk of data loss during "
"secondary creation. Please check your storage" % vgUuid)
cmd = shell.ShellCmd("vgcreate -qq --shared --addtag '%s::%s::%s::%s' --metadatasize %s %s %s" %
(INIT_TAG, hostUuid, time.time(), linux.get_hostname(),
DEFAULT_VG_METADATA_SIZE, vgUuid, " ".join(diskPaths)))
cmd(is_exception=False)
logger.debug("created vg %s, ret: %s, stdout: %s, stderr: %s" %
(vgUuid, cmd.return_code, cmd.stdout, cmd.stderr))
if cmd.return_code != 0 and raise_excption:
if cmd.return_code != 0 and raise_exception:
raise RetryException("ret: %s, stdout: %s, stderr: %s" %
(cmd.return_code, cmd.stdout, cmd.stderr))
elif cmd.return_code != 0:
Expand Down Expand Up @@ -640,7 +643,7 @@ def config_lvm(host_id, enableLvmetad=False):

lvm.start_lvmlockd(cmd.ioTimeout)
logger.debug("find/create vg %s lock..." % cmd.vgUuid)
rsp.isFirst = self.create_vg_if_not_found(cmd.vgUuid, disks, cmd.hostUuid, allDisks, cmd.forceWipe)
rsp.isFirst = self.create_vg_if_not_found(cmd.vgUuid, disks, cmd.hostUuid, allDisks, cmd.forceWipe, cmd.isFirst)

# sanlock table:
#
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@ def __init__(self):
def disconnect(self, vgUuid, hostUuid):
return sharedblock_utils.shareblock_disconnect(vgUuid=vgUuid,hostUuid=hostUuid)

def connect(self, sharedBlockUuids, allSharedBlockUuids, vgUuid, hostUuid, hostId, forceWipe=False):
def connect(self, sharedBlockUuids, allSharedBlockUuids, vgUuid, hostUuid, hostId, forceWipe=False, isFirst=True):
return sharedblock_utils.shareblock_connect(
sharedBlockUuids=sharedBlockUuids,
allSharedBlockUuids=allSharedBlockUuids,
vgUuid=vgUuid,
hostId=hostId,
hostUuid=hostUuid,
forceWipe=forceWipe
forceWipe=forceWipe,
isFirst=isFirst
)

def logout(self, vgUuid, hostUuid):
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import os.path
import random

from kvmagent.test.shareblock_testsuite.shared_block_plugin_teststub import SharedBlockPluginTestStub
from kvmagent.test.utils import sharedblock_utils,pytest_utils,storage_device_utils
from zstacklib.utils import bash
from zstacklib.utils import bash,lvm
from unittest import TestCase
from zstacklib.test.utils import misc,env
import concurrent.futures
Expand Down Expand Up @@ -50,4 +51,19 @@ def test_sharedblock_connect(self):
to_do.append(future)

for future in concurrent.futures.as_completed(to_do):
self.assertEqual(future.result().success, True, future.result().error)
if not future.result().success:
self.assertEqual("other thread is connecting now" in future.result().error, True, future.result().error)

r, o ,e = bash.bash_roe("vgchange --lockstop %s" % vgUuid)
self.assertEqual(0, r, e)

r, o = bash.bash_ro(" pvs -oname --noheading -Svg_name=%s" % vgUuid)
disk = o.strip().splitlines()[0].strip()
bash.bash_roe("wipefs -af %s" % disk)
r = bash.bash_r("vgs | grep %s" % vgUuid)
self.assertNotEqual(0, r)

rsp = self.connect([blockUuid], [blockUuid], vgUuid, hostUuid, hostId, forceWipe=True, isFirst=False)
self.assertEqual(False, rsp.success, rsp.error)
r = bash.bash_r("vgs | grep %s" % vgUuid)
self.assertNotEqual(0, r)
5 changes: 3 additions & 2 deletions kvmagent/kvmagent/test/utils/sharedblock_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,16 @@ def sharedblock_ping(vgUuid):
}))

@misc.return_jsonobject()
def shareblock_connect(sharedBlockUuids=None, allSharedBlockUuids=None, vgUuid=None,hostId=None,hostUuid=None, forceWipe=True):
def shareblock_connect(sharedBlockUuids=None, allSharedBlockUuids=None, vgUuid=None,hostId=None,hostUuid=None, forceWipe=True, isFirst=True):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shareblock_connect 函数中添加了一个新参数 isFirst。请确认所有调用此函数的代码都已更新,以包含这个新参数。由于 isFirst 的默认值设置为 True,这可能会影响函数的现有行为,需要验证这一点是否符合预期。

return get_sharedblock_plugin().connect(misc.make_a_request({
"sharedBlockUuids":sharedBlockUuids, # [], ls /dev/disk/by-id -l|grep scsi
"allSharedBlockUuids":allSharedBlockUuids,
"vgUuid": vgUuid ,# random uuid
"hostId":hostId,
"hostUuid": hostUuid,
"forceWipe": forceWipe,
"primaryStorageUuid":vgUuid
"primaryStorageUuid":vgUuid,
"isFirst":isFirst
}))

@misc.return_jsonobject()
Expand Down