Rename dontUsePreAllocatedDataVolumes to disablePreAllocatedDataVolumes #70
Conversation
gardener-robot
added
area/storage
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot-ci-2
added
needs/ok-to-test
pkg/controller/worker/machines.go
Outdated
| @@ -207,7 +207,7 @@ func (w *workerDelegate) allocateDataVolumes(ctx context.Context) error { | |||
| } | |||
|
|
|||
| for _, machineClass := range w.machineClasses { | |||
| if !machineClass["dontUsePreAllocatedDataVolumes"].(bool) { | |||
| if !machineClass["useDynamicallyAllocatedDataVolumes"].(bool) { | |||
There was a problem hiding this comment.
in this case you should remove negation in this condition, should be
if machineClass["useDynamicallyAllocatedDataVolumes"].(bool) {
There was a problem hiding this comment.
No, I shouldn't. The semantic of the option is the same, only the name has changed.
There was a problem hiding this comment.
@stoyanr but this means(from a semantic point of view) that the default is actually using the dynamic data volume allocation(dv created on vm creation).
There was a problem hiding this comment.
so in that case if somebody will set useDynamicallyAllocatedDataVolumes = true they won't use it as there is negation. Semantic is different.
docs/usage-as-end-user.md
Outdated
| # Don't use pre-allocated data volumes. Defaults to 'false'. | ||
| dontUsePreallocatedDataVolumes: true | ||
| # Use dynamically allocated data volumes instead of pre-allocated data volumes. Defaults to 'false'. | ||
| useDynamicallyAllocatedDataVolumes: true |
There was a problem hiding this comment.
in this case shouldn't be default = true? You removed negation (word "don't"). What should be the default value? By default I think we want to have dynamically allocated data volumes.
There was a problem hiding this comment.
See above, useDynamicallyAllocatedDataVolumes is the same as dontUsePreAllocatedDataVolumes, i.e. the 2 options are:
- pre-allocated data volumes (default)
- dynamically allocated data volumes
There was a problem hiding this comment.
my apologies, you're right I didn't read this correctly, see my comment #65 (comment)
docs/usage-as-end-user.md
Outdated
| ``` | ||
|
|
||
| Currently, these KubeVirt-specific options may include: | ||
|
|
||
| * The DNS policy and DNS configuration for the KubeVirt VMs used as shoot cluster nodes. For more information, see [DNS for Services and Pods](https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/). | ||
| * Whether to use *pre-allocated data volumes* with KubeVirt VMs. With pre-allocated data volumes (the default), a data volume is created in advance for each machine class / worker pool, the OS image is imported into this volume only once, and actual KubeVirt VM data volumes are cloned from this data volume. Typically, this significantly speeds up the data volume creation process. You can disable this feature by setting the `dontUsePreallocatedDataVolumes` option to `false`. | ||
| * Whether to use *pre-allocated data volumes* with KubeVirt VMs. With pre-allocated data volumes (the default), a data volume is created in advance for each machine class / worker pool, the OS image is imported into this volume only once, and actual KubeVirt VM data volumes are cloned from this data volume. Typically, this significantly speeds up the data volume creation process. You can disable this feature by setting the `useDynamicallyAllocatedDataVolumes` option to `true`. |
There was a problem hiding this comment.
You can disable this feature by setting the
useDynamicallyAllocatedDataVolumesoption totrue.
Should be
You can disable this feature by setting the `useDynamicallyAllocatedDataVolumes` option to `false`.
There was a problem hiding this comment.
See above, no, to disable the pre-allocated data volumes feature (which is enabled by default), useDynamicallyAllocatedDataVolumes should be set to true, not false.
stoyanr
force-pushed
the
rename-dont-use-preallocated
branch
from
79ec65e to
bcebf08
Compare
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
stoyanr
changed the title
|
@mfranczy Renamed as suggested in the issue comment. |
gardener-robot
added
priority/3
How to categorize this PR?
/area storage
/kind technical-debt
/priority normal
/platform kubevirt
What this PR does / why we need it:
Renames
dontUsePreAllocatedDataVolumestouseDynamicallyAllocatedDataVolumesto avoid using "don't" and double negation.Which issue(s) this PR fixes:
Fixes #65
Special notes for your reviewer:
Release note: