UB-1835 new rescan and delete method #294
Conversation
Signed-off-by: 27149chen <[email protected]>
Signed-off-by: 27149chen <[email protected]>
Signed-off-by: 27149chen <[email protected]>
Signed-off-by: 27149chen <[email protected]>
Signed-off-by: 27149chen <[email protected]>
Signed-off-by: 27149chen <[email protected]>
There was a problem hiding this comment.
Reviewable status: 0 of 38 files reviewed, 26 unresolved discussions (waiting on @27149chen, @olgashtivelman, and @shay-berman)
local/scbe/scbe_rest_client.go, line 28 at r2 (raw file):
) //go:generate counterfeiter -o ../../fakes/fake_scbe_rest_client.go . ScbeRestClient
any reason why you changed the fake location?
remote/mounter/mounter_factory.go, line 10 at r2 (raw file):
) //go:generate counterfeiter -o ../../fakes/fake_mounter_factory.go . MounterFactory
why new location?
remote/mounter/scbe.go, line 55 at r2 (raw file):
func (s *scbeMounter) prepareVolumeMountProperties(mountRequest resources.MountRequest) *resources.VolumeMountProperties { volumeWWN := mountRequest.VolumeConfig["Wwn"].(string) volumeLunNumber := -1
what does it mean -1? who handle it in a case no lun id?
remote/mounter/scbe.go, line 154 at r2 (raw file):
} func (s *scbeMounter) prepareVolumeActionAfterDetachProperties(request resources.AfterDetachRequest) *resources.VolumeMountProperties {
please reduce code duplication.
Looks like this function is similar to prepareVolumeMountProperties.
So just use prepareVolumeMountProperties(wwn, lunid) instead of duplicate the function with different request object.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
defer b.logger.Debug("Released rescanLock for volumeWWN", logs.Args{{"volumeWWN", wwn}}) if !rescanForCleanUp {
explain why you drop it please?
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 201 at r2 (raw file):
} } if !rescanForCleanUp {
why you drop it?
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 202 at r2 (raw file):
} // DisconnectAll remove the device from host after the volume is unmapped.
the name DisconnectAll is not clear to me.
do you mean to do iscsiadm logout? is it just removing the devices or both?
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 205 at r2 (raw file):
func (b *blockDeviceMounterUtils) DisconnectAll(volumeMountProperties *resources.VolumeMountProperties) error { // in case of FC : if no iscsiadm on the machine or no session login - this will log a warning not fail! if err := b.blockDeviceUtils.Disconnect(block_device_utils.ISCSI, volumeMountProperties); err != nil {
is it relevant if no iscsi needed?
remote/mounter/block_device_mounter_utils/block_device_utils_mounter_test.go, line 298 at r2 (raw file):
Expect(protocol).To(Equal(block_device_utils.ISCSI)) }) It("should fail if scsi disconnect fail", func() {
what does it mean? scsi disconnect?
remote/mounter/block_device_mounter_utils/resources.go, line 21 at r2 (raw file):
import "github.com/IBM/ubiquity/resources" //go:generate counterfeiter -o ../../../fakes/fake_block_device_mounter_utils.go . BlockDeviceMounterUtils
please try to keep the fakes in the same location.
remote/mounter/block_device_mounter_utils/resources.go, line 21 at r2 (raw file):
//go:generate counterfeiter -o ../fakes/fake_block_device_mounter_utils.go . BlockDeviceMounterUtils type BlockDeviceMounterUtils interface { RescanAll(wwn string, rescanForCleanUp bool, extraLunZeroScanning bool) error
I understand why u removed the extralun param, but why the rescanforcleanup?
remote/mounter/block_device_utils/block_device_utils.go, line 27 at r2 (raw file):
} func NewBlockDeviceUtilsWithExecutorAndConnector(executor utils.Executor, fcConnector initiator.Connector) BlockDeviceUtils {
if this only for test?
can you please add a short section in the PR that explain the code design as well. Its very long PR to scan.
remote/mounter/block_device_utils/mpath.go, line 57 at r2 (raw file):
} func (b *blockDeviceUtils) Discover(volumeWwn string, deepDiscovery bool) (string, error) {
General comment - did u run staging CI on this branch?
remote/mounter/block_device_utils/rescan.go, line 71 at r2 (raw file):
func (b *blockDeviceUtils) RescanSCSI() error { defer b.logger.Trace(logs.DEBUG)()
why removing the trace?
remote/mounter/block_device_utils/rescan.go, line 84 at r2 (raw file):
func (b *blockDeviceUtils) RescanSCSI(volumeMountProperties *resources.VolumeMountProperties) error { var err error for i := 0; i < 6; i++ {
- 6 should be a const.
- why 6 retries?
- how the retries impact the performance?
remote/mounter/block_device_utils/rescan.go, line 88 at r2 (raw file):
return b.logger.ErrorRet(err, "RescanSCSI failed", logs.Args{{"volumeWWN", volumeMountProperties.WWN}}) } if _, _, err = b.getMultipathOutputAndDeviceUid(volumeMountProperties.WWN); err == nil {
please explain because the function name doesn't helm me to understand.
is it to discover the mpath device? why not using the discover function?
what if its not ound
remote/mounter/block_device_utils/rescan.go, line 92 at r2 (raw file):
} b.logger.Warning("Can't find the new volume in multipath output after rescan, sleep one second and try again.") time.Sleep(1 * time.Second)
why 1 second? what is the exception here? how much time the connect should take?
remote/mounter/block_device_utils/rescan.go, line 97 at r2 (raw file):
} // TODO: improve it to make it faster
still relevant?
remote/mounter/block_device_utils/rescan.go, line 99 at r2 (raw file):
// TODO: improve it to make it faster func (b *blockDeviceUtils) DisconnectISCSI() error { return b.RescanISCSI()
how disconnect related to rescan iscsi?
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
//TODO: get channel and target if volumeMountProperties.LunNumber == -1 { return "- - -"
who uses this case? how do u test it?
remote/mounter/initiator/linuxfc.go, line 64 at r2 (raw file):
} systool := "systool"
please set as const
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
systool := "systool" if err := lfc.exec.IsExecutable(systool); err != nil { lfc.logger.Warning(fmt.Sprintf("No systool installed, get from path %s instead.", FC_HOST_SYSFS_PATH))
is systool is mandatory or not? why its only a warning?
do we need to ask the customer to install this tool or not? if not what is the impact? if there is no impact then why we need it?
remote/mounter/initiator/linuxfc.go, line 70 at r2 (raw file):
} out, err := lfc.exec.Execute(systool, []string{"-c", "fc_host", "-v"})
You must use Execute with timeout, to prevent blocking.
remote/mounter/initiator/linuxfc.go, line 72 at r2 (raw file):
out, err := lfc.exec.Execute(systool, []string{"-c", "fc_host", "-v"}) if err != nil { lfc.logger.Warning(fmt.Sprintf("Executing systool failed with error: %v. Get from path %s instead.", err, FC_HOST_SYSFS_PATH))
in the text please use the const instead
remote/mounter/initiator/linuxfc.go, line 78 at r2 (raw file):
// No FC HBAs were found if len(out) == 0 { return []string{}
maybe warnning should raise here. maybe error as well. host withouth hba that using FC?
remote/mounter/initiator/linuxfc.go, line 100 at r2 (raw file):
for _, host := range hostInfos { hbas = append(hbas, host.Name())
is systool is mandatory?
maybe there is other way to identify if hba is active, maybe its inside the /sys info?
There was a problem hiding this comment.
Reviewable status: 0 of 38 files reviewed, 34 unresolved discussions (waiting on @27149chen and @olgashtivelman)
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
port_id = "0x0ecfd3" port_name = "0xc05076e511803cb0" port_state = "Online"
r u sure u cannot get this info from the /sys/.../...?
and what happened if u do the echo on none active hba? what is the impact?
remote/mounter/initiator/linuxfc.go, line 181 at r2 (raw file):
*/ func generateHBAsInfoFromSystoolOutput(out []byte) []map[string]string {
please make sure u have UT for this function. sound like long logic that need coverage.
remote/mounter/initiator/linuxscsi.go, line 23 at r2 (raw file):
func (ls *linuxSCSI) RemoveSCSIDevice(device string) error { deviceName := strings.Replace(device, "/dev/", "", 1) path := SYS_BLOCK_PATH + fmt.Sprintf("/%s/device/delete", deviceName)
look strange
please add sys_block_path into the fmt.sprintf like %s/%s/device/delete
Note you can also use golang module for adding path (instead of using /), but its low priority
remote/mounter/initiator/connectors/fibre_channel.go, line 49 at r2 (raw file):
if len(hbas) == 0 { c.logger.Warning("No FC HBA is found.")
is it warning or error?
remote/mounter/initiator/connectors/fibre_channel.go, line 70 at r2 (raw file):
func (c *fibreChannelConnector) removeDevices(devices []string) error { // Do we need to flush io?
good question, what do u think? anyway at this point the fs should be unmounted, so not sure.
remote/mounter/initiator/connectors/fibre_channel.go, line 87 at r2 (raw file):
lunNumber := volumeMountProperties.LunNumber out, err := c.exec.Execute(multipath, []string{"-ll", "|", fmt.Sprintf(`egrep "[0-9]+:[0-9]+:[0-9]+:%d "`, lunNumber)})
r u sure it will get only 1 line?
using egrep in golang, i think its not the best practice.
remote/mounter/initiator/connectors/fibre_channel.go, line 118 at r2 (raw file):
} path := strings.Split(line, " ")[2] if strings.HasPrefix(path, "sd") {
1.not sure if its correct to count on sd as prefix. could be risky.
2. what happened if there are faulty device? make sure u r not failing there.
remote/mounter/initiator/connectors/fibre_channel_test.go, line 19 at r2 (raw file):
|- 0:0:4:255 sda 8:0 active ready running |- 0:0:5:255 sdb 8:16 active ready running |- 0:0:6:255 sdc 8:32 active ready running
check more inputs
There was a problem hiding this comment.
Reviewable status: 0 of 38 files reviewed, 34 unresolved discussions (waiting on @olgashtivelman and @shay-berman)
local/scbe/scbe_rest_client.go, line 28 at r2 (raw file):
Previously, shay-berman wrote…
any reason why you changed the fake location?
After run "go generate ./..." I find that lots of the fake files are in wrong place, so I fix the paths to make sure every fake file is in old fakes/ folder.
remote/mounter/mounter_factory.go, line 10 at r2 (raw file):
Previously, shay-berman wrote…
why new location?
After run "go generate ./..." I find that lots of the fake files are in wrong place, so I fix the paths to make sure every fake file is in old fakes/ folder.
remote/mounter/scbe.go, line 55 at r2 (raw file):
Previously, shay-berman wrote…
what does it mean -1? who handle it in a case no lun id?
I just want to make sure if there is no lunid in volumeConfig, we can rescan all the luns, it is a default value.
remote/mounter/scbe.go, line 154 at r2 (raw file):
Previously, shay-berman wrote…
please reduce code duplication.
Looks like this function is similar to prepareVolumeMountProperties.
So just use prepareVolumeMountProperties(wwn, lunid) instead of duplicate the function with different request object.
okay, improved
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
Previously, shay-berman wrote…
explain why you drop it please?
rescan and cleanUp are totally different actions, so I separate them to different functions
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 201 at r2 (raw file):
Previously, shay-berman wrote…
why you drop it?
rescan and cleanUp are totally different actions, so I separate them to different functions
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 202 at r2 (raw file):
Previously, shay-berman wrote…
the name DisconnectAll is not clear to me.
do you mean to do iscsiadm logout? is it just removing the devices or both?
removing device information from host (clean up), I named it "RemoveAll", but I think it is confusing too, "remove" sounds like "delete", so I name it "DisconnectAll" because os_brick use this name.
What do you prefer? Disconnect or Remove or Cleanup?
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 205 at r2 (raw file):
Previously, shay-berman wrote…
is it relevant if no iscsi needed?
yes, since we have no way to distinguish an iscsi and scsi host , we will run both. I agree that it's not a good idea.
remote/mounter/block_device_mounter_utils/block_device_utils_mounter_test.go, line 298 at r2 (raw file):
Previously, shay-berman wrote…
what does it mean? scsi disconnect?
disconnect or remove or cleanup? which do you prefer?
remote/mounter/block_device_mounter_utils/resources.go, line 21 at r2 (raw file):
Previously, shay-berman wrote…
please try to keep the fakes in the same location.
Explained
remote/mounter/block_device_mounter_utils/resources.go, line 21 at r2 (raw file):
Previously, shay-berman wrote…
I understand why u removed the extralun param, but why the rescanforcleanup?
Explained above, I separate them to two functions
remote/mounter/block_device_utils/block_device_utils.go, line 27 at r2 (raw file):
Previously, shay-berman wrote…
if this only for test?
can you please add a short section in the PR that explain the code design as well. Its very long PR to scan.
Yes.
I have added a description for the main logic of this PR
remote/mounter/block_device_utils/mpath.go, line 57 at r2 (raw file):
Previously, shay-berman wrote…
General comment - did u run staging CI on this branch?
not yet
remote/mounter/block_device_utils/rescan.go, line 71 at r2 (raw file):
Previously, shay-berman wrote…
why removing the trace?
add it back
remote/mounter/block_device_utils/rescan.go, line 84 at r2 (raw file):
Previously, shay-berman wrote…
- 6 should be a const.
- why 6 retries?
- how the retries impact the performance?
rescan by echo is an async action, though it is fast enough, we can not guarantee that it is done immediately. So adding retries here is just in case, counts doesn't matter very much. in most cases, less than one retry will happen.
I use 61 is because os_brick use 32
remote/mounter/block_device_utils/rescan.go, line 88 at r2 (raw file):
Previously, shay-berman wrote…
please explain because the function name doesn't helm me to understand.
is it to discover the mpath device? why not using the discover function?what if its not ound
sorry to make you confusing, I forgot the description of this function, it is added.
I didn't change anything, just move part of the code out of Discover() so that I can reuse it.
remote/mounter/block_device_utils/rescan.go, line 92 at r2 (raw file):
Previously, shay-berman wrote…
why 1 second? what is the exception here? how much time the connect should take?
Explained above, it is very fast.
remote/mounter/block_device_utils/rescan.go, line 97 at r2 (raw file):
Previously, shay-berman wrote…
still relevant?
yes
remote/mounter/block_device_utils/rescan.go, line 99 at r2 (raw file):
Previously, shay-berman wrote…
how disconnect related to rescan iscsi?
disconnecting (removing) a iscsi disk also need to improve, but I don't have time, so I add a TODO, and use the way we did before.
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, shay-berman wrote…
who uses this case? how do u test it?
It just in case if we didn't get the lunid from backend (or we do want to rescan all luns), we can rescan all the luns
remote/mounter/initiator/linuxfc.go, line 64 at r2 (raw file):
Previously, shay-berman wrote…
please set as const
Done.
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, shay-berman wrote…
is systool is mandatory or not? why its only a warning?
do we need to ask the customer to install this tool or not? if not what is the impact? if there is no impact then why we need it?
with it, we can get all the active HBAs, without it, we can get all the HBAs under /sys/class/fc_host/.
So it is not mandatory for now, but we'd better have it, because there are lots of other info in it which is useful if we want to improve the scan process more.
AFAIK, it is a builtin tool in Redhat and Ubuntu
remote/mounter/initiator/linuxfc.go, line 70 at r2 (raw file):
Previously, shay-berman wrote…
You must use Execute with timeout, to prevent blocking.
systool is not a block command
remote/mounter/initiator/linuxfc.go, line 72 at r2 (raw file):
Previously, shay-berman wrote…
in the text please use the const instead
Which text?
remote/mounter/initiator/linuxfc.go, line 78 at r2 (raw file):
Previously, shay-berman wrote…
maybe warnning should raise here. maybe error as well. host withouth hba that using FC?
maybe HBAs are not active. anyway, rescan should not care about it, but discover do. to rescan(or getHbas) action itself, it is passed. so the process is: rescan -> discover -> can not see the new device -> rescan ... At the end, we will raise a volume not found error instead of a rescan error
remote/mounter/initiator/linuxfc.go, line 100 at r2 (raw file):
Previously, shay-berman wrote…
is systool is mandatory?
maybe there is other way to identify if hba is active, maybe its inside the /sys info?
systool is a common way to do it.
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
Previously, shay-berman wrote…
r u sure u cannot get this info from the /sys/.../...?
and what happened if u do the echo on none active hba? what is the impact?
systool is a easier way.
If no HBA is active, we won't echo anything
remote/mounter/initiator/linuxfc.go, line 181 at r2 (raw file):
Previously, shay-berman wrote…
please make sure u have UT for this function. sound like long logic that need coverage.
yes, I have. And it is os_brick method, I just cover it from python to golang
remote/mounter/initiator/linuxscsi.go, line 23 at r2 (raw file):
Previously, shay-berman wrote…
look strange
please add sys_block_path into the fmt.sprintf like %s/%s/device/delete
Note you can also use golang module for adding path (instead of using /), but its low priority
done
remote/mounter/initiator/connectors/fibre_channel.go, line 49 at r2 (raw file):
Previously, shay-berman wrote…
is it warning or error?
warning, as I explained above.
remote/mounter/initiator/connectors/fibre_channel.go, line 70 at r2 (raw file):
Previously, shay-berman wrote…
good question, what do u think? anyway at this point the fs should be unmounted, so not sure.
maybe we need to flush multipath, but in k8s env, no data io will happen here I think
remote/mounter/initiator/connectors/fibre_channel.go, line 87 at r2 (raw file):
Previously, shay-berman wrote…
r u sure it will get only 1 line?
using egrep in golang, i think its not the best practice.
We just need a few characters at the beginning of each line, so it doesn't matter. I can filter it in golang, but I think it is more simple
remote/mounter/initiator/connectors/fibre_channel.go, line 118 at r2 (raw file):
Previously, shay-berman wrote…
1.not sure if its correct to count on sd as prefix. could be risky.
2. what happened if there are faulty device? make sure u r not failing there.
Maybe just remove this check, what do you think?
remote/mounter/initiator/connectors/fibre_channel_test.go, line 19 at r2 (raw file):
Previously, shay-berman wrote…
check more inputs
Done.
Signed-off-by: 27149chen <[email protected]>
Signed-off-by: 27149chen <[email protected]>
Signed-off-by: 27149chen <[email protected]>
There was a problem hiding this comment.
Reviewable status: 0 of 38 files reviewed, 16 unresolved discussions (waiting on @27149chen and @olgashtivelman)
local/scbe/scbe_rest_client.go, line 28 at r2 (raw file):
Previously, 27149chen wrote…
After run "go generate ./..." I find that lots of the fake files are in wrong place, so I fix the paths to make sure every fake file is in old fakes/ folder.
since its a comment, there is a good chance its not accurate.
thanks
remote/mounter/scbe.go, line 55 at r2 (raw file):
Previously, 27149chen wrote…
I just want to make sure if there is no lunid in volumeConfig, we can rescan all the luns, it is a default value.
But it will never be used. (as far as I understand) and if so then why to support this option? (u will need to test it.)
If we always get the lun ID, then if there is no lun ID I believe u should raise error because something wrong,
anyway, if u want to support it make sure u test it, and the QA also cover it. if its not needed then just return error.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
Previously, 27149chen wrote…
rescan and cleanUp are totally different actions, so I separate them to different functions
so the flow skip the same? its hard to see if from the PR.
so do we still skip discovery when cleanup?
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 202 at r2 (raw file):
Previously, 27149chen wrote…
removing device information from host (clean up), I named it "RemoveAll", but I think it is confusing too, "remove" sounds like "delete", so I name it "DisconnectAll" because os_brick use this name.
What do you prefer? Disconnect or Remove or Cleanup?
if its clean up the devices from the host, then just cleanupDevice.
but what does it mean disconnect - r u do iscsi logout? if not and its just delete the mpath device and the phisical devices, then yes - cleanupVolumeDevices or something.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 205 at r2 (raw file):
Previously, 27149chen wrote…
yes, since we have no way to distinguish an iscsi and scsi host , we will run both. I agree that it's not a good idea.
why?
but u have the indication if you run for iscsi or not. try to check if u have something that can help.
anyway as far as i remember we do run iscsi things if the iscsiadm exist on the host.
remote/mounter/block_device_mounter_utils/block_device_utils_mounter_test.go, line 298 at r2 (raw file):
Previously, 27149chen wrote…
disconnect or remove or cleanup? which do you prefer?
i guess cleanupVolumeDevices will make more sense
remote/mounter/block_device_mounter_utils/resources.go, line 21 at r2 (raw file):
Previously, 27149chen wrote…
Explained above, I separate them to two functions
but the flow keep the same?
remote/mounter/block_device_utils/mpath.go, line 57 at r2 (raw file):
Previously, 27149chen wrote…
not yet
ok, just recommendation - if possible try to run staging next time before code review just to make sure the code is ok.
remote/mounter/block_device_utils/rescan.go, line 71 at r2 (raw file):
Previously, 27149chen wrote…
add it back
thanks
remote/mounter/block_device_utils/rescan.go, line 84 at r2 (raw file):
Previously, 27149chen wrote…
rescan by echo is an async action, though it is fast enough, we can not guarantee that it is done immediately. So adding retries here is just in case, counts doesn't matter very much. in most cases, less than one retry will happen.
I use 61 is because os_brick use 32
ok sounds good
remote/mounter/block_device_utils/rescan.go, line 88 at r2 (raw file):
Previously, 27149chen wrote…
sorry to make you confusing, I forgot the description of this function, it is added.
I didn't change anything, just move part of the code out of Discover() so that I can reuse it.
so its the original code , just moved out to another place?
remote/mounter/block_device_utils/rescan.go, line 97 at r2 (raw file):
Previously, 27149chen wrote…
yes
if its important open ticket to remember. anyway maybe add how to improve it in one liner.
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, 27149chen wrote…
It just in case if we didn't get the lunid from backend (or we do want to rescan all luns), we can rescan all the luns
do we support such use case? if its a code that never get used then please remove it - less things to test.
remote/mounter/initiator/linuxfc.go, line 64 at r2 (raw file):
Previously, 27149chen wrote…
Done.
maybe i miss it but i ment for golang const like at the top
const Pi = 3.14
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, 27149chen wrote…
with it, we can get all the active HBAs, without it, we can get all the HBAs under /sys/class/fc_host/.
So it is not mandatory for now, but we'd better have it, because there are lots of other info in it which is useful if we want to improve the scan process more.AFAIK, it is a builtin tool in Redhat and Ubuntu
r u sure its built in tool? did u checked it with rhel 7? and other OS we support? I am not sure its comes built in (i ddn't have it on ubuntu)
anyway you need to be accurate - do u ask for the user to install it or not? and if not how it will impact? what will get wrong ? what is the risk of not having it?
if there is no risk and we don't loose nothing - then drop it (less code to test). if its important for the release then make sure u document that this tool must be install on the host as part of the pre-requisites.
remote/mounter/initiator/linuxfc.go, line 70 at r2 (raw file):
Previously, 27149chen wrote…
systool is not a block command
got u
anyway i would put timeout - who knows what could get wrong. we try to put timeout to all OS commands
remote/mounter/initiator/linuxfc.go, line 72 at r2 (raw file):
Previously, 27149chen wrote…
Which text?
of the warning ... systool...
remote/mounter/initiator/linuxfc.go, line 78 at r2 (raw file):
Previously, 27149chen wrote…
maybe HBAs are not active. anyway, rescan should not care about it, but discover do. to rescan(or getHbas) action itself, it is passed. so the process is: rescan -> discover -> can not see the new device -> rescan ... At the end, we will raise a volume not found error instead of a rescan error
ok so discover will fail to find it, right? if so its good to go
remote/mounter/initiator/linuxfc.go, line 100 at r2 (raw file):
Previously, 27149chen wrote…
systool is a common way to do it.
as mentioned only if it bring value
if not lets don't tell the customer to install another rpm
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
Previously, 27149chen wrote…
systool is a easier way.
If no HBA is active, we won't echo anything
is cinder use this tool?
what will happend if u echo to a none active hba?
remote/mounter/initiator/connectors/fibre_channel.go, line 70 at r2 (raw file):
Previously, 27149chen wrote…
maybe we need to flush multipath, but in k8s env, no data io will happen here I think
not sure
remote/mounter/initiator/connectors/fibre_channel.go, line 87 at r2 (raw file):
Previously, 27149chen wrote…
We just need a few characters at the beginning of each line, so it doesn't matter. I can filter it in golang, but I think it is more simple
up 2 u
remote/mounter/initiator/connectors/fibre_channel.go, line 118 at r2 (raw file):
Previously, 27149chen wrote…
Maybe just remove this check, what do you think?
not sure
try to make sure u take the device, sometimes device are not sd..., its something the client can change in udev. so take care
Signed-off-by: 27149chen <[email protected]>
There was a problem hiding this comment.
Reviewable status: 0 of 40 files reviewed, 16 unresolved discussions (waiting on @olgashtivelman and @shay-berman)
remote/mounter/scbe.go, line 55 at r2 (raw file):
Previously, shay-berman wrote…
But it will never be used. (as far as I understand) and if so then why to support this option? (u will need to test it.)
If we always get the lun ID, then if there is no lun ID I believe u should raise error because something wrong,anyway, if u want to support it make sure u test it, and the QA also cover it. if its not needed then just return error.
It just a default value, this method will be used in both mount and unmount, in unmount we can not get the lunid because it is unmapped. If I don't set it to -1, the default value will be 0, which is wrong.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
Previously, shay-berman wrote…
so the flow skip the same? its hard to see if from the PR.
so do we still skip discovery when cleanup?
yes
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 202 at r2 (raw file):
Previously, shay-berman wrote…
if its clean up the devices from the host, then just cleanupDevice.
but what does it mean disconnect - r u do iscsi logout? if not and its just delete the mpath device and the phisical devices, then yes - cleanupVolumeDevices or something.
Done
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 205 at r2 (raw file):
Previously, shay-berman wrote…
why?
but u have the indication if you run for iscsi or not. try to check if u have something that can help.
anyway as far as i remember we do run iscsi things if the iscsiadm exist on the host.
Yes, we always run both before, to distinguish them, we need SC to give us more info.
remote/mounter/block_device_mounter_utils/block_device_utils_mounter_test.go, line 298 at r2 (raw file):
Previously, shay-berman wrote…
i guess cleanupVolumeDevices will make more sense
Done.
remote/mounter/block_device_mounter_utils/resources.go, line 21 at r2 (raw file):
Previously, shay-berman wrote…
but the flow keep the same?
yes
remote/mounter/block_device_utils/mpath.go, line 57 at r2 (raw file):
Previously, shay-berman wrote…
ok, just recommendation - if possible try to run staging next time before code review just to make sure the code is ok.
Passed
remote/mounter/block_device_utils/rescan.go, line 88 at r2 (raw file):
Previously, shay-berman wrote…
so its the original code , just moved out to another place?
yes. I was, but in my last commit, I reused it to get the device names (sda, sdb, ...), but logic remain the same
remote/mounter/block_device_utils/rescan.go, line 97 at r2 (raw file):
Previously, shay-berman wrote…
if its important open ticket to remember. anyway maybe add how to improve it in one liner.
Done.
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, shay-berman wrote…
do we support such use case? if its a code that never get used then please remove it - less things to test.
it can make our code more robust
remote/mounter/initiator/linuxfc.go, line 64 at r2 (raw file):
Previously, shay-berman wrote…
maybe i miss it but i ment for golang const like at the top
const Pi = 3.14
fixed
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, shay-berman wrote…
r u sure its built in tool? did u checked it with rhel 7? and other OS we support? I am not sure its comes built in (i ddn't have it on ubuntu)
anyway you need to be accurate - do u ask for the user to install it or not? and if not how it will impact? what will get wrong ? what is the risk of not having it?if there is no risk and we don't loose nothing - then drop it (less code to test). if its important for the release then make sure u document that this tool must be install on the host as part of the pre-requisites.
It's important, I will ask Dima to add it.
remote/mounter/initiator/linuxfc.go, line 70 at r2 (raw file):
Previously, shay-berman wrote…
got u
anyway i would put timeout - who knows what could get wrong. we try to put timeout to all OS commands
Done.
remote/mounter/initiator/linuxfc.go, line 72 at r2 (raw file):
Previously, shay-berman wrote…
of the warning ... systool...
Not necessary for this PR, there are many error messages like this.
remote/mounter/initiator/linuxfc.go, line 78 at r2 (raw file):
Previously, shay-berman wrote…
ok so discover will fail to find it, right? if so its good to go
yes
remote/mounter/initiator/linuxfc.go, line 100 at r2 (raw file):
Previously, shay-berman wrote…
as mentioned only if it bring value
if not lets don't tell the customer to install another rpm
we need it, will ask Dima to add it
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
Previously, shay-berman wrote…
is cinder use this tool?
what will happend if u echo to a none active hba?
yes, it takes time, will slow down our rescan
remote/mounter/initiator/connectors/fibre_channel.go, line 70 at r2 (raw file):
Previously, shay-berman wrote…
not sure
OK
remote/mounter/initiator/connectors/fibre_channel.go, line 118 at r2 (raw file):
Previously, shay-berman wrote…
not sure
try to make sure u take the device, sometimes device are not sd..., its something the client can change in udev. so take care
Done.
Signed-off-by: 27149chen <[email protected]>
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, 7 unresolved discussions (waiting on @27149chen and @olgashtivelman)
remote/mounter/scbe.go, line 55 at r2 (raw file):
Previously, 27149chen wrote…
It just a default value, this method will be used in both mount and unmount, in unmount we can not get the lunid because it is unmapped. If I don't set it to -1, the default value will be 0, which is wrong.
why a function that named "prepareVolumeMountProperties" also used in unmount.
maybe u can use it only in mount and fail if no lunID given. and then another function for unmap where you don't care about lunid.
then the logic will be explicit for the use case.
thoughts?
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
Previously, 27149chen wrote…
yes
can u clarify why?
what is the impact? I think that this discovery was done in order to reduce the number of rescans if its already exist. So do u think its not needed after your change? if so why.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 205 at r2 (raw file):
Previously, 27149chen wrote…
Yes, we always run both before, to distinguish them, we need SC to give us more info.
so the logic remains the same or you changed it?
before did we 'cleanup' iscsi devices even if if its not iscsi? Can you remind me what is the cleanup do for iscsi devices that is not doing for SCSI devices?
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, 27149chen wrote…
it can make our code more robust
let me ask it different -
is there a way that our client can do something to trigger this code (- - - instead of - - lunID)?
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, 27149chen wrote…
It's important, I will ask Dima to add it.
please clarify
- Do you going to instruct the customer to install this systool on his worker nodes?
- Can you confirm if its built in in rhel7 and ubuntu. and if we support suse then also if its exist in suse.
- So if its mandatory and you instruct the customer to install it, then its better to fail if not exist.
if its not mandatory and the code can handle without any issue even if systool not exist, then don't instruct the user to install it.
So what is the decision here?
remote/mounter/initiator/linuxfc.go, line 100 at r2 (raw file):
Previously, 27149chen wrote…
we need it, will ask Dima to add it
again if its mandatory, then fail if not exist and make sure dima instruct the customer to install the relevant package.
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
Previously, 27149chen wrote…
yes, it takes time, will slow down our rescan
does cinder works even if this systool not exist?
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, 7 unresolved discussions (waiting on @olgashtivelman and @shay-berman)
remote/mounter/scbe.go, line 55 at r2 (raw file):
Previously, shay-berman wrote…
why a function that named "prepareVolumeMountProperties" also used in unmount.
maybe u can use it only in mount and fail if no lunID given. and then another function for unmap where you don't care about lunid.
then the logic will be explicit for the use case.
thoughts?
mountProperties fits for both mount and unmount, cinder will reuse the mount properties in unmount, k8s will store the mount properties in cache, and use it in unmount too.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
Previously, shay-berman wrote…
can u clarify why?
what is the impact? I think that this discovery was done in order to reduce the number of rescans if its already exist. So do u think its not needed after your change? if so why.
I didn't change the logic outside the rescan, so if there is a discovery before rescan, it will be still there
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 205 at r2 (raw file):
Previously, shay-berman wrote…
so the logic remains the same or you changed it?
before did we 'cleanup' iscsi devices even if if its not iscsi? Can you remind me what is the cleanup do for iscsi devices that is not doing for SCSI devices?
- the same.
- yes
- they are totally different, iscsi will use iscsiadm
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, shay-berman wrote…
let me ask it different -
is there a way that our client can do something to trigger this code (- - - instead of - - lunID)?
in some odd cases that we can't get the lunid, maybe it is a strange id, maybe there is a population issue in sc, maybe there is a connection issue for a short time, maybe we lose the info in ubiquity because of some reason.
since the next steps all happen in host side, we should make sure that it can continue, instead of stopping because of a non-fatal error.
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, shay-berman wrote…
please clarify
- Do you going to instruct the customer to install this systool on his worker nodes?
- Can you confirm if its built in in rhel7 and ubuntu. and if we support suse then also if its exist in suse.
- So if its mandatory and you instruct the customer to install it, then its better to fail if not exist.
if its not mandatory and the code can handle without any issue even if systool not exist, then don't instruct the user to install it.
So what is the decision here?
- yes.
- It is builtin if it is a full-install. and it is in the local iso, no need to download from outside.
- ok, I will fail it.
remote/mounter/initiator/linuxfc.go, line 100 at r2 (raw file):
Previously, shay-berman wrote…
again if its mandatory, then fail if not exist and make sure dima instruct the customer to install the relevant package.
Done.
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
Previously, shay-berman wrote…
does cinder works even if this systool not exist?
no, it is mandatory.
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, 7 unresolved discussions (waiting on @olgashtivelman and @shay-berman)
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, 27149chen wrote…
- yes.
- It is builtin if it is a full-install. and it is in the local iso, no need to download from outside.
- ok, I will fail it.
- Think again, we should not fail, maybe systool is installed but can't function well, maybe it has compatible issue with some other tools, we should continue, since we have a workaround. This is also for the consideration of robustness
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, 5 unresolved discussions (waiting on @27149chen and @olgashtivelman)
remote/mounter/scbe.go, line 55 at r2 (raw file):
Previously, 27149chen wrote…
mountProperties fits for both mount and unmount, cinder will reuse the mount properties in unmount, k8s will store the mount properties in cache, and use it in unmount too.
can u explain "cinder will reuse the mount properties in unmount, k8s will store the mount properties in cache, and use it in unmount too." ?
who store in cache and what? not sure I understand you.
anyway, just tried to give u feedback about making it more narrow for your use case(since lunid -1 is not relevant for unmap and nor to map). if u think its ok then let it be.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
Previously, 27149chen wrote…
I didn't change the logic outside the rescan, so if there is a discovery before rescan, it will be still there
so just ask - is there any impact by removing this discovery? something we loose?
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, 27149chen wrote…
in some odd cases that we can't get the lunid, maybe it is a strange id, maybe there is a population issue in sc, maybe there is a connection issue for a short time, maybe we lose the info in ubiquity because of some reason.
since the next steps all happen in host side, we should make sure that it can continue, instead of stopping because of a non-fatal error.
so how QA should test this feature? if its not testable(because client cannot get to this situation) then consider to remove it please.
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, 27149chen wrote…
- Think again, we should not fail, maybe systool is installed but can't function well, maybe it has compatible issue with some other tools, we should continue, since we have a workaround. This is also for the consideration of robustness
please decide if systool is mandatory or not for the client. if so make sure you have instruction in UG.
I think it will be confusing to say - if u don't have systool then the driver will work but slower.
Since it looks like u want to support with and without systool on the host. Can u explain what is the customer impact if there is systool and what will be the impact if there is no systool?
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
Previously, 27149chen wrote…
no, it is mandatory.
so why you insist to support host without systool?
just take the call, but it should be very clear to the user - you need it the tool or not. and if not what is the impact.
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, 4 unresolved discussions (waiting on @27149chen and @olgashtivelman)
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 173 at r2 (raw file):
Previously, shay-berman wrote…
so just ask - is there any impact by removing this discovery? something we loose?
my mistake - the reviewable for some strange reason hide the next lines. So its there and all it ok.
comment resolve - sorry - next time i will double check if reviewable hide lines
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, 4 unresolved discussions (waiting on @olgashtivelman and @shay-berman)
remote/mounter/scbe.go, line 55 at r2 (raw file):
Previously, shay-berman wrote…
can u explain "cinder will reuse the mount properties in unmount, k8s will store the mount properties in cache, and use it in unmount too." ?
who store in cache and what? not sure I understand you.anyway, just tried to give u feedback about making it more narrow for your use case(since lunid -1 is not relevant for unmap and nor to map). if u think its ok then let it be.
- cinder: mount and unmount are two methods in same class, both will use same mount properties.
- k8s: store the properties in a cache object called actualStateOfWorld when mount, and unmount will reuse it.
- we can let it be
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, shay-berman wrote…
so how QA should test this feature? if its not testable(because client cannot get to this situation) then consider to remove it please.
I don't agree, QA can't can't cover 100% situations, it is impossible.
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, shay-berman wrote…
please decide if systool is mandatory or not for the client. if so make sure you have instruction in UG.
I think it will be confusing to say - if u don't have systool then the driver will work but slower.Since it looks like u want to support with and without systool on the host. Can u explain what is the customer impact if there is systool and what will be the impact if there is no systool?
- Yes, UG is updated.
- We don't need to tell user about it, just ask him to install.
- I have explained that for now without systool is not a fatal error so that we should not fail, but we should have systool, it will speed up the rescan.
remote/mounter/initiator/linuxfc.go, line 139 at r2 (raw file):
Previously, shay-berman wrote…
so why you insist to support host without systool?
just take the call, but it should be very clear to the user - you need it the tool or not. and if not what is the impact.
without systool is not a fatal error so that we should not fail, but we should have systool, it will speed up the rescan. So we just tell the user that he must install this tool
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, all discussions resolved (waiting on @olgashtivelman)
remote/mounter/initiator/linuxfc.go, line 53 at r2 (raw file):
Previously, 27149chen wrote…
I don't agree, QA can't can't cover 100% situations, it is impossible.
your call
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, 27149chen wrote…
- Yes, UG is updated.
- We don't need to tell user about it, just ask him to install.
- I have explained that for now without systool is not a fatal error so that we should not fail, but we should have systool, it will speed up the rescan.
ok
please make sure Qa will test without systool if you plan to support without it.
There was a problem hiding this comment.
Reviewable status: 0 of 41 files reviewed, all discussions resolved (waiting on @olgashtivelman)
remote/mounter/initiator/linuxfc.go, line 66 at r2 (raw file):
Previously, shay-berman wrote…
ok
please make sure Qa will test without systool if you plan to support without it.
ok, I will add comment under the ticket
There was a problem hiding this comment.
- please update design wiki page to make it up2date with this change.
- Run staging before merging to dev.
- Please watch also a9000 production ci job to make sure no impact on it as well.
Reviewable status: 0 of 41 files reviewed, all discussions resolved (waiting on @olgashtivelman)
Signed-off-by: 27149chen <[email protected]>
|
@27149chen - note : chmod-mountpoint branch is not related to this rescan PR, so there is no point to merge it inside this PR. |
The purpose of this PR is to drop iscan-scsi-bus.sh, including:
Rescan
systool -c fc_host -vto get all the active fc hbas (host0, host1, ...)echo "- - lunid" > /sys/class/scsi_host/host0/scanto scan each hbaDelete
3. get all the multipath devices (sda, sdb, ...) of a volume from output of command
multipath -ll4. using
echo 1 > /sys/block/sda/device/deleteto delete each deviceThe main change of this PR is adding three files:
It is a very lite version of os_brick.
This change is