Earlier  
Later
New Search
Posted Nick Remark
#openstack-cyborg - 2020-04-16
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.
03:56:39 chenke actually. no one can cover all scenriaos.
03:56:43 s_shogo I also discussed the Placement-Cyborg interaction in enable/disable API , with xinranwang.
03:57:03 shaohe_feng we have issues(same to sudar's comments) this in Dan's patch, let us check, why not we give this?
03:57:08 shaohe_feng let me check it.
03:57:51 shaohe_feng another things:
03:58:31 xinranwang chenke's patch has fix several secenarios already. But as we said, it is difficult to cover all sceanrios.
03:58:32 shaohe_feng we need More accelerators drivers support
03:59:03 xinranwang I think we can revert like this patch does, and discuss the placement and db update decoupling in V.
03:59:20 chenke agree.
03:59:21 brinzhang if we add a flag to mark the data, I think we also need to add a period task to check this flag, and then do resource consistency
03:59:49 Sundar Our philosophy should be that Cyborg db is the source of knowledge about accelerators in any OpenStack cloud. If we let Placement take precedence, that reduces the value of Cyborg. That's just my opinion. As brinzhang said, it may be complex to make those changes.
04:00:10 Sundar brinzhang: we already have a periodc task from the agent

Earlier   Later