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 changing root disk size #345

Closed
wants to merge 4 commits into from

Conversation

cheald
Copy link

@cheald cheald commented Oct 3, 2019

This depends on vmware/go-vcloud-director#251

This simply adds the ability to specify a root_disk parameter when defining a VM, which indicates the disk's size in megabytes. For example:

// vm.tf.json
{
  "resource": {
    "vcd_vapp_vm": {
      "az-app1": {
        "name": "az-app1",
        "root_disk": 256000,
        // ...
      }
    }
  }
}

@ghost ghost added the size/XS label Oct 3, 2019
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

Here are a few preliminary comments.

Please modify go.mod with a replace directive (that will be later removed) to allow this PR to pick up your code in go-vcloud-director.
See go.mod in another PR as an example.
Once you've done that, please monitor the Travis task associated to this PR and address any issue that is making the check fail.

Regarding the PR itself, the inclusion of a new field should be reflected in creation, read, and update.

Furthermore, there should be either a new test or an expanded existing test that uses the new field.

@ghost ghost added size/S and removed size/XS labels Oct 4, 2019
@cheald
Copy link
Author

cheald commented Oct 4, 2019

I've added the replace clause in go.mod, and it's working as expected locally, but I don't see Travis performing a go get or similar; do I need to go mod vendor and commit the vendored changes as well? I don't want to clutter up the diff with dependency changes unless that's desired.

@dataclouder
Copy link
Contributor

I've added the replace clause in go.mod, and it's working as expected locally, but I don't see Travis performing a go get or similar; do I need to go mod vendor and commit the vendored changes as well? I don't want to clutter up the diff with dependency changes unless that's desired.

You need to run go mod tidy and go mod vendor. Once your change in go-vcloud-director is merged, you will remove the replace, run the above commands again, and all should be well.

@ghost ghost added size/L dependencies and removed size/S labels Oct 4, 2019
@cheald
Copy link
Author

cheald commented Oct 4, 2019

Thanks! And thanks for your patience - I haven't worked with Go modules before! Still learning the ropes.

@lvirbalas
Copy link
Collaborator

Thank you @cheald for contributing! I have some usage-based questions:

  • How does operating system of the OVA react when you change the size? Have you tried with different OVAs of both Linux and Windows variants? I'd think that there should be cases where only partition gets resized, but the extra space is not instantly accessible from the OS. Would be great if you could elaborate on this aspect with different OVA examples.
  • What happens if you reduce the size as opposed to making it larger?

@vbauzys
Copy link
Contributor

vbauzys commented Oct 4, 2019

From perspective point of view. If we can change any disk size in OVA, maybe we should let it in terraform too, not only root/first one ?

@cheald
Copy link
Author

cheald commented Oct 4, 2019

How does operating system of the OVA react when you change the size?

Under Linux, the larger disk size is immediately reflected via lslbk. Filesystems have to be resized to larger extents via resize2fs or similar. This is functionally identical to EBS resizing at AWS. I haven't tested it under Windows.

What happens if you reduce the size as opposed to making it larger?

vCD will not permit this; attempting to shrink a disk results in an API error.

From perspective point of view. If we can change any disk size in OVA, maybe we should let it in terraform too, not only root/first one ?

That's certainly possible. This work could be expanded into full management of internal disks, to complement the independent disk attachment. The govcd APIs take bus/unit IDs and aren't hardcoded to the root disk, so the Terraform provider would just need additional work to consume it.

(As an aside: how do I kick off a Travis build for my latest push? It didn't seem to happen automatically.)

@Didainius
Copy link
Collaborator

@cheald,
Sorry for this being silent for so long. We have been busy on rolling out 2.5.0 and got buried under other tasks.

Now we have yet another task #311 which we want to bring in. I am looking at them and thinking - as the root disk is same as any other disk (just mostly often having address 0:0, but I believe not necessarily for some templates) we could design a structure to accomplish both goals.

What are your thoughts on such proposal? As this may become more time demanding should someone else pick #311 and cover the root disk size functionality as well in the solution of that issue or would you like to bring #311 forward?

@cheald
Copy link
Author

cheald commented Nov 6, 2019 via email

@vbauzys
Copy link
Contributor

vbauzys commented Dec 4, 2019

Hi @cheald. I started work on different approach due that we need support various features, not only root internal disk. You can check and give a spin in https://github.com/terraform-providers/terraform-provider-vcd/pull/412 it is still draft.

@cheald
Copy link
Author

cheald commented Dec 4, 2019

Looking good! I'll close this PR, since it looks like you're headed in a more robust direction. Thank you!

@cheald cheald closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants