Make postgres use a PVC in the kessel-relations template#831
Make postgres use a PVC in the kessel-relations template#831Rajagopalan-Ranganathan merged 1 commit intoproject-kessel:mainfrom
Conversation
|
Can one of the admins verify this patch? |
Reviewer's GuideConfigures the kessel-relations Postgres deployment to use a named PersistentVolumeClaim for its data directory and defines the PVC resource in the same manifest. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
a993880
into
project-kessel:main
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The volume is mounted at
/temp/data, which is unlikely to be where the Postgres container actually stores its data (commonly/var/lib/postgresql/dataorPGDATA); consider aligning the mountPath with the image’s data directory so the PVC actually persists the database. - You’re using a hard-coded
1Girequest for thepostgres-dataPVC; consider parameterizing the storage size (e.g., via an environment variable or template variable) so different environments can tune storage requirements without editing the manifest. - If this PVC name (
postgres-data) is intended to be shared between multiple Postgres deployments, double-check that this won’t causeReadWriteOnceconflicts across pods/nodes; otherwise, consider using distinct PVC names per deployment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The volume is mounted at `/temp/data`, which is unlikely to be where the Postgres container actually stores its data (commonly `/var/lib/postgresql/data` or `PGDATA`); consider aligning the mountPath with the image’s data directory so the PVC actually persists the database.
- You’re using a hard-coded `1Gi` request for the `postgres-data` PVC; consider parameterizing the storage size (e.g., via an environment variable or template variable) so different environments can tune storage requirements without editing the manifest.
- If this PVC name (`postgres-data`) is intended to be shared between multiple Postgres deployments, double-check that this won’t cause `ReadWriteOnce` conflicts across pods/nodes; otherwise, consider using distinct PVC names per deployment.
## Individual Comments
### Comment 1
<location path="deploy/kessel-relations.yaml" line_range="103-101" />
<code_context>
+ volumeMounts:
+ - name: postgres-data
+ mountPath: /temp/data
+ volumes:
+ - name: postgres-data
+ persistentVolumeClaim:
+ claimName: postgres-data
</code_context>
<issue_to_address>
**issue (bug_risk):** The indentation of the `- name: postgres-data` line makes the YAML invalid.
Because `- name: postgres-data` is aligned with `volumes:`, it’s not actually in the `volumes` list and will cause a YAML parse error. Indent it under `volumes:` (e.g., two spaces further) so it becomes a proper list item.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| cpu: ${SPICEDB_POSTGRES_CPU_REQUEST} | ||
| memory: ${SPICEDB_POSTGRES_MEMORY_REQUEST} | ||
| volumeMounts: | ||
| - name: postgres-data |
There was a problem hiding this comment.
issue (bug_risk): The indentation of the - name: postgres-data line makes the YAML invalid.
Because - name: postgres-data is aligned with volumes:, it’s not actually in the volumes list and will cause a YAML parse error. Indent it under volumes: (e.g., two spaces further) so it becomes a proper list item.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
volumesblock appears to be indented as a child of the container rather than as a sibling ofcontainersunder the pod spec, which will result in an invalid pod spec; it should be moved up one level so it sits alongsidecontainers. - The
PersistentVolumeClaimname and requested storage size are currently hard-coded (postgres-data,1Gi); consider parameterizing these so different deployments or environments can tune or avoid name collisions without editing the template.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `volumes` block appears to be indented as a child of the container rather than as a sibling of `containers` under the pod spec, which will result in an invalid pod spec; it should be moved up one level so it sits alongside `containers`.
- The `PersistentVolumeClaim` name and requested storage size are currently hard-coded (`postgres-data`, `1Gi`); consider parameterizing these so different deployments or environments can tune or avoid name collisions without editing the template.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Similar to #830 -- but in the correct file
Summary by Sourcery
Deployment: