Earlier  
Later
New Search
Posted Nick Remark
#openstack-cyborg - 2020-04-23
03:22:57 brinzhang if there is a UT, that we can see clear their different
03:25:34 chenke actually. 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:26:22 chenke s/and/can/
03:27:20 Sundar To check if a value is a UUID, we can do: try:
03:27:43 Sundar There must be some oslo check for it too. Checking now ...
03:27:56 Yumeng Sundar: 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 before
03:28:12 brinzhang Sundar: what do your consider? I am not very familiar with bandit
03:28:46 Sundar For UUID: oslo_utils.uuidutils.is_uuid_like(val)
03:28:57 chenke As 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:29:16 Yumeng Sundar: I will talk to Li Liu, and see if he has better suggestion. I will sync you later.
03:29:34 brinzhang Sundar: yes, this just only check the parameter is a uuid, not determine it's type
03:29:45 shaohe_feng can we double check to make make the uuid is srting
03:29:48 shaohe_feng string
03:29:50 Sundar self.assertTrue( oslo_utils.uuidutils.is_uuid_like(val) )
03:29:55 brinzhang so add UT to cover this change, do you agree?
03:30:16 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:18 shaohe_feng in
03:31:20 shaohe_feng format again str(uuid)
03:31:23 chenke agree and a type check about uuid. and add a test to cover this check situation.
03:32:28 shaohe_feng IMHO, a good function should can handle both UUID or str type both
03:32:28 songwenping_ should we add try...except at download_path generate?
03:32:51 brinzhang add str() just only ensure the parameter is right, of course, that also need to add UT
03:33:05 Sundar Yumeng: 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:35:15 brinzhang Sundar, Yumeng: add the UT, that can accept before the deadline of RC1, otherwise, I would rather postpone, we cannot merge the ERROR logical
03:36:41 xinranwang s_shogo: do you want talk about program API?
03:37:03 shaohe_feng any way, str(uuid) is not bad.
03:37:21 Sundar brinzhang: of course, others need to be fixed and reviewed.
03:37:21 shaohe_feng yes, how is the process of program API?
03:38:02 Yumeng Sundar: 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:04 Sundar Yumeng, the bandit patches do not seem to form a series? Enabling bandit as voting job depends only on the SPDK patch https://review.opendev.org/#/c/720475/3
03:38:50 Yumeng Sundar: Enabling bandit as voting job depends on the series actually. the four patches
03:39:18 Yumeng https://review.opendev.org/#/c/720479/3//COMMIT_MSG
03:39:52 Yumeng but the four patches are independet.
03:40:16 Yumeng are mutually independet.
03:40:22 brinzhang Yumeng: https://review.opendev.org/#/c/720475/ this one is enough, your patches alread have a rebase logical
03:41:22 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:23 Sundar Yumeng: 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:42:01 Sundar I gave +2 with the understanding that other patches need to merge first
03:44:06 Yumeng Sundar, 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:38 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:45:21 xinranwang it is also depends on which version of OPAE we installed, right ?
03:45:23 Yumeng Sundar, brinzhang and all: agree with double check uuid to make sure it's a str, and I will double check the unittest
03:45:25 s_shogo I tried get OPAE version from "fpgainfo fme", but that format seems to have difference between some OPAE version
03:45:43 shaohe_feng we can push OPAE team to provide a unify API for program
03:45:50 brinzhang The 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 case
03:45:57 Sundar I 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 suffix
03:46:36 shaohe_feng Let's them distinguish to different bitstream format and call the right cmd
03:47:19 shaohe_feng In my former demo, I did not change any cyborg code.
03:47:20 Sundar xinranwang: 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:50 Sundar May be this can be a goal for Victoria release :)
03:48:01 shaohe_feng I just write a shell wrap to call fpgasupdate or fpgaconf.
03:48:38 shaohe_feng I can do, OPAE team can also distinguish them=D
03:48:38 Sundar shaohe_feng: In a production env, with different types of FPGAs from different vendors, we would need a better mechanism.
03:48:59 Sundar It is not just OPAE -- but hopefully other vendors too
03:50:36 shaohe_feng Yes, but that's hard. No now know all vendors product well. So different vendors maintainer their special code :')
03:51:52 Sundar The 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:52:46 shaohe_feng For image upload, it is necessary to call the vendors special tools to parser the metadata of their image
03:53:12 shaohe_feng and format glance metadata
03:53:46 shaohe_feng we are tying to do it. This can make user more easy to user cyborg
03:53:56 shaohe_feng and avoid error prone
03:54:40 Sundar shaohe_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:55:15 xinranwang It 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:56 shaohe_feng Sundar, agree. I'm make the planing to improve it.
03:56:14 Sundar xinranwang: 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:57:49 s_shogo (IMO, multiple OPAE in single env is little difficult to operate.)
03:58:58 Sundar s_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.
04:00:11 xinranwang shaohe_feng Sundar does OPAE team plan to unify a set of API for different cards?
04:00:21 s_shogo Sundar, Thanks, I missed that possibility.that's right.
04:00:50 shaohe_feng xinranwang let us feedback to them and push them -_-
04:02:03 shaohe_feng I guess no. we can user, but they are not.
04:02:07 Sundar xinranwang: I can explain that offline, because we are hitting the top of the hour.
04:02:32 shaohe_feng so they do not know the pain-point
04:03:04 Sundar Any other topic for today?
04:04:31 Sundar Tomorrow is the last day for RC release. We may not be able to merge any patches for Ussuri after that.
04:04:56 Sundar Thanks a lot to all of you. Have a good day. Bye.
04:05:01 Sundar #endmeeting
04:05:02 xinranwang Sundar: ok
04:05:03 openstack Meeting ended Thu Apr 23 04:05:00 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
04:05:04 xinranwang byebye
04:05:05 openstack Minutes: http://eavesdrop.openstack.org/meetings/openstack_cyborg/2020/openstack_cyborg.2020-04-23-03.01.html
04:05:06 openstack Minutes (text): http://eavesdrop.openstack.org/meetings/openstack_cyborg/2020/openstack_cyborg.2020-04-23-03.01.txt
04:05:07 openstack Log: http://eavesdrop.openstack.org/meetings/openstack_cyborg/2020/openstack_cyborg.2020-04-23-03.01.log.html
04:05:15 s_shogo bye
04:05:33 shaohe_feng do we still need to refactor the accelerator driver
08:20:21 openstackgerrit YumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory] https://review.opendev.org/720143
08:20:43 openstackgerrit YumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory] https://review.opendev.org/720143
08:47:07 openstackgerrit YumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory] https://review.opendev.org/720143
08:50:22 openstackgerrit YumengBao proposed openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory] https://review.opendev.org/720143
10:19:14 openstackgerrit Merged openstack/cyborg master: Fix bandit error: [B108:hardcoded_tmp_directory] https://review.opendev.org/720143
11:34:01 openstackgerrit zhurong proposed openstack/cyborg master: Fix bandit error: Ascend driver:[B602:subprocess_popen_with_shell_equals_true] https://review.opendev.org/720456
11:36:26 openstackgerrit zhurong proposed openstack/cyborg master: Fix bandit error: [B104:hardcoded_bind_all_interfaces] https://review.opendev.org/720149
11:39:36 openstackgerrit YumengBao proposed openstack/cyborg master: Fix bandit error: Ascend driver:[B602:subprocess_popen_with_shell_equals_true] https://review.opendev.org/720456
12:55:10 openstackgerrit Merged openstack/cyborg master: Fix bandit error: [B104:hardcoded_bind_all_interfaces] https://review.opendev.org/720149
#openstack-cyborg - 2020-04-24
02:02:41 openstackgerrit Merged openstack/cyborg master: Fix bandit error: SPDK driver:[B602:subprocess_popen_with_shell_equals_true] https://review.opendev.org/720475
02:35:08 openstackgerrit Merged openstack/cyborg master: Fix bandit error: Ascend driver:[B602:subprocess_popen_with_shell_equals_true] https://review.opendev.org/720456
02:35:09 openstackgerrit Merged openstack/cyborg master: Change bandit job from non-voting to voting https://review.opendev.org/720479
08:47:34 openstackgerrit OpenStack Release Bot proposed openstack/cyborg master: Update master for stable/ussuri https://review.opendev.org/722636
08:47:36 openstackgerrit OpenStack Release Bot proposed openstack/cyborg master: Add Python3 victoria unit tests https://review.opendev.org/722637
#openstack-cyborg - 2020-04-26
01:26:21 openstackgerrit Merged openstack/cyborg-tempest-plugin master: Fix a typo https://review.opendev.org/696252
01:34:05 openstackgerrit Merged openstack/cyborg-specs master: Replacing the HTTP protocal with HTTPS in cyborg-driver-proposal.rst. https://review.opendev.org/617781

Earlier   Later