Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for expanding volumes #30

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

iamjoemccormick
Copy link
Member

Checklist before merging:

  • Make any changes needed for the operator based deployment.
  • Documentation: Has documentation been added for new functionality? Does any existing documentation need to be updated or removed?
  • Update documentation to indicate support for volume expansion, emphasizing this is essentially a no-op as far as the driver and BeeGFS are concerned.
  • Tests: Is there adequate test coverage for new functionality? Do any existing tests need to be updated?
  • Dependencies: Does this PR rely on any changes to dependencies? If so has the go.mod or equivalent been updated to point at a version that contains the related changes?
  • Git Hygiene: Is the commit history squashed down reasonably?

Which issue(s) does this PR address?

Resolves #29

Does this PR depend on any other PRs to be merged first?

No

What does this PR do / why do we need it?

While currently volume "capacity" has no meaning as far as the driver is concerned, some\ applications rely on the capacity of the PV/PVC in the K8s API to make certain decisions. For these applications it is helpful to support volume resizing.

Are there any specific topics we should discuss before merging?

  • Are there any downsides to enabling this behavior?
  • Does adding support for expanding volumes now have any implications in the future if volume capacity has meaning (i.e., the driver/BeeGFS begin enforcing the requested capacity)?

@iamjoemccormick iamjoemccormick self-assigned this Jun 9, 2024
Copy link
Collaborator

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

Looks about right to me. I was thinking the ControllerExpandVolume call should at least check whether the entirety of the BeeGFS file system was big enough to handle the requested capacity, but it looks like CreateVolume doesn't do this either. That check should be introduced in both locations simultaneously if it ever is.

@iamjoemccormick iamjoemccormick force-pushed the iamjoemccormick/support-volume-expansion branch from 4f51ff1 to 4f37cf1 Compare June 10, 2024 15:01
@iamjoemccormick
Copy link
Member Author

Looks about right to me. I was thinking the ControllerExpandVolume call should at least check whether the entirety of the BeeGFS file system was big enough to handle the requested capacity, but it looks like CreateVolume doesn't do this either. That check should be introduced in both locations simultaneously if it ever is.

Thanks for the review! I also considered a check to verify the available/total capacity was less than the requested capacity, but since there is currently never any checking around capacity I thought it better not to. Agreed if this is added it should be done universally.

By the way, after reviewing the first test failure I realized even though this is a no-op, we must still check we got a valid volume ID so I fixed that and also set CapControllerExpansion: true in the test driver setup. Last piece is to make all this work with the operator based deployment (which seems to be the reason tests are failing now). Will try and carve out some time later this week to wrap that up.

Update documentation to reflect the new functionality
@iamjoemccormick iamjoemccormick requested a review from ejweber July 7, 2024 00:10
@iamjoemccormick
Copy link
Member Author

It took a little longer than I intended to get back to this, but volume expansion now works when the driver is deployed using the operator. If you have time, would definitely welcome a second set of eyes on those changes!

Copy link
Collaborator

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

I'm in no position to test these changes, and I don't remember enough about the operator setup to know if everything is above board there, but it all looks reasonable.

Left a couple of comments, but no blockers in my opinion.

@ejweber
Copy link
Collaborator

ejweber commented Jul 15, 2024

Looks like I can't resolve the conversations, but the answers are good. Feel free to resolve them and merge when convenient.

@iamjoemccormick
Copy link
Member Author

Looks like I can't resolve the conversations, but the answers are good. Feel free to resolve them and merge when convenient.

Good to know, I went ahead and invited your as a collaborator for next time :) Thanks again!

@iamjoemccormick iamjoemccormick merged commit a53f937 into master Jul 15, 2024
20 checks passed
@iamjoemccormick iamjoemccormick deleted the iamjoemccormick/support-volume-expansion branch July 15, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for expanding volumes
2 participants