| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-cyborg - 2020-04-16 | |||
| 03:18:18 | shaohe_feng | link: https://etherpad.opendev.org/p/cyborg-victoria-goals | |
| 03:18:32 | shaohe_feng | I have add this program API in the etherpad | |
| 03:18:42 | shaohe_feng | this is really needed in product env to adopt cyborg | |
| 03:18:43 | Yumeng | Sundar: from my side. I think Zuul check bandit failure can be merged before Ussuri is due. https://review.opendev.org/#/q/topic:fix-bandit-check-failures+(status:open+OR+status:merged) | |
| 03:18:56 | Sundar | Yumeng: agreed | |
| 03:19:23 | Sundar | But it is failing the bandit check :) | |
| 03:19:32 | brinzhang | Yumeng, I found the bandit are not be voted, so you should enable it in zuul task | |
| 03:19:59 | brinzhang | and that should be works fine, that we can consider merge it. | |
| 03:20:31 | brinzhang | a questions, what is bandit to do? I am know few of it | |
| 03:20:32 | Yumeng | Sundar, brinzhang: yes. After we fixed all failure, we can set that to voting. and at that time, bandit check will not fail again. | |
| 03:21:09 | brinzhang | IMO, firstly, you should enable this task in zuul task | |
| 03:21:29 | Sundar | brinzhang: Bandit is a tool to detect security flaws. https://pypi.org/project/bandit/ | |
| 03:21:32 | brinzhang | than fix the failure, that we can ensure that correctly | |
| 03:22:20 | brinzhang | Sundar: ack, thanks, will see it later | |
| 03:22:23 | Sundar | brinzhang: bandit is now a non-voting job. We need to fix the issues there and then make it voting. Otherwise, other open patches will be affected. | |
| 03:22:48 | xinranwang | https://review.opendev.org/#/c/696089/ The rafactor arq api is also important, I think. | |
| 03:22:48 | Yumeng | bandit is for code security check, which can check operations like if shell operations in cyborg code is safe | |
| 03:22:49 | chenke | Ye. I think that's the right way | |
| 03:23:07 | Sundar | So, the programming API https://review.opendev.org/698190 is not for Ussuri? | |
| 03:23:10 | brinzhang | Sundar: I mean, we should enable it as the base patch, and then fix the issue, right? | |
| 03:24:00 | shaohe_feng | Bandit will scan the python code and gen AST to check security issues | |
| 03:24:49 | s_shogo | Sundar , shaohe_feng : As I mentioned in the program patch , as reply for the Sundar's comment (https://review.opendev.org/698190) , I would like to take discussion the interface of PATCH API or not. IMO, that effects the schedule of merge. | |
| 03:25:15 | shaohe_feng | we had better let programming API in cyborg as soon as possible | |
| 03:25:34 | shaohe_feng | so we should try do our best to make it ready | |
| 03:26:04 | shaohe_feng | this is a very important feature | |
| 03:26:16 | shaohe_feng | s_shogo thank you for post this feature. | |
| 03:26:19 | Sundar | brinzhang: IMHO, it is better to have a patch series that fixes the open bandit issues and then make it a voting job. We need to merge other open patches for Ussuri. They will be affected if we make it a voting job too soon. Agreed? | |
| 03:26:24 | brinzhang | shaohe_feng：I also prefer to use update method | |
| 03:26:45 | shaohe_feng | update for firmware update? | |
| 03:27:04 | shaohe_feng | do you means a new update API? | |
| 03:27:52 | brinzhang | Sundar: we just enable the vote in zuul, but cannot merged, that cannot impact any open patches, I think | |
| 03:27:59 | Yumeng | Sundar,brinzhang : Agreed. We should fix bandit failures first, then update bandit job as voting in zuul check. otherwise, if we change voting first, other patches will get Zuul -1. | |
| 03:28:19 | shaohe_feng | let's welcome a new fresher haibin-huang | |
| 03:28:38 | brinzhang | if we just fix that bandit failures, we cannot ensure it work fine after we enabled bandit vote in zuul | |
| 03:28:40 | Yumeng | brinzhang: I will post all patches for bandit today. | |
| 03:28:44 | shaohe_feng | she will try to help verify the program API in product evn. | |
| 03:29:17 | chenke | s/she/he | |
| 03:29:28 | haibin-huang23 | I am very glad to join cyborg family. | |
| 03:29:29 | Sundar | Welcome haibin-huang . Would you like to introduce yourself? | |
| 03:29:38 | shaohe_feng | sorry, he :] | |
| 03:29:48 | Yumeng | welcome haibin-huang! :) | |
| 03:29:53 | shaohe_feng | haibin-huang you can help to review this new API | |
| 03:29:58 | brinzhang | welcome haibin-huang | |
| 03:30:03 | chenke | welcome haibin again. since beijing hackson. | |
| 03:30:17 | s_shogo | welcome haibin-huang | |
| 03:30:18 | xinranwang | welcome haibin-huang23 | |
| 03:30:26 | songwenping | welcome haibin-huang too | |
| 03:30:41 | shaohe_feng | #link https://review.opendev.org/#/c/698190/ | |
| 03:30:59 | brinzhang | Yumeng: ok, sound good, we shuold ensure the bandit run works fine, than star merge the open fix bandit patches | |
| 03:31:04 | haibin-huang23 | I am from Intel. I work on ONAP in past years. I focus on multicloud in ONAP project | |
| 03:31:22 | haibin-huang23 | thank you everyone | |
| 03:31:55 | Sundar | Good to know you, haibin-huang23 | |
| 03:32:24 | brinzhang | shaohe_feng, Sundar: the program API, if we use update instead of patch, is it ok? | |
| 03:33:51 | Sundar | brinzhang: The HTTP verbs are PUT, POST and PATCH. When you say update, which one do you mean? | |
| 03:34:22 | brinzhang | shaohe_feng, Sundar: ... PUT, soory | |
| 03:34:33 | Sundar | IMO, PATCH is the one that is closest in semantics to the programming operation. We are also using it for programming during ARQ binding. | |
| 03:35:06 | shaohe_feng | brinzhang you can read feilding's paper | |
| 03:35:42 | shaohe_feng | try to find which is more accurate | |
| 03:36:33 | brinzhang | I just think it's not very popular, if you all think that ok, I will be not concern this ^ | |
| 03:36:49 | songwenping | https://review.opendev.org/#/c/718584/ | |
| 03:36:55 | Sundar | PUT should be idempotent. PATCH need not be. The programming operation is not idempotent -- it can fail once and succeed the next time | |
| 03:37:26 | shaohe_feng | but the patch seems is newer method, I'm not sure feilding have discuss the patch in his paper. | |
| 03:38:09 | Sundar | brinzhang: I think you pointed me to some Nova oprrations. But Nova developers themselves criticized my patches when I followed their precedent. :) | |
| 03:38:20 | Sundar | *operations with PUT | |
| 03:39:01 | brinzhang | Sundar: I see that, and I have not looked into | |
| 03:39:11 | brinzhang | pls see https://review.opendev.org/#/c/718584/ | |
| 03:40:46 | brinzhang | and https://review.opendev.org/#/c/718584, that revert device and deployable when resource provider create fail | |
| 03:41:25 | Sundar | Does everybody think that the right approach is to delete/revert Cyborg objects when a Placement update fails? | |
| 03:41:30 | brinzhang | Sundar: did you review this patch? | |
| 03:41:37 | shaohe_feng | brinzhang why do you prefer PUT? | |
| 03:42:18 | songwenping | i am sorry my network is not good | |
| 03:42:45 | brinzhang | songwenping: Sundar said Does everybody think that the right approach is to delete/revert Cyborg objects when a Placement update fails? | |
| 03:43:36 | Sundar | xinranwang, Yumeng, s_shogo, shaohe_feng, all: Re. https://review.opendev.org/#/c/718584, Does everybody think that the right approach is to delete/revert Cyborg objects when a Placement update fails? | |
| 03:44:43 | xinranwang | IMO, It is reasonable. | |
| 03:45:16 | xinranwang | Otherwise, the conductor will not notice the diff and will now report to placement correctly. | |
| 03:45:20 | shaohe_feng | Let me think about it | |
| 03:45:46 | xinranwang | which will cause inconsistency | |
| 03:46:03 | Sundar | Ok. if you all decide that the best approach is to revert Cyborg db updates, then why not do Placement update first and update Cyborg db only if that succeeds? | |
| 03:46:18 | songwenping | xinranwang:right | |
| 03:47:02 | shaohe_feng | yes， there will be disaccord between placement and cyborg DB | |
| 03:47:48 | Sundar | Personally, I'd prefer to update Cyborg db always, and mark the objects that failed to sync with Placement. That way, operators will know that Cyborg did its job correctly and Placement failed. | |
| 03:48:03 | shaohe_feng | Sundar our design is do placement update first. | |
| 03:48:28 | shaohe_feng | we have discuss this issue in Dan smith's patch. | |
| 03:49:00 | brinzhang | Sundar: I saw you comment, I think your suggestion will be compex, but it is also an alternative way | |
| 03:49:20 | brinzhang | s/compex/complex | |
| 03:49:30 | shaohe_feng | do Placement update first and update Cyborg db only if that succeeds is the easiest way to keep consistent | |
| 03:49:46 | shaohe_feng | we have discuss it before. | |
| 03:50:26 | Sundar | Ok, I'll support your decision. Has it been tested fully in various scenarios, including Placement service down for some time and then comes up? | |
| 03:50:39 | shaohe_feng | bet another things: if placement DB successfully, but cyborg DB failed | |
| 03:50:51 | shaohe_feng | then what should cyborg do? | |
| 03:51:28 | xinranwang | Sundar: please see chenke's patch here: https://review.opendev.org/#/c/711912/2 | |
| 03:51:32 | songwenping | Sundar:no problem | |
| 03:51:41 | brinzhang | shaohe_feng: that will be raise > 500, I rembered chenke done this | |
| 03:52:06 | songwenping | i have test this scenario | |
| 03:52:10 | xinranwang | The commit message has explain the root reason of data imconsistency | |
| 03:53:02 | shaohe_feng | so I prefer make sure cyborg DB successfully first. | |
| 03:53:48 | Yumeng | agree we do the revert to handle the exception. and Let's add reconsidering placement report as a V release goal. whether we need to decouple placement report or not. | |
| 03:55:27 | xinranwang | If placement succeed and cyborg DB failed. The next time cyborg conductor do diff, it finds a diff, writes db and update placement with same info. I think the placment will return like "resourve provider already exists" something like this, But it will not crash the service. What do you think? | |
| 03:55:40 | shaohe_feng | sundar's comment: Personally, I'd prefer to update Cyborg db always, and mark the objects that failed to sync with Placement. That way, operators will know that Cyborg did its job correctly and Placement failed. | |
| 03:56:03 | brinzhang | this way just reduce the date inconsistence's scenario, that cannot cover all scenarios. | |