feat(apps): add list and patch#1344
Conversation
Coverage Report for CI Build 27267062499Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 86.133%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
|
You can access the deployment of this PR at https://renku-ci-ds-1344.dev.renku.ch |
| ) -> JSONResponse: | ||
| project_id = ULID.from_str(query.project_id) if query.project_id is not None else None | ||
| apps = await self.apps_repo.list_apps(user=user, project_id=project_id) | ||
| return json([app.as_apispec().model_dump(exclude_none=True, mode="json") for app in apps]) |
There was a problem hiding this comment.
I am sorry I missed this in the first review. But you can use validate_and_dump here. This is from renku_data_services.base_models.validation. For example see how get_all for data connector does this.
| raise errors.MissingResourceError( | ||
| message=f"App with name '{app_name}' does not exist or you do not have access to it." | ||
| ) | ||
|
|
||
| launcher = await self.session_repo.get_launcher(user, runtime_state.launcher_id) | ||
|
|
||
| authorized = await self.authz.has_permission(user, ResourceType.project, launcher.project_id, Scope.WRITE) | ||
| if not authorized: | ||
| raise errors.MissingResourceError( | ||
| message=f"App with name '{app_name}' does not exist or you do not have authorization to modify it." | ||
| ) |
There was a problem hiding this comment.
nitpick: make the two error messages the same so people cannot tell how far they got and what does or does not exist in case they do not have access.
| async def set_app_deployment_resources(self, app_name: str, resource_class: ResourceClass) -> AppRuntimeState: | ||
| """Update the container resources of the app to match the given resource class.""" | ||
| cluster = await self.__client.cluster_by_id(self.__cluster_id) | ||
| meta = K8sObjectMeta( | ||
| name=app_name, | ||
| namespace=cluster.namespace, | ||
| cluster=cluster.id, | ||
| gvk=KNATIVE_SERVICE_GVK, | ||
| user_id=DUMMY_RENKU_APP_USER_ID, | ||
| ) | ||
| patch_body: list[dict[str, Any]] = [ | ||
| { | ||
| "op": "replace", | ||
| "path": "/spec/template/spec/containers/0/resources", | ||
| "value": _resources_from_resource_class(resource_class), | ||
| } | ||
| ] | ||
| updated = await self.__client.patch(meta, patch_body) | ||
| return _extract_runtime_state(KnativeService.model_validate(updated.manifest)) |
There was a problem hiding this comment.
suggestion (non-blocking): It is better to use a strategic merge patch because it will not depend on the position of the container but would match on the container name. Which is more robust. One slight setback with a strategic merge patch is that it will update only unless you set the field to None. So if you have fields that are not set in the required/final state you should not exclude them but set them to None.
We can just take this as a future improvement though. And we just add a TODO comment here or open an issue. This will only be a problem once we add more than 1 container.
There was a problem hiding this comment.
I just checked in the code and the patching in the k8s client we used to wrap kr8s is either json or merge patch. It does not have a way to make it a strategic merge patch. So lets just add a TODO comment or open an issue. Our wrapper around the kr8s library just guesses the patch type and it never guesses strategic merge patch.
| if state == apispec.AppState.hibernated: | ||
| latest = await self.k8s_client.hibernate_app_deployment(app_name) | ||
| elif state == apispec.AppState.running: | ||
| latest = await self.k8s_client.resume_app_deployment(app_name) |
There was a problem hiding this comment.
nitpick(non-blocking): You could potentially save some api calls here and patch only if the current state does not match the desired state. Doing the same for the resource class is a bit too scary and the resource class could have been updated and it has new values that should be updated.
| latest: AppRuntimeState = runtime_state | ||
| if resource_class_id is not None: | ||
| resource_class = await self.rp_repo.get_resource_class(user, resource_class_id) | ||
| latest = await self.k8s_client.set_app_deployment_resources(app_name, resource_class) |
There was a problem hiding this comment.
When you are going from hibernated to running you should pull the resource class and update it. Even if the resource class id is the same the definition of the resource class may have changed and in this case we should also update the resources.
4 participants
/deploy renku=add-apps-beta extra-values=apps.enabled=true