| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-cyborg - 2020-04-23 | |||
| 03:17:19 | shaohe_feng | maybe not the source code error | |
| 03:17:37 | shaohe_feng | something wrong it the functional test code. | |
| 03:17:38 | brinzhang | chenke: we also should keep the UT can works fine | |
| 03:17:40 | chenke | @brin . Did you encounter the problem when running the fun test? | |
| 03:17:54 | shaohe_feng | s/something wrong it the functional test code. | |
| 03:17:58 | Yumeng | if not uuid, it should have raised an error | |
| 03:18:03 | chenke | I guess. | |
| 03:18:29 | brinzhang | chenke: I paste my test step, you can review again | |
| 03:19:06 | chenke | ok. | |
| 03:19:31 | brinzhang | Yumeng: so, there will be negative and active scenario need to be test, if the bitstream_uuid is str, or not str | |
| 03:19:32 | Sundar | Got disconnected when I logged into my VPN | |
| 03:19:33 | s_shogo | I also check the test test of brin. | |
| 03:19:49 | s_shogo | s/test test/test step | |
| 03:22:15 | Yumeng | brinzhang: 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 object | |
| 03:22:34 | Sundar | Yumeng: In https://review.opendev.org/720475, do we want to run bash with some unknown variable as argument? That can be insecure too. | |
| 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 | |