hashicorp / packer-plugin-vsphere

Packer plugin for VMware vSphere Builder
https://www.packer.io/docs/builders/vsphere
Mozilla Public License 2.0
96 stars 93 forks source link

Missleading documentation: `disk_size` is in MiB not in MB #348

Closed lindhe closed 9 months ago

lindhe commented 9 months ago

Overview of the Issue

In the docs, it says for disk_size that the value is "The size of the disk in MB":

docs

It is in fact in MiB (1 048 576 bytes) not MB (1 000 000 bytes). It may seem like a minor detail, but these things are important. Trusting the documentation to be correct, I spent hours debugging why the disk size was incorrect for me. I do grant you that MB is not 100% wrong, just ambiguous, because of the legacy of the term. But since we can use a more correct term now, I think we should.

More occurrences

I have a hard time pin-pointing the exact line in this GitHub repo, but there are a handful of lines like this one:

"The size of the disk in MB" strings in this repo

https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/builder/vsphere/common/storage_config.go#L94 https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/docs-partials/builder/vsphere/common/DiskConfig-required.mdx#L3 https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/docs-partials/builder/vsphere/clone/CloneConfig-not-required.mdx#L5 https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/.web-docs/components/builder/vsphere-iso/README.md?plain=1#L975 https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/builder/vsphere/clone/step_clone.go#L36 https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/.web-docs/components/builder/vsphere-clone/README.md?plain=1#L59 https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/.web-docs/components/builder/vsphere-clone/README.md?plain=1#L186

I do not know if all of these are incorrect, or if some of them are actually in MB. That should be investigated before changing them.

Multiple-byte units

For more information about the binary and decimal unit prefixes, please see https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

tenthirtyam commented 9 months ago

This would only need to be changed in the .go files an the documentation would be generated by packer-sdc.

tenthirtyam commented 9 months ago

Sizes are typically measured in binary (base 2), not decimal (base 10).

So when diskSize is multiplied by 1024 such as:

Here...

https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/builder/vsphere/driver/disk.go#L51-L70

And here...

https://github.com/hashicorp/packer-plugin-vsphere/blob/4c8910c81b4c14981a83a0aca6323b52cb155cda/builder/vsphere/driver/vm.go#L681-L701

... it suggests that diskSizeis in mebibytes (MiB), not megabytes (MB). (And that disk.CapacityInKB is in kibibytes (KiB), too.)

1 MiB = 2^20 bytes = 1,048,576 bytes 
1 MB = 10^6 bytes = 1,000,000 bytes

The difference between these two definitions can indeed cause confusion and bugs, so it's important to be clear about which one is being used

tenthirtyam commented 9 months ago

See https://github.com/hashicorp/packer-plugin-vsphere/pull/349.

lindhe commented 9 months ago

Wow, how quick was that!?! Thanks for the comments and the PR! I'll give the PR a second pair of eyes.