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 persistent volume size option to config #3820

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

praveenkumar
Copy link
Member

This pr enable user to specify the size which is used by persistent volume in case of microshift preset. As of now we just extend the disk size to root mounted partition and free space (which used for pvc) is only 15GiB. Some users want to have more free space to consume it for pvc as per the workload and with this option now it is possible.

$ crc config set disk-size 50
$ crc config set persistent-volume-size 20
$ crc start
[...]
<crc_vm> $ sudo vgdisplay
  --- Volume group ---
  VG Name               rhel
  System ID
  Format                lvm2
  Metadata Areas        1
  Metadata Sequence No  5
  VG Access             read/write
  VG Status             resizable
  MAX LV                0
  Cur LV                1
  Open LV               1
  Max PV                0
  Cur PV                1
  Act PV                1
  VG Size               <49.02 GiB
  PE Size               4.00 MiB
  Total PE              12549
  Alloc PE / Size       7429 / <29.02 GiB
  Free  PE / Size       5120 / 20.00 GiB
  VG UUID               O7d5v9-F8HE-E0Dj-e9sE-1qgv-F74d-CZz34d

Fixes: Issue #3747

@jsliacan
Copy link
Contributor

jsliacan commented Sep 5, 2023

So PVC size comes out of the disk size, right? I.e. 0GiB <= pvc size <= disk size - 15GiB? I'm assuming that the last inequality is actually strict to leave space for other things. What would be the maximum possible pvc size for a fixed disk size?

// vgFree space as part of the bundle is default to ~15.02G (16127098880 byte)
vgFree := 16127098880
// 1GB = 1073741824 bytes
vgFree := persistentVolumeSize * 1073741824
Copy link
Contributor

@gbraad gbraad Sep 18, 2023

Choose a reason for hiding this comment

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

GB := 107374... // bytes
vgFree := persistentVolumeSize * GB

which makes things s stand out more clearly than the comment

@@ -91,6 +92,8 @@ func RegisterSettings(cfg *Config) {
"Enable experimental features (true/false, default: false)")
cfg.AddSetting(EmergencyLogin, false, ValidateBool, SuccessfullyApplied,
"Enable emergency login for 'core' user. Password is randomly generated. (true/false, default: false)")
cfg.AddSetting(PersistentVolumeSize, constants.DefaultPersistentVolumeSize, validatePersistentDiskSize, SuccessfullyApplied,
fmt.Sprintf("Total size in GiB of the persistent volume (must be greater than or equal to '%d')", constants.DefaultPersistentVolumeSize))
Copy link
Contributor

Choose a reason for hiding this comment

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

used for what?
it misses explanation as containers arent stored here

Copy link
Member Author

Choose a reason for hiding this comment

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

I am going to use https://github.com/openshift/microshift/blob/main/docs/contributor/rhel4edge_iso.md#disk-partitioning same explanation Unallocated disk space of 9GB size remains in the rhel volume group to be used by the CSI driver.

@gbraad
Copy link
Contributor

gbraad commented Sep 18, 2023

@jsliacan I had the same question just now pop up. also, what is the usage as that isnt explained to the user. ephemeral and persistent have clear usecases,...

@praveenkumar
Copy link
Member Author

So PVC size comes out of the disk size, right? I.e. 15GiB <= pvc size <= disk size? I'm assuming that the last inequality is actually strict to leave space for other things. What would be the maximum possible pvc size for a fixed disk size?

@jsliacan As of now maximum pvc size user can allocate <Total Disk Size> - 15G so if user put 40GB as disk size then he can use ~ 25G for pvc.

@praveenkumar praveenkumar self-assigned this Sep 26, 2023
This pr enable user to specify the size which is used by persistent
volume in case of microshift preset. As of now we just extend the disk
size to root mounted partition and free space (which used for pvc) is
only 15GiB. Some users want to have more free space to consume it for
pvc as per the workload and with this option now it is possible.

```
$ crc config set disk-size 50
$ crc config set persistent-volume-size 20
$ crc start
[...]
<crc_vm> $ sudo vgdisplay
  --- Volume group ---
  VG Name               rhel
  System ID
  Format                lvm2
  Metadata Areas        1
  Metadata Sequence No  5
  VG Access             read/write
  VG Status             resizable
  MAX LV                0
  Cur LV                1
  Open LV               1
  Max PV                0
  Cur PV                1
  Act PV                1
  VG Size               <49.02 GiB
  PE Size               4.00 MiB
  Total PE              12549
  Alloc PE / Size       7429 / <29.02 GiB
  Free  PE / Size       5120 / 20.00 GiB
  VG UUID               O7d5v9-F8HE-E0Dj-e9sE-1qgv-F74d-CZz34d
```
Copy link
Member

@anjannath anjannath left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -91,6 +92,8 @@ func RegisterSettings(cfg *Config) {
"Enable experimental features (true/false, default: false)")
cfg.AddSetting(EmergencyLogin, false, ValidateBool, SuccessfullyApplied,
"Enable emergency login for 'core' user. Password is randomly generated. (true/false, default: false)")
cfg.AddSetting(PersistentVolumeSize, constants.DefaultPersistentVolumeSize, validatePersistentVolumeSize, SuccessfullyApplied,
fmt.Sprintf("Total size in GiB of the persistent volume used by the CSI driver for %s preset (must be greater than or equal to '%d')", preset.Microshift, constants.DefaultPersistentVolumeSize))
Copy link
Member

Choose a reason for hiding this comment

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

ahh, sorry i forgot to mention this earlier, maybe we should use preset.Microshift.ForDisplay() here

Copy link
Member Author

Choose a reason for hiding this comment

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

@anjannath since preset have string method %s should able to get preset and Display is for end user display I think here they should know the preset value instead of Display?

@openshift-ci
Copy link

openshift-ci bot commented Sep 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anjannath

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link

openshift-ci bot commented Sep 27, 2023

@praveenkumar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-crc 306fbb6 link true /test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@praveenkumar praveenkumar merged commit d745575 into crc-org:main Sep 28, 2023
13 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants