Thursday, 2020-04-23

*** brinzhang has joined #openstack-cyborg00:12
*** swp20 has joined #openstack-cyborg00:12
*** songwenping_ has quit IRC00:14
*** brinzhang_ has quit IRC00:14
*** songwenping_ has joined #openstack-cyborg00:28
*** swp20 has quit IRC00:32
*** songwenping__ has joined #openstack-cyborg00:38
*** songwenping_ has quit IRC00:42
*** brinzhang has quit IRC00:42
*** songwenping_ has joined #openstack-cyborg01:08
*** songwenping__ has quit IRC01:11
*** songwenping__ has joined #openstack-cyborg02:12
*** songwenping_ has quit IRC02:15
*** songwenping_ has joined #openstack-cyborg02:32
*** songwenping__ has quit IRC02:35
*** xinranwang has joined #openstack-cyborg02:55
*** Sundar has joined #openstack-cyborg02:58
*** s_shogo has joined #openstack-cyborg02:58
*** brinzhang has joined #openstack-cyborg02:59
SundarHi all03:01
Sundar#startmeeting openstack-cyborg03:01
openstackMeeting started Thu Apr 23 03:01:58 2020 UTC and is due to finish in 60 minutes.  The chair is Sundar. Information about MeetBot at
openstackUseful Commands: #action #agreed #help #info #idea #link #topic #startvote.03:02
*** openstack changes topic to " (Meeting topic: openstack-cyborg)"03:02
openstackThe meeting name has been set to 'openstack_cyborg'03:02
s_shogoHi all03:02
songwenping_Hi all03:02
xinranwangHi all03:02
*** Yumeng has joined #openstack-cyborg03:03
*** chenke has joined #openstack-cyborg03:03
Yumeng#info Yumeng03:03
brinzhanghi all03:03
Yumenghi all03:03
SundarGood, we have a quorum.03:03
SundarDoes anybody have a specific thing to discuss?03:04
chenkehi all03:04
chenke#info chenke03:04
xinranwang#info xinranwang03:04
s_shogo#info s_shogo03:04
brinzhang#info brinzhang03:05
*** shaohe_feng has joined #openstack-cyborg03:05
Yumeng hi all, pls help to review and merge
brinzhangI have some summary work in my company, so there few work take here in this meeting, I am sorry03:06
brinzhangYumeng: I reviewed the first patch, I hope you can add the test case to cover your changes03:06
SundarLooking at
Yumengbrinzhang: I just replied your comments:
SundarAgree with Yumeng -- bandit fixes are important03:06
*** haibin-huang has joined #openstack-cyborg03:07
SundarI'll review them after this meeting03:07
brinzhangYumeng: I mean, you should add test case to test
YumengSundar: Thanks!03:07
brinzhangthe fpga_program_v2()03:07
brinzhangWe can’t give up adding new use cases because they do n’t affect existing use cases, can we?03:08
SundarAlso, I'd say the functional tests patch is important03:08
Yumengbrinzhang: I know. fpga_program_v2() here just made a temp file and downloaded the image, then removed the file. it's more like a file operation. If we add unit test for fpga_program_v2(), they are all by mock.03:09
brinzhangSundar, there is an issue, that cannot work successful in my local, and I havenot found the reason now03:09
Yumengbrinzhang: that's why at the beginnig , we didn't  add unit test for this func03:09
brinzhangif anyone good at functional test, please give some help ^, thanks03:10
Sundarbrinzhang: Are you saying the functional tests do not run in your local env, or they run but do not pass?03:10
brinzhangSundar, that why I am confusing, so I give -1 in this patch03:10
brinzhangyou can run tox -e functional in you local env, and test. The test cannot cover the functional dir too.03:11
xinranwangI looked into brinzhang's functional test issue, It seems that all funtional tests code are not loaded successfully.03:11
brinzhangxinranwang: right03:11
brinzhangYumeng: I understand, the UT mainly keep the function work fine, mock data is enough, I think03:12
shaohe_fengthe problem is: str + UUID ?03:12
brinzhangshaohe_feng: yes, I reviewed your comments in PS3, and left some comments in latest patch03:14
shaohe_fengYumeng have check the type of uuid?  seems chenke has checked it,  already str format.03:14
chenkeI had alerady check it. str format.03:15
chenkeBut functional test may be some error about this type.03:15
*** tetsuro has joined #openstack-cyborg03:16
*** tetsuro has quit IRC03:16
chenkeFrom the logical point of view of the code, when calling the method, uuid is read from the device profile, which is itself str, and we do not need to do conversion.03:16
Yumengshaohe_feng: yes, uuid. shogo has helped check this patch in his real FPGA env.03:16
shaohe_fengmaybe not the source code error03:17
shaohe_fengsomething wrong it the functional test code.03:17
brinzhangchenke: we also should keep the UT can works fine03:17
chenke@brin .  Did you encounter the problem when running the fun test?03:17
shaohe_fengs/something wrong it the functional test code.03:17
Yumengif not uuid, it should have raised an error03:17
chenkeI guess.03:18
brinzhangchenke: I paste my test step, you can review again03:18
*** Sundar has quit IRC03:18
*** Sundar has joined #openstack-cyborg03:19
brinzhangYumeng: so, there will be negative and active scenario need to be test, if the bitstream_uuid is str, or not str03:19
SundarGot disconnected when I logged into my VPN03:19
s_shogoI also check the test test of brin.03:19
s_shogos/test test/test step03:19
Yumengbrinzhang:  I think the mian difference is  this step of your test step: bitstream_uuid=uuid.uuid1(). this step manually set the bitstream_uuid type to uuid object03:22
SundarYumeng: In, do we want to run bash with some unknown variable as argument? That can be insecure too.03:22
brinzhangif there is a UT, that we can see clear their different03:22
chenkeactually. UT for this method, only can verify the logic of method. The UT will not ensure the type of uuid.  We can not expect UT and do everything for us. It can help our code more strongly.03:25
SundarTo check if a value is a UUID, we can do: try:03:27
SundarThere must be some oslo check for it too. Checking now ...03:27
YumengSundar: emmm.. seems in spdk start_server() function, we cannot avoid the server_name (unkown variable) as argument , but without shell=True is much better than before03:27
brinzhangSundar: what do your consider? I am not very familiar with bandit03:28
SundarFor UUID: oslo_utils.uuidutils.is_uuid_like(val)03:28
chenkeAs we often hear, practice is the only criterion for testing truth. I think your test is very good. If necessary, I think we can add a uuid type check. If it doesn’t pass, throw an exception. Do you think this is a good idea?03:28
YumengSundar: I will talk to Li Liu, and see if he has better suggestion. I will sync you later.03:29
brinzhangSundar: yes, this just only check the parameter is a uuid, not determine it's type03:29
shaohe_fengcan we double check to make make the uuid is srting03:29
Sundarself.assertTrue( oslo_utils.uuidutils.is_uuid_like(val) )03:29
brinzhangso add UT to cover this change, do you agree?03:29
songwenping_Just looking at this patch. Brin's error occurred in _os.path.join(dir, pre + name + suf). Should we check the name type?03:30
shaohe_fengformat again str(uuid)03:31
chenkeagree and a type check about uuid. and add a test to cover this check situation.03:31
shaohe_fengIMHO, a good function should can handle both UUID or str type both03:32
songwenping_should we add try...except at download_path generate?03:32
brinzhangadd str() just only ensure the parameter is right, of course, that also need to add UT03:32
SundarYumeng: Re. SPDK, it was added early on, but I don't know if anybody has checked that it works. Sure, please check. SInce tomorrow is the deadline for RC1 for Ussuri, I think we can aim to close other bandit patches except this one.03:33
brinzhangSundar, Yumeng: add the UT, that can accept before the deadline of RC1, otherwise, I would rather postpone, we cannot merge the ERROR logical03:35
xinranwangs_shogo:  do you want talk about program API?03:36
shaohe_fengany way, str(uuid) is not bad.03:37
Sundarbrinzhang: of course, others need to be fixed and reviewed.03:37
shaohe_fengyes, how is the process of program API?03:37
YumengSundar:  Re SPDK. I will sync with Li today, I can make a quick update today. anyway, current change should be at least better than now. IMHO, also safe to merge.03:38
SundarYumeng, the bandit patches do not seem to form a series? Enabling bandit as voting job depends only on the SPDK patch
YumengSundar: Enabling bandit as voting job depends on the series actually. the four patches03:38
Yumengbut the four patches are independet.03:39
Yumengare mutually independet.03:40
brinzhangYumeng: this one is enough, your patches alread have a rebase logical03:40
s_shogo@xinranwang Thanks for your mention, related to program API, as you commented, I would like to discuss how to select the command, like "fpgaconf" or "fpgasupdate" from OPAE version.03:41
SundarYumeng: I see. Usually, we don;t use Depends-On for patches within the same project. But I see what you are trying to do.03:41
SundarI gave +2 with the understanding that other patches need to merge first03:42
YumengSundar, brinzhang: Yes. at first,I didn't use Depends-On for patches within our project. later I got some question, so just add this for more clarification for others to quickly review.03:44
Sundar@s_shogo xinranwang: Ideally, we shouldn't hardcode the command name fpgasupdate or fpgaconf. One possible approach is to have metadata with the bitstream image that identifies its type, and use that type to identify the command.03:44
xinranwangit is also depends on which version of OPAE we installed, right ?03:45
YumengSundar, brinzhang and all: agree with double check uuid to make sure it's a str, and I will double check the unittest03:45
s_shogoI tried get OPAE version from "fpgainfo fme", but that format seems to have difference between some OPAE version03:45
shaohe_fengwe can push OPAE team to provide a unify API for program03:45
brinzhangThe depend-on, just only ensure the latest patch cannot merged earlier than the base patach, so it can add in the same project, in nova there is a common case03:45
SundarI had once thought of using the image suffix, whether it is ".gbs" or not, to indeitfy the command. However, AFAIK, the bitstreams that need fpgaconf or fpgasupdate may both have the same suffix03:45
shaohe_fengLet's them distinguish to different bitstream format and call the right cmd03:46
shaohe_fengIn my former  demo, I did not  change any cyborg code.03:47
Sundarxinranwang: Yes, it depends on OPAE version too. However, if we let the operator install OPAE libraries and we remove that OPAE version setting from devstack -- which would be a good thing -- we would not be able to use the version.03:47
SundarMay be this can be a goal for Victoria release :)03:47
shaohe_fengI just write a shell wrap to call fpgasupdate or fpgaconf.03:48
shaohe_fengI can do, OPAE team can also distinguish them=D03:48
Sundarshaohe_feng: In a production env, with different types of FPGAs from different vendors, we would need a better mechanism.03:48
SundarIt is not just OPAE -- but hopefully other vendors too03:48
shaohe_fengYes, but that's hard. No now know all vendors product well. So different vendors  maintainer their special code :')03:50
SundarThe special code must be inside their resp. FPGA drivers. The image file need only have metadata that indicates the image type, maybe 'accel:bitstream_type=gbs' in Glance metadata.03:51
shaohe_fengFor image upload,  it is necessary to call the vendors special tools to parser the metadata of their image03:52
shaohe_fengand format glance metadata03:53
shaohe_fengwe are tying to do it. This can make user more easy to user cyborg03:53
shaohe_fengand avoid error prone03:53
Sundarshaohe_feng: That would be useful. Nova/Glance developers told me that it is better if Cyborg does not call Glance API for that. Whoever does the image upload should just add the metadata, and Cyborg can check whether it is present.03:54
xinranwangIt is better to let operator choose the right version, and configure it in conf file, cyborg just need to read this file and choose the right commands.03:55
shaohe_fengSundar,  agree.  I'm make the planing to improve it.03:55
Sundarxinranwang: The conf file approach is fine too, as long as there are not too many image types. Right now, there is only 1, so no problem :)03:56
s_shogo(IMO, multiple OPAE in single env is little difficult to operate.)03:57
Sundars_shogo: Agreed. However, unfortunately, different FPGA cards require different versions of OPAE today. If you need to use more than one in the same cluster, maybe you can put them in different compute nodes, so that each node has only 1 OPAE version.03:58
xinranwangshaohe_feng Sundar does OPAE team plan to unify a set of API for different cards?04:00
s_shogoSundar, Thanks, I missed that possibility.that's right.04:00
shaohe_fengxinranwang let us feedback to them and push them -_-04:00
shaohe_fengI guess no.  we can user, but they are not.04:02
Sundarxinranwang: I can explain that offline, because we are hitting the top of the hour.04:02
shaohe_fengso they do not know the pain-point04:02
SundarAny other topic for today?04:03
SundarTomorrow is the last day for RC release. We may not be able to merge any patches for Ussuri after that.04:04
SundarThanks a lot to all of you. Have a good day. Bye.04:04
*** openstack changes topic to "Pending patches (Meeting topic: openstack-cyborg)"04:05
xinranwangSundar:  ok04:05
openstackMeeting ended Thu Apr 23 04:05:00 2020 UTC.  Information about MeetBot at . (v 0.1.4)04:05
openstackMinutes (text):
shaohe_fengdo we still need to refactor the accelerator driver04:05
*** tetsuro has joined #openstack-cyborg04:11
*** s_shogo has quit IRC04:18
*** Sundar has quit IRC04:40
*** brinzhang_ has joined #openstack-cyborg04:44
*** brinzhang has quit IRC04:47
*** shaohe_feng has quit IRC05:34
*** chenke has quit IRC06:03
*** links has joined #openstack-cyborg08:11
*** igordc has quit IRC08:13
openstackgerritYumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory]
openstackgerritYumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory]
openstackgerritYumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory]
openstackgerritYumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory]
*** tetsuro has quit IRC09:24
openstackgerritMerged openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory]
*** links has quit IRC10:52
*** links has joined #openstack-cyborg11:02
openstackgerritzhurong proposed openstack/cyborg master: Fix bandit error: Ascend driver:[B602:subprocess_popen_with_shell_equals_true]
openstackgerritzhurong proposed openstack/cyborg master: Fix bandit error: [B104:hardcoded_bind_all_interfaces]
openstackgerritYumengBao proposed openstack/cyborg master: Fix bandit error: Ascend driver:[B602:subprocess_popen_with_shell_equals_true]
*** jraju__ has joined #openstack-cyborg12:41
*** links has quit IRC12:41
*** haibin-huang has quit IRC12:43
openstackgerritMerged openstack/cyborg master: Fix bandit error: [B104:hardcoded_bind_all_interfaces]
*** igordc has joined #openstack-cyborg13:07
*** xinranwang has quit IRC13:24
*** jraju__ has quit IRC17:14
*** igordc has quit IRC19:39
*** igordc has joined #openstack-cyborg20:32
*** igordc has quit IRC21:51
*** igordc has joined #openstack-cyborg22:26
*** igordc has quit IRC23:05

Generated by 2.15.3 by Marius Gedminas - find it at!