refactor: decouple the dependency on ProcessSpec for cnative app #1726
Conversation
| @@ -46,6 +46,7 @@ class ResourceQuota: | |||
| # 资源配额方案到资源请求的映射表 | |||
| # CPU REQUEST = 200m | |||
| # MEMORY REQUEST 的计算规则: 当 Limits 大于等于 2048 Mi 时，值为 Limits 的 1/2; 当 Limits 小于 2048 Mi 时，值为 Limits 的 1/4 | |||
| # 云原生应用实际的 requests 配置策略在 operator 中实现, 这里的值并非实际生效值 | |||
| PLAN_TO_REQUEST_QUOTA_MAP = { | |||
There was a problem hiding this comment.
所以这里仅仅只是作为展示用，没有和资源实际生效的连起来是吧。
记录一个单据，需要实现一下统一管理和配置应用的资源？
| @@ -208,6 +211,41 @@ def _find_condition(self, type_: MResConditionType) -> Optional[MetaV1Condition] | |||
| return None | |||
|
|
|||
|
|
|||
| class BkAppResourceByEnvLister: | |||
There was a problem hiding this comment.
这个类型抽象感觉不是特别直接和干脆，一下看不出来具体的功能。结合这个模块下的其他函数，是不是直接叫 list_mres_by_env(app: Application, environment: str) 更直接一点；cluster_name / namespace 感觉也是放在函数内直接处理就好。
| def namespace(self): | ||
| """应用所在命名空间 | ||
|
|
||
| Note: 云原生应用仅支持部署在单集群下, 否则 paas_wl.bk_app.processes.watch.ProcInstByEnvListWatcher 无法正常 list/watch |
There was a problem hiding this comment.
这个函数提及和 ProcInstByEnvListWatcher 之间的限制，感觉不是特别好，函数本身的适用性变差了；感觉获取资源列表的时候，完全可以处理所有的 cluster 和 namespace，兼容多值的情况；返回结果的时候，可以把“多集群/多namespace”作为结果的一部分暴露出来，ProcInstByEnvListWatcher 应该报错就自己去报错。
| if ProcSpecUpdater(self.env, proc_type).spec_object.autoscaling: | ||
| self.scale_auto(proc_type, False) | ||
| try: | ||
| module_process_spec = ModuleProcessSpec.objects.get(module=self.env.module, name=proc_type) |
There was a problem hiding this comment.
Nit: 这个查询和 try/except NotFound多次出现，可以考虑做一个小函数。
| from paasng.platform.engine.models.deployment import ProcessTmpl | ||
|
|
||
|
|
||
| def _list_proc_specs(env: ModuleEnvironment) -> List[Dict]: | ||
| """Return all processes specs of an app | ||
| @define |
There was a problem hiding this comment.
之前是不是讨论过 shim 模块的定位模糊，这些代码是不是不要放在 shim 里面？
| def _list_proc_specs(env: ModuleEnvironment) -> List[Dict]: | ||
| """Return all processes specs of an app | ||
| @define | ||
| class _CNativeProcessSpec: |
There was a problem hiding this comment.
名字要不还是别以下划线开头？名字里要不要加个 “live” 之类词，表明是从线上获取。
| return module_processes_specs | ||
|
|
||
|
|
||
| def _gen_cnative_process_specs(res: BkAppResource, environment: str) -> list[_CNativeProcessSpec]: |
There was a problem hiding this comment.
在 procs.replicas 里面有 BkAppProcScaler/ReplicasReader/AutoscalingReader 几个类，其中的部分逻辑和这个函数是重复的。建议参考已有的几个 Reader 实现这个函数？（代码也可以放到那边去重新组织，主目的是“从 AppResource” 里还原环境有关的进程信息）
| judge_operation_frequent(wl_app, process_type, self._operation_interval) | ||
| except ProcessOperationTooOften as e: | ||
| raise error_codes.PROCESS_OPERATION_TOO_OFTEN.f(str(e), replace=True) | ||
| # TODO 由于云原生应用去除了对 ProcessSpec model 的依赖, 因此暂时不做操作频率限制, 待后续评估后再添加? |
There was a problem hiding this comment.
可以录个单，judge_operation_frequent 修改为基于 redis 实现
| procs = self.deployment.get_processes() | ||
| proc_mgr = ProcessManager(self.engine_app.env) | ||
| proc_mgr.sync_processes_specs(procs) | ||
| proc_mgr.sync_processes_probes(procs) |
There was a problem hiding this comment.
这里不能完全去除，比如 sync_processes_probes 仍然需要（刚好在改这块）
There was a problem hiding this comment.
这里是处理的云原生应用，云原生应用的探针都在 ModuleProcessSpec.probes 定义, 如果有的话，应该想办法统一到 ModuleProcessSpec.probes 中。sync_processes_probes 主要是同步到 paas_wl.bk_app.processes.models.ProcessProbe 中，里面的探针仅用于普通应用
| assert specs[1].resource_limit == {"cpu": "4000m", "memory": "1024Mi"} | ||
| assert specs[1].resource_limit_quota == {"cpu": 4000, "memory": 1024} | ||
| assert specs[1].resource_requests == {"cpu": "200m", "memory": "256Mi"} | ||
| assert specs[1].target_status == "stop" |
There was a problem hiding this comment.
Nit：assert 有点多，可以考虑拆成几个 test 函数，各自验证各自的字段，比如 quota/replicas_and_scaling。
解耦云原生应用对 paas_wl 侧 ProcessSpec 等模型的依赖，改为直接从线上解析 Spec