Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an idempotent instance provisioning API keyed by engine_app_name, backed by new persistence models to track provisioning results and coordinate concurrent requests.
Changes:
- Introduces
InstanceRecord+OperationLockmodels (and migration) to support idempotent provisioning and simple DB-backed locking. - Adds
idempotent_provisionendpoint and related URL routing; updates async delete to markInstanceRecordasDELETING. - Adds test coverage for idempotent provisioning behavior (create/reuse/conflict/concurrency simulation) and bumps package version + changelog.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sdks/paas-service/paas_service/views.py | Implements idempotent_provision and updates async delete to mark records deleting |
| sdks/paas-service/paas_service/models.py | Adds InstanceRecord and OperationLock with DB-backed lock acquisition/release |
| sdks/paas-service/paas_service/migrations/0010_operationlock_instancerecord.py | Creates new tables for instance records and operation locks |
| sdks/paas-service/paas_service/constants.py | Adds InstanceRecordStatus and lock timing/polling constants |
| sdks/paas-service/paas_service/urls.py | Routes POST on instances collection to idempotent_provision; adds confirm_bound route |
| sdks/paas-service/tests/test_apis.py | Adds tests covering idempotent provision, reuse, lock conflict, and concurrency behavior |
| sdks/paas-service/paas_service/base_vendor.py | Adds return type annotation for get_provider_cls |
| sdks/paas-service/pyproject.toml | Bumps version to 2.0.4 |
| sdks/paas-service/paas_service/init.py | Bumps __version__ to 2.0.4 |
| sdks/paas-service/CHANGES.md | Documents 2.0.4 changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @classmethod | ||
| def acquire_lock(cls, lock_key: str, timeout_seconds: int | None = None) -> bool: | ||
| """尝试获取锁 |
There was a problem hiding this comment.
OperationLock.acquire_lock uses the int | None union syntax in the type annotation, which is a Python 3.10+ feature. This project declares requires-python = ">=3.8,<3.12", so this will be a syntax error under Python 3.8/3.9. Please switch to Optional[int] (or int with a sentinel) or bump the minimum supported Python version accordingly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e24d2da7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,4 +1,8 @@ | |||
| # 版本历史 | |||
|
|
|||
| ## 2.0.4 | |||
There was a problem hiding this comment.
可以把功能放在库的 README 里面,涵盖新接口的主要使用方式、覆盖场景,等等。
| return self._prepared_fields_data | ||
|
|
||
|
|
||
| class InstanceRecord(UuidAuditedModel): |
There was a problem hiding this comment.
关键模型,需要在 docstring 说明模块功能,假设不知道这个概念的读者想要了解什么信息,补充这些信息。
|
|
||
|
|
||
| class InstanceRecord(UuidAuditedModel): | ||
| engine_app_name = models.CharField(verbose_name="引擎应用名称", max_length=64) |
There was a problem hiding this comment.
这个字段是否不使用明确的 engine_app_name,而是使用抽象的比如“唯一标识符”之类的更好?只是说目前 client 方选择使用 engine_app_name 作为这个标识符。
另外之前是否讨论过用 engine_app_name 存在取消-再绑定,无法分配新实例问题?是否应该增加其他要素在这个标识符里?
There was a problem hiding this comment.
- 可以,改为
unique_key。其实想过这个问题,但看了下这个 provision 接口的实际场景, engine_app_name 是必传的,所以直接写死了就用engine_app_name作为幂等键 - 解决了之前说的 异步删除场景下,无法分配新实例的问题。在模型的
status字段里,会标记该单据对应分配的实例的生命状态
| tenant_id = tenant_id_field_factory() | ||
|
|
||
|
|
||
| class OperationLock(models.Model): |
There was a problem hiding this comment.
同前,增加文档。Operation 是一个很泛用的词,需要知道这个 Operatiton 是什么东西,比如是一个通用的,还是专门针对实例分配场景设计的。
这两个增加的模型名字我感觉都可以再斟酌下:
InstanceRecord——可以被理解成实例的任何记录比如使用记录、删除记录
OperationLock——可以被理解成任何“操作”的锁,比如删除实例
单独看这两个名字,实际上是无法和他们所承担的业务功能联系起来的。
|
|
||
|
|
||
| class OperationLock(models.Model): | ||
| id = models.AutoField(primary_key=True) |
| "params": { | ||
| "engine_app_name": "bkapp-myapp-stag", # 必填,作为幂等键 | ||
| } | ||
| } |
There was a problem hiding this comment.
文档可以再详细一些,比如之前的 provison 是需要传入 UUID 的,但是新的接口不需要,那么其中的变化和区别是什么?什么场景用什么?这是使用者所最关系的。
| assert instance_with_credentials.to_be_deleted is True | ||
| assert record.status == InstanceRecordStatus.DELETING | ||
|
|
||
| def test_idempotent_provision_does_not_reuse_deleted_instance( |
There was a problem hiding this comment.
整个测试文件,更建议把核心逻辑抽离到 Views 层之外,详尽的测试主要针对抽离出的这层完成,View/API 层简单测一下就好。
一般的 MVC 架构,视图层应该尽量薄,因为需要考虑业务核心逻辑会在 View 之外的地方被使用(比如未来可能支持后台命令出发幂等 provision?)
| service_instance.prerender(request) | ||
| return Response(serializers.ServiceInstanceSLZ(service_instance).data, status=201) | ||
|
|
||
| def _get_existing_instance_response(self, request, engine_app_name, plan_id): |
There was a problem hiding this comment.
考虑把这些逻辑抽离在 views 之外,比如放在 idem_prov.py 中
| return resp | ||
|
|
||
| lock_key = engine_app_name | ||
| lock_acquired = False |
| return resp | ||
|
|
||
| if not lock_acquired: | ||
| return Response( |
There was a problem hiding this comment.
整个 idem provison 接口的设计是否应该调整一下?目前我看来,一次调用仍然是会等待很长时间才会返回,为了跟更符合异步 Provison 流程,API 设计是否应该把 InstanceRecord 的状态暴露出现,拆分“创建单”、“查询状态”、“获取结果”等 API,调用方通过多次短调用来完成整套流程。
比如:
- 启动 provision:返回单据 ID
- 根据 ID 获取单据结果:返回状态,调用方可根据状态持续轮询以获取实例信息
如果是这种短链接设计,Service 服务端甚至不需要锁等待,在“启动 provision”阶段,是有 unique 索引所保护的,假如遇到索引冲突,直接返回已创建的单据 ID 即可。
There was a problem hiding this comment.
确实这样更合理。
但是这样改的话,因为接口使用方法变了,paas-apiserver 和其它用到这个接口的地方都需要主动适配分配逻辑(多次请求)
paas-apiserver 侧明面上需要改的地方只有 servicehub.remote.provision (远程增强服务分配实例)这一个地方,但担心会有一些暗面的影响
增加表
InstanceRecord,OperationLock来实现幂等创建, 幂等键为engine_app_name。