Closed hfeish closed 5 years ago
coveralls
commented
6 years ago Coverage decreased (-0.4%) to 52.325% when pulling e87f9ea7ff6e2a0666c2393aacb0d71bb0d52a99 on fix/UB-1103_DS8k_need_lun0_before_rescan into 4cadd2acec7fc77182550f27408d608cc5da46c7 on dev.
hfeish
commented
6 years ago @tzurE , please help to review, thanks!
ranhrl
commented
6 years ago Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.
remote/mounter/scbe.go, line 58 at r1 (raw file):
// Known issue with rescan-scsi-bus.sh, LUN0 must be the first mapped logical unit, otherwise it will not be able to scan any other logical unit // For DS8k, it only support FC and without LUN0 mapped default, so we need to do "rescan-scsi-bus.sh -a" to discover the LUN0 if s.config.SkipRescanISCSI {
This name "SkipRescanISCSI" is a bad way of saying "ConfigurationIsISCSI" or "ConfigurationIsFC". I already discussed it once with Shay, I am not sure if there is a ticket to change it to a more readable name.
remote/mounter/scbe.go, line 60 at r1 (raw file):
if s.config.SkipRescanISCSI { if err := s.blockDeviceMounterUtils.RescanAllTargets(!s.config.SkipRescanISCSI, volumeWWN); err != nil { return "", s.logger.ErrorRet(err, "RescanAll failed again")
Please be more accurate with the message of the error. "RescanAll failed with error code {} for device {volumeWWN}"
remote/mounter/block_device_utils/rescan.go, line 27 at r1 (raw file):
defer b.logger.Trace(logs.DEBUG)() switch protocol {
protocol value could be either FC, ISCSI or both, I think we should refactor to reflect it. It is not clear what SCSI means if you have FC and ISCSI.
remote/mounter/block_device_utils/rescan.go, line 76 at r1 (raw file):
commands := []string{"rescan-scsi-bus", "rescan-scsi-bus.sh"} rescanCmd := "" for _, cmd := range commands {
Code duplication, you should have a separate method to get the correct rescan command and use it for both.
remote/mounter/block_device_utils/rescan.go, line 85 at r1 (raw file):
return b.logger.ErrorRet(&commandNotFoundError{commands[0], errors.New("")}, "failed") } args := []string{"-a"} // Only for the Lun0 rescan on DS8k
the comment should state rescan all targets if lun was not find in order to find a lun0 on a new target.
Comments from Reviewable
tzurE
commented
6 years ago Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.
remote/mounter/scbe.go, line 58 at r1 (raw file):
This name "SkipRescanISCSI" is a bad way of saying "ConfigurationIsISCSI" or "ConfigurationIsFC". I already discussed it once with Shay, I am not sure if there is a ticket to change it to a more readable name.
+1
remote/mounter/block_device_utils/rescan.go, line 76 at r1 (raw file):
Code duplication, you should have a separate method to get the correct rescan command and use it for both.
+1
Comments from Reviewable
hfeish
commented
6 years ago Review status: 0 of 8 files reviewed at latest revision, 5 unresolved discussions.
remote/mounter/scbe.go, line 58 at r1 (raw file):
+1
I don't find the ticket in Jira, do we need to open a new ticket to handle this or fix this in this PR? If we need to fix now, any details about the new name?
remote/mounter/scbe.go, line 60 at r1 (raw file):
Please be more accurate with the message of the error. "RescanAll failed with error code {} for device {volumeWWN}"
Changed to s.logger.ErrorRet(err, "RescanAll Targets failed", logs.Args{{"volumeWWN", volumeWWN}})
remote/mounter/block_device_utils/rescan.go, line 27 at r1 (raw file):
protocol value could be either FC, ISCSI or both, I think we should refactor to reflect it. It is not clear what SCSI means if you have FC and ISCSI.
Changed to BOTH, ISCSI, FC
remote/mounter/block_device_utils/rescan.go, line 76 at r1 (raw file):
+1
Changed to pass "-a" or "-r" as a string arg to the RescanSCSI
Comments from Reviewable
ranhrl
commented
6 years ago Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.
remote/mounter/scbe.go, line 58 at r1 (raw file):
I don't find the ticket in Jira, do we need to open a new ticket to handle this or fix this in this PR? If we need to fix now, any details about the new name?
You can open a new ticket and discuss it with Shay when he is back
Comments from Reviewable
hfeish
commented
6 years ago Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.
remote/mounter/scbe.go, line 58 at r1 (raw file):
You can open a new ticket and discuss it with Shay when he is back
Open UB-1187
Comments from Reviewable
shay-berman
commented
6 years ago Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.
a discussion (no related file): Is this fix is mandatory? I don't like that we doing additional rescan if we don't find the device. Lets talk about it offline.
Comments from Reviewable
ranhrl
commented
6 years ago Reviewed 2 of 7 files at r1, 6 of 6 files at r2. Review status: all files reviewed at latest revision, 5 unresolved discussions.
a discussion (no related file):
Is this fix is mandatory? I don't like that we doing additional rescan if we don't find the device. Lets talk about it offline.
What is the alternative, Shay?
Comments from Reviewable
shay-berman
commented
6 years ago Review status: all files reviewed at latest revision, 5 unresolved discussions.
a discussion (no related file):
What is the alternative, Shay?
if rescan -a is only for ds8k use case, then I would like to make sure it will run only for ds8k use case. For example, you can identify if the wwn that you want to discover is for ds8k, if so then you can add this flow (if the first discover failed then run another one with rescan -a. but only for ds8k luns).
In xiv you have the ability to identify if the wwn is xiv or a9000 system, maybe its the same for ds8k.
Comments from Reviewable
hfeish
commented
6 years ago @shay-berman , please help to review
beckmani
commented
6 years ago local/scbe/scbe.go, line 446 at r3 (raw file):
//Save the lun number in the map, if get the same wwn, then update the lunNumber with latest lunNumberDict[existingVolume.WWN] = scbeResponseMap.LunNumber
I suggest that you log the case where the lunNumber in the dict is different from the current lun number
Comments from Reviewable
beckmani
commented
6 years ago remote/mounter/scbe.go, line 46 at r3 (raw file):
func NewTestScbeMounter(scbeRemoteConfig resources.ScbeRemoteConfig, blockDeviceMounterUtils block_device_mounter_utils.BlockDeviceMounterUtils) resources.Mounter { return &scbeMounter{ logger: logs.GetLogger(),
Too many spaces.
Comments from Reviewable
beckmani
commented
6 years ago remote/mounter/scbe.go, line 66 at r3 (raw file):
if err != nil { // Known issue with rescan-scsi-bus.sh, LUN0 must be the first mapped logical unit, otherwise it will not be able to scan any other logical unit // For DS8k, it only support FC and without LUN0 mapped default, so we need to do "rescan-scsi-bus.sh -a" to discover the LUN0
Please add a reference url to RH known issue.
Comments from Reviewable
beckmani
commented
6 years ago remote/mounter/scbe_test.go, line 6 at r3 (raw file):
. "github.com/IBM/ubiquity/remote/mounter" . "github.com/IBM/ubiquity/resources" "github.com/IBM/ubiquity/fakes"
Nit: Alignment
Comments from Reviewable
beckmani
commented
6 years ago remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 200 at r3 (raw file):
} b.logger.Debug("rescanLock.TryLock failed", logs.Args{{"error", err}}) time.Sleep(time.Duration(500*time.Millisecond))
Move 500 to be constant
Comments from Reviewable
beckmani
commented
6 years ago remote/mounter/scbe_test.go, line 24 at r3 (raw file):
mountRequestForDS8kLun0 = MountRequest{Mountpoint:"test_mountpointDS8k", VolumeConfig:map[string]interface{}{"Name":"u_vol","PhysicalCapacity":2040, "Profile":"gold", "StorageType":"2107", "UsedCapacity":2040, "Wwn":"wwn", "attach-to":"xnode1",
Use const for PhysicalCapacity, StorageType, Profile, and UsedCapacity (Here and below)
Comments from Reviewable
beckmani
commented
6 years ago Reviewed 2 of 7 files at r1, 5 of 6 files at r2, 6 of 6 files at r3. Review status: all files reviewed, 11 unresolved discussions (waiting on @ranhrl, @tzurE, @hfeish, and @alonm)
Comments from Reviewable
hfeish
commented
6 years ago Review status: 10 of 13 files reviewed, 11 unresolved discussions (waiting on @beckmani, @ranhrl, @tzurE, @hfeish, and @alonm)
local/scbe/scbe.go, line 446 at r3 (raw file):
I suggest that you log the case where the lunNumber in the dict is different from the current lun number
Done
remote/mounter/scbe.go, line 46 at r3 (raw file):
Too many spaces.
Run with gofmt
remote/mounter/scbe.go, line 66 at r3 (raw file):
Please add a reference url to RH known issue.
Add url here
remote/mounter/scbe_test.go, line 6 at r3 (raw file):
Nit: Alignment
run with gofmt
remote/mounter/scbe_test.go, line 24 at r3 (raw file):
Use const for PhysicalCapacity, StorageType, Profile, and UsedCapacity (Here and below)
done
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 200 at r3 (raw file):
Move 500 to be constant
Check that all the para here not use constant, so keep constent
Comments from Reviewable
shay-berman
commented
6 years ago Review status: 10 of 13 files reviewed, 25 unresolved discussions (waiting on @beckmani, @ranhrl, @tzurE, @hfeish, and @alonm)
Dockerfile, line 10 at r4 (raw file):
FROM alpine:3.7 RUN apk --no-cache add ca-certificates=20171114-r0 openssl=1.0.2o-r0
?? how its related to the PR?
local/scbe/scbe.go, line 55 at r4 (raw file):
var ( SupportedFSTypes = []string{"ext4", "xfs"} lunNumberDict = make(map[string]int)
what is this? be careful ubiquity is thread save
local/scbe/scbe.go, line 388 at r4 (raw file):
volConfig[resources.ScbeKeyVolAttachToHost] = hostAttach // Mounter mount will use this extra info to determine is DS8k lun0
fix text "determine if its DS8k lun0"
local/scbe/scbe.go, line 389 at r4 (raw file):
// Mounter mount will use this extra info to determine is DS8k lun0 lunNumber, ok := lunNumberDict[scbeVolume.WWN]
Sorry I don't understand what is it? remember ubiquity server is thread save. the get and set should be atomic
and what happened if ubiquity crash? is this data should be consistent?
Can we get the lun number from SCBE? volume detail? instead of save it in a memory map.
local/scbe/scbe.go, line 394 at r4 (raw file):
} volConfig[resources.LunNumber] = lunNumber
but if !ok, then what is the lunNumber?
local/scbe/scbe.go, line 490 at r4 (raw file):
//Delete the lun number message in the map, will do nothing if wwn not exist in the map delete(lunNumberDict, existingVolume.WWN)
whats happened if it fail?
remote/mounter/scbe.go, line 46 at r3 (raw file):
Run with gofmt
please keep standard, call it NewScbeMounterWithMounterUtil()
and then reuse code and update NewScbeMounter to use this function and pass the blockDeviceMounterUtils := block_device_mounter_utils.NewBlockDeviceMounterUtils() to it reuse code
remote/mounter/scbe.go, line 68 at r4 (raw file):
// For DS8k, it only support FC and without LUN0 mapped default, so we need to do "rescan-scsi-bus.sh -a" to discover the LUN0 // Url: UB-1103 in https://www.ibm.com/support/knowledgecenter/SS6JWS_3.4.0/RN/sc_rn_knownissues.html s.logger.Debug("volumeConfig: ", logs.Args{{"volumeConfig: ", mountRequest.VolumeConfig}})
please be more descriptive in the log, what you want to state here?
remote/mounter/scbe.go, line 69 at r4 (raw file):
// Url: UB-1103 in https://www.ibm.com/support/knowledgecenter/SS6JWS_3.4.0/RN/sc_rn_knownissues.html s.logger.Debug("volumeConfig: ", logs.Args{{"volumeConfig: ", mountRequest.VolumeConfig}}) storageType, ok := mountRequest.VolumeConfig[resources.StorageType]
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 200 at r3 (raw file):
Check that all the para here not use constant, so keep constent
so just move to const - its the right thing todo anyway.
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 174 at r4 (raw file):
} } if err := b.blockDeviceUtils.Rescan(block_device_utils.BOTH); err != nil {
what is BOTH? its confusing
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 189 at r4 (raw file):
// 2. multipathing rescan // return error if one of the steps fail func (b *blockDeviceMounterUtils) RescanAllTargets(withISCSI bool, wwn string) error {
why you are not using the same RescanAll but just with param for all targets? instead of duplicate this rescan logic
remote/mounter/block_device_utils/block_device_utils_test.go, line 82 at r4 (raw file):
It("Rescan SCSI fails if rescan-scsi-bus command missing", func() { fakeExec.IsExecutableReturns(cmdErr) err = bdUtils.Rescan(block_device_utils.BOTH)
i don't like you changed this BOTH thing its not clear and made you change a lot of things.
remote/mounter/block_device_utils/rescan.go, line 28 at r4 (raw file):
switch protocol { case BOTH:
this is very confusing what is BOTH mean? please be more descriptive here.
remote/mounter/block_device_utils/rescan.go, line 52 at r4 (raw file):
} func (b *blockDeviceUtils) RescanSCSI(str string) error {
please call it rescanArgs instead of str which is not descriptive.
resources/resources.go, line 85 at r4 (raw file):
const LunNumber = "LunNumber" const StorageType = "StorageType"what does it mean? is it relevant? please be more descriptive.
Comments from Reviewable
shay-berman
commented
6 years ago @hfeish I review this PR, and I think you need to do a revisit according to my comment. The basic issue is that you don't need to save lun ID in the ubiquity process memory - it could lead to consistent\concurrency issues.
Please review the comments.
shay-berman
commented
6 years ago Review status: 3 of 17 files reviewed, 30 unresolved discussions (waiting on @beckmani, @ranhrl, @tzurE, @hfeish, @shay-berman, and @alonm)
Dockerfile, line 10 at r4 (raw file):
?? how its related to the PR?
please reply Fei
local/scbe/scbe.go, line 55 at r4 (raw file):
what is this? be careful ubiquity is thread save
ok thanks for removing it
local/scbe/scbe.go, line 389 at r4 (raw file):
Sorry I don't understand what is it? remember ubiquity server is thread save. the get and set should be atomic and what happened if ubiquity crash? is this data should be consistent? Can we get the lun number from SCBE? volume detail? instead of save it in a memory map.
ok thanks for fixing it
local/scbe/scbe.go, line 394 at r4 (raw file):
but if !ok, then what is the lunNumber?
ok thanks for removing it
local/scbe/scbe.go, line 386 at r5 (raw file):
volConfig[resources.ScbeKeyVolAttachToHost] = volMapInfo.Host volConfig[resources.LunNumber] = volMapInfo.LunNumber
please keep the conversion of keys ScbeKeyVolAttachLunNumToHost
local/scbe/scbe_rest_client_test.go, line 189 at r5 (raw file):
Expect(err).NotTo(HaveOccurred()) Expect(volMapInfo.Host).To(Equal(fakeHost)) Expect(volMapInfo.LunNumber).To(Equal(fakeLunNumber))
where is the fakelunnum define? i don't see it in the commit.
remote/mounter/scbe.go, line 69 at r4 (raw file):
1. move all this code to function that called isDS8kLun0(volumeconfig object) - cleaner code 2. what is ubiqutiy crash during mount, what about does it still work in the second run? idempotent must be consider here.
Fei, please reply on my comment so i will know that you fix them. then its easier for me to resolve the comment.
remote/mounter/scbe.go, line 63 at r5 (raw file):
// For DS8k, "rescan-scsi-bus.sh -r" cannot discover the LUN0, need to use "rescan-scsi-bus.sh -a" instead s.logger.Debug("volumeConfig: ", logs.Args{{"volumeConfig: ", mountRequest.VolumeConfig}}) isDs8kLun0 := isDS8kLun0(mountRequest)
Fie , there is no need to use this isDs8kLun0 variable, just use the function in the if statement.
you know - "less is more" :-)
remote/mounter/scbe.go, line 141 at r5 (raw file):
func isDS8kLun0(mountRequest resources.MountRequest) bool { storageType, ok := mountRequest.VolumeConfig[resources.StorageType]
please use err instead of ok (i think its standard way - double check me)
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 174 at r4 (raw file):
what is BOTH? its confusing
please reply my comment so i will know what you did to resolve it
remote/mounter/block_device_utils/block_device_utils_test.go, line 100 at r5 (raw file):
}) It("Rescan fails if unknown protocol", func() { err = bdUtils.Rescan(3)
what is 3?
remote/mounter/block_device_utils/rescan.go, line 32 at r5 (raw file):
case ISCSI: return b.RescanISCSI() case FC:
why u call it FC?
its confusing
i don't think u need special protocol for that, it sound more like argument for the rescan.
Comments from Reviewable
hfeish
commented
6 years ago Review status: 2 of 17 files reviewed, 30 unresolved discussions (waiting on @beckmani, @ranhrl, @tzurE, @hfeish, @shay-berman, and @alonm)
Dockerfile, line 10 at r4 (raw file):
please reply Fei
if use openssl=1.0.2n-r0 , it will fail when build the image, so use 1.0.2o instead
local/scbe/scbe.go, line 386 at r5 (raw file):
please keep the conversion of keys ScbeKeyVolAttachLunNumToHost
done
local/scbe/scbe_rest_client_test.go, line 189 at r5 (raw file):
where is the fakelunnum define? i don't see it in the commit.
in local/scbe/scbe_test.go, same place with "fakeHost"
remote/mounter/scbe.go, line 46 at r3 (raw file):
please keep standard, call it NewScbeMounterWithMounterUtil() and then reuse code and update NewScbeMounter to use this function and pass the blockDeviceMounterUtils := block_device_mounter_utils.NewBlockDeviceMounterUtils() to it reuse code
Done
remote/mounter/scbe.go, line 69 at r4 (raw file):
Fei, please reply on my comment so i will know that you fix them. then its easier for me to resolve the comment.
If ubiquity crash during mount, for the second run, it still work
remote/mounter/scbe.go, line 63 at r5 (raw file):
Fie , there is no need to use this isDs8kLun0 variable, just use the function in the if statement. you know - "less is more" :-)
Done
remote/mounter/scbe.go, line 141 at r5 (raw file):
please use err instead of ok (i think its standard way - double check me)
Here we try to find whether the key is in the map, as I found, OK means true, and its a standard way to use OK here
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 200 at r3 (raw file):
so just move to const - its the right thing todo anyway.
Done
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 174 at r4 (raw file):
please reply my comment so i will know what you did to resolve it
Change back to SCSI
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 189 at r4 (raw file):
why you are not using the same RescanAll but just with param for all targets? instead of duplicate this rescan logic
Done
remote/mounter/block_device_utils/block_device_utils_test.go, line 82 at r4 (raw file):
i don't like you changed this BOTH thing its not clear and made you change a lot of things.
Change bake to SCSI
remote/mounter/block_device_utils/block_device_utils_test.go, line 100 at r5 (raw file):
what is 3?
we only support protocol {0, 1, 2}, so if bigger than 2 it will fail
remote/mounter/block_device_utils/rescan.go, line 28 at r4 (raw file):
this is very confusing what is BOTH mean? please be more descriptive here.
change back to SCSI
remote/mounter/block_device_utils/rescan.go, line 52 at r4 (raw file):
please call it rescanArgs instead of str which is not descriptive.
Done
remote/mounter/block_device_utils/rescan.go, line 32 at r5 (raw file):
why u call it FC? its confusing i don't think u need special protocol for that, it sound more like argument for the rescan.
Because for FC we use "-a", and its same level as "ISCSI", or do you have any other suggestion?
resources/resources.go, line 85 at r4 (raw file):
> ``` > const LunNumber = "LunNumber" > const StorageType = "StorageType" > ``` what does it mean? is it relevant? please be more descriptive.
Add description
Comments from Reviewable
shay-berman
commented
6 years ago Review status: 2 of 17 files reviewed, 27 unresolved discussions (waiting on @beckmani, @ranhrl, @tzurE, @hfeish, @shay-berman, and @alonm)
Dockerfile, line 10 at r4 (raw file):
if use openssl=1.0.2n-r0 , it will fail when build the image, so use 1.0.2o instead
this is strange, please check it out in the dev its already openssl=1.0.2o-r0
maybe u didn't create this branch from dev! please check
remote/mounter/scbe.go, line 146 at r6 (raw file):
lunNumber, ok := mountRequest.VolumeConfig[resources.ScbeKeyVolAttachLunNumToHost] if !ok { lunNumber = -1
just return here false, instead of setting to -1
remote/mounter/scbe_test.go, line 73 at r6 (raw file):
}) It("should fail to discover svc with lun1 if failed to discover with '-r' ", func() {
please make sure u have enough testing and full coverage for the change. its hard for me to see the full coverage
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 179 at r6 (raw file):
} } else { if err := b.blockDeviceUtils.Rescan(block_device_utils.FC); err != nil {
its confusing
what is FC vs SCSI???
its not readable
as i mentioned before - you need to pass extra argument for the rescan. so you still have protocol but extra param for rescans.
Comments from Reviewable
hfeish
commented
6 years ago Review status: 1 of 16 files reviewed, 27 unresolved discussions (waiting on @beckmani, @ranhrl, @tzurE, @hfeish, @shay-berman, and @alonm)
Dockerfile, line 10 at r4 (raw file):
this is strange, please check it out in the dev its already openssl=1.0.2o-r0 maybe u didn't create this branch from dev! please check
It because it still 1.0.2n-r0 when I create this branch from dev at Apr 2, too long ago. SO I change to 1.0.2o-r0 here otherwise it will fail to build the test build
remote/mounter/scbe.go, line 146 at r6 (raw file):
just return here false, instead of setting to -1
Done
remote/mounter/scbe_test.go, line 73 at r6 (raw file):
please make sure u have enough testing and full coverage for the change. its hard for me to see the full coverage
Add test cases
remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go, line 179 at r6 (raw file):
its confusing what is FC vs SCSI??? its not readable as i mentioned before - you need to pass extra argument for the rescan. so you still have protocol but extra param for rescans.
use readable para here
Comments from Reviewable
yixuan0825
commented
6 years ago @shay-berman ， Hi Shay, could you help to review the new patch based on Fei's patch and your comments.
shay-berman
commented
5 years ago what is the status of this PR?
yixuan0825
commented
5 years ago It's the pull request by Fei, and it is the old design. I will close this PR.
Installation of Ubiquity fails on DS8k because rescan-scsi-bus.sh hit a LUN0 issue, to avoid this we use "rescan-scsi-bus.sh -a" if the previous rescan "rescan-scsi-bus.sh -r" fail, and since "rescan-scsi-bus.sh -a" is slowly than "rescan-scsi-bus.sh -r", we still use the "rescan-scsi-bus.sh -r" as default rescan
Below is the procedure:
Run sanity passed, will run with XAVI
Signed-off-by: feihuang feihuang@feihuangs-mbp.cn.ibm.com
This change is