Skip to content

Conversation

@lexxxel
Copy link
Contributor

@lexxxel lexxxel commented Nov 5, 2025

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated acceptance tests in /fwprovider/tests for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.

This PR is an addition to #2319. So please do merge #2319 before reviewing this PR!

Proof of Work

image image image image image image

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #1435
After some playing around with mount points, I also documented and fixed #2322

Signed-off-by: Alexander Petermann <alex@petermann.dev>
Signed-off-by: Alexander Petermann <alex@petermann.dev>
Signed-off-by: Alexander Petermann <alex@petermann.dev>
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @lexxxel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the proxmox_lxc_container Terraform resource by introducing the ability to resize LXC container root disks and update several root filesystem properties in-place. This change improves the flexibility and operational efficiency for managing LXC containers, as users can now modify disk size and other rootfs settings without the need for resource recreation, with reboots triggered only when essential for certain property changes.

Highlights

  • LXC Root Disk Resizing: Introduced the capability to resize LXC container root disks dynamically, allowing for in-place expansion of storage without requiring container recreation.
  • In-place RootFS Property Updates: Enabled in-place updates for various root filesystem properties including ACL, quota, replication, and mount options. Changes to these properties will now trigger a container reboot if necessary, rather than forcing recreation.
  • API Client Enhancement: Added a new ResizeContainerDisk method to the Proxmox API client, facilitating direct communication with the Proxmox API for disk resizing operations.
  • Terraform Schema Updates: Modified the ForceNew attribute for disks and rootfs.size within the Terraform proxmox_lxc_container resource schema from true to false, allowing these attributes to be updated without destroying and recreating the container.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a valuable feature for resizing LXC container root disks. The overall approach is sound, but I've identified a few critical issues in the resource update logic that could lead to runtime panics or silent failures. My review comments focus on adding necessary error handling, ensuring safe access to pointer fields to prevent panics, and correcting flawed comparison logic. I've also included a suggestion to improve consistency in one of the new data structures. Addressing these points will significantly improve the robustness of the implementation.

@lexxxel lexxxel changed the title Feature/lxc root disk resize feature(lxc): enable root disk resize without container recreation Nov 5, 2025
@lexxxel lexxxel changed the title feature(lxc): enable root disk resize without container recreation feature(lxc): enable root disk resize without forced new container Nov 5, 2025
@lexxxel
Copy link
Contributor Author

lexxxel commented Nov 5, 2025

I implemented the resize path that increases the root disk without a forced replacement. However, in case the new disk should be smaller than the current disk, I do 'just' return an error. It would make sense to trigger a forceNew, but I'm not so familiar with this code base and I don't see where I can do the check (smaller -> recreate; larger -> update). Any suggestions would be appreciated.

Signed-off-by: Alexander Petermann <alex@petermann.dev>
@lexxxel lexxxel marked this pull request as draft November 5, 2025 22:52
@lexxxel
Copy link
Contributor Author

lexxxel commented Nov 5, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a valuable feature to allow resizing the root disk of an LXC container without forcing a recreation of the resource. The implementation is largely on the right track, but I've identified a few critical issues that need to be addressed. These include an unhandled error which could lead to an inconsistent state, potential nil pointer dereferences that could cause the provider to panic, and a logic bug in change detection for the quota property. I've provided specific code suggestions to resolve these problems. Additionally, I've included a minor suggestion to improve an error message for better clarity.

@lexxxel
Copy link
Contributor Author

lexxxel commented Nov 5, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a valuable feature to allow resizing the root disk of an LXC container without forcing a recreation of the resource. The overall approach is sound, but I've identified a couple of critical issues in the update logic that could lead to panics or silent error swallowing. Additionally, I've provided a suggestion to improve the resource lifecycle by handling disk shrinking at the plan stage, as hinted by a TODO in the code.

Signed-off-by: Alexander Petermann <alex@petermann.dev>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 6, 2025
@lexxxel
Copy link
Contributor Author

lexxxel commented Nov 6, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enables resizing the root disk for LXC containers without forcing a recreation of the container. The changes involve removing ForceNew from the disk schema, adding a custom diff function to handle disk shrinking by forcing a new resource, and updating the container update logic to call the new resize API endpoint. The implementation is mostly correct, but I've found a few issues, including a bug that would cause unnecessary API calls, some leftover debugging code, and a potential Go version compatibility issue. My review comments provide suggestions to address these points.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Alexander Petermann <alex@petermann.dev>
@lexxxel
Copy link
Contributor Author

lexxxel commented Nov 6, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the ability to resize an LXC container's root disk without forcing a new container resource. This is achieved by removing ForceNew from the disk schema and adding update logic to handle resizing. The changes also include a custom diff function to prevent disk shrinking by forcing a new resource in that case.

My review includes two main points:

  1. A high-severity bug in the update logic where disk sizes are compared by pointer instead of by value, which would lead to unnecessary API calls.
  2. A medium-severity suggestion to clean up the custom diff function by removing debug code and a confusing ForceNew call.

Overall, the changes are well-structured and address the intended feature. After addressing the identified issues, the PR should be in good shape.

…er a recreate

Signed-off-by: Alexander Petermann <alex@petermann.dev>
@lexxxel lexxxel force-pushed the feature/lxc_rootDisk_resize branch from 49d343d to 79ddf1a Compare November 6, 2025 12:12
@lexxxel
Copy link
Contributor Author

lexxxel commented Nov 6, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enables resizing the root disk of an LXC container without forcing a new resource, which is a great enhancement. The changes involve removing ForceNew from the disk size attribute, adding a CustomizeDiff to handle disk shrinking, and implementing the resize logic in the containerUpdate function. The approach is sound, but I've identified two critical issues. One is the use of Go 1.22-specific syntax, which could cause compilation failures on older Go versions. The other is a potential nil pointer dereference in the update logic when checking the old disk size. Please address these issues to ensure the stability and compatibility of this new feature.


oldSize := containerConfig.RootFS.Size
size := types.DiskSizeFromGigabytes(int64(diskBlock[mkDiskSize].(int)))
if *oldSize > *size {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There is a potential nil pointer dereference here. oldSize is of type *types.DiskSize and can be nil if the size property is not returned by the Proxmox API for the container's rootfs. Dereferencing it with *oldSize on this line will cause a panic. You should add a nil check for oldSize before dereferencing it.

Suggested change
if *oldSize > *size {
if oldSize != nil && *oldSize > *size {

@lexxxel lexxxel marked this pull request as ready for review November 6, 2025 12:27
@lexxxel lexxxel changed the title feature(lxc): enable root disk resize without forced new container feature(lxc): enable root disk resize without recreate. recreate container if mount points are added, deleted or changed in size Nov 6, 2025
Signed-off-by: Alexander Petermann <alex@petermann.dev>
@lexxxel lexxxel marked this pull request as draft November 21, 2025 23:51
@bpg
Copy link
Owner

bpg commented Nov 22, 2025

Okay, we'd need to rebase this, after the previous PR got merged.

@lexxxel
Copy link
Contributor Author

lexxxel commented Nov 22, 2025

Yes and I will clean it up too. Will take me untill Tuesday unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants