Draft: Add tests for importing templates

[thijsvandergugten]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for PR followers and do not help prioritize the request

Description

In this merge-request, it is attempted to add automated tests for importing templates. Since there are some differences between the output received from Opennebula and the data written into the Terraform-state, some workarounds were necessary.

References

#356

New or Affected Resource(s)

• opennebula_template

Checklist

• I have created an issue and I have mentioned it in References
• My code follows the style guidelines of this project (use go fmt)
• My changes generate no new warnings or errors
• I have updated the unit tests and they pass succesfuly
• I have commented my code, particularly in hard-to-understand areas
• I have updated the documentation (if needed)
• I have updated the changelog file

[treywelsh]

Hi @thijsvandergugten,

What is the status of this PR ? are you experiencing some difficulties or do you need some help ?

[thijsvandergugten]

Hello @treywelsh,

As far as I can see, the code for this PR should be ready, since the tests are passing. However, since this is my first pull-request, I am not sure whether the code makes sense from the perspective of an Opennebula-expert or Terraform-expert:

• There are some hardcoded references to field-names from the Opennebula-output. Is there a more elegant way to deal with the difference between tags and other information about an Opennebula-template?
• Currently the Terraform-state for (for example) the fields OS or FEATURES are stored as a list of dictionaries, where this list has (always?) one element. Does my PR handle this correctly?
• As far as I can see, Opennebula usually returns uppercase-text, but in Terraform most often lowercase-text is used. Does my PR handle this correctly? Does my PR need to include more explanation and/or documentation-updates about uppercase versus lowercase? If Opennebula always gives uppercase-text as an api-response, we also need to write uppercase-text in the Terraform-state, even if there is lowercase-text in the Terraform-resource-file.

Of course I would like to write comments and/or documentation, but before this I would like to know whether this PR is good enough to start adding documentation, and whether anything needs to be written in the changelog.

[treywelsh]

Hi,

Thanks for your PR, it's an interesting one for someone who didn't contribute before.
I don't pretend to be an expert but I'll try to understand your work and to help you as much as I can. Feel free to discuss if you don't agree with something.

I asked you the state of this PR because at first glance I didn't get some changes.
I'll details them in a review, this message it to answer to the points of your message above with more details.
I'll probably run some manual tests later to be sure I fully get how your code works (in addition, acceptance test coverage is sometimes not sufficient: missing test cases etc..).

• There are some hardcoded references to field-names from the Opennebula-output. Is there a more elegant way to deal with the difference between tags and other information about an Opennebula-template?

  You don't like to have to copy/paste strings like "CPU", "MEMORY" etc. right ?
  If yes: I agree with you we shouldn't copy/paste strings keys (Ok, I did it in somes places but I don't pretend it's a good thing).
  For each resource there is a package that contains a bunch of constant values to not have to type strings, look at the top of the resource template file in the import section:
  vmk "github.com/OpenNebula/one/src/oca/go/src/goca/schemas/vm/keys"
  So in the template file you're editing, instead of "CPU", you just have to use this: vmk.CPU
  There is vmk.Description, vmk.Memory etc.
  However, some keys may be missing, to add more values feel free to make a PR to goca (the Golang binding for the OpenNebula XML-RPC API) in the one repository here

• Currently the Terraform-state for (for example) the fields OS or FEATURES are stored as a list of dictionaries, where this list has (always?) one element. Does my PR handle this correctly?

  To be honest I don't really like this nesting of complex data structures (with TypeSet, TypeList etc.) but it's probably done this way to be able to represent all kinds of schemas.
  For instance, a TypeSet could be used to represent various kind of data structures:

  • a list of IDs: [3, 6, 4343]
  • a single data structure with each attribute described once

  For a bit more of details see examples in the terraform documentation on TypeSet data type

  So yes the section features that we retrieve in the code is a list of dictionnaries with zero or (only) one element: a dictionary with unique keys. And yes, for this case the list may seems useless.
  But yes It seems you get how to read these values

• As far as I can see, Opennebula usually returns uppercase-text, but in Terraform most often lowercase-text is used. ...

  I keep using lower case for consistency across the provider, but I don't have strong argument to keeping lowercase all names in the tf files.
  I'm ok with uppercase everywhere and I understand your point but the initial choice (not sure it was mine) was lowercase.
  At least open a separate issue to handle this to avoid mixings problems.

[treywelsh]

interesting first shot, thanks for your efforts

[treywelsh]

I think we don't need this, tags are already read in the flattenVMUserTemplate method called later in the code.
There is some parts of the code that are in methods due to the fact that are shared between resources.
The template resource share some code with the resources: VM, virtual router instance, and virtual router instance template

[thijsvandergugten]

In the method flattenVMUserTemplate there was a line if tagsInterface, ok := d.GetOk("tags"); ok {. Because of this, only tags which already were present in the state will be processed. I will add a commit where the method flattenVMUserTemplate is updated: it will have a loop over all elements in the template, instead of over all elements in the state.

[treywelsh]

Please don't call to much methods in the same line this make you code more difficult to read, it's not a problem to add more code lines to make the code more readable/maintainable.

However you get how to read TypeSet elements OS, FEATURES etc. that's a good point

[thijsvandergugten]

This long line has been separated in multiple lines, and an explanation has been added.

[treywelsh]

Reuse getOS for the context make this code part less understandable.
You could just rename getOS for something more generic like getVector
Or you could instead define a new map in each case of the switch.

[thijsvandergugten]

Thanks for the suggestion, now the name getVector is used.

[treywelsh]

I'm surprised that you set an empty value, you could retrieve the description from the template via

desc, err := tpl.Get(vmk.Description)
...

[thijsvandergugten]

The fields description and sched_requirements were difficult to handle: if this field is removed from the Terraform-resource-configuration, it does not get removed from Terraform-state (it has an empty value, which gives problems in the unit-tests). I will write a new commit with an update of the method flattenVMUserTemplate: in this method, the description-field is already taken care of

[treywelsh]

Why did you change the case ? is it for test purposes ?
Not a big deal but this just add a bit of noise for this PR.

[thijsvandergugten]

In the file opennebula/resource_opennebula_virtual_machine_test.go, there are a lot of resources with the key context. All these context-definitions have uppercase-keys, so it was easier to update this file opennebula/resource_opennebula_template_test.go. This is also an example of conflicts between uppercase-text (Opennebula) and lowercase-text (Terraform).

[treywelsh]

I didn't used ImportStateVerify field before, I'll need to read more doc on it, test it.

Next step, you could update your PR, discuss a bit with me and I'll make a new review to move forward.

[thijsvandergugten]

Hello,

Thanks for your response. In have followed up on the detailed reviews, and written a response on some of them. About the general points:

• Indeed, I prefer not to copy strings like CPU or MEMORY. Where possible, I will use the keys from the imported package vmk.
• Thanks for the explanation on the different datatypes in Terraform. To prevent random selections of fields in my PR, I will adhere to the schema commonTemplateSchemas() which is defined in the file resource_opennebula_template.go.
• Thanks for the explanation on uppercase versus lowercase. I will create a separate issue about this.

[thijsvandergugten]

Some automated tests are failing, which is related to #330. In this issue, the result was that tags in a virtual-machine-template will not be used in the Terraform-state for a virtual machine. This adds some complexity when a Terraform-managed virtual machines is imported into the Terraform-state.

[treywelsh]

I saw some diffs in the CI test results so I tried to quickly dig around theses ones to understand what's happening.

• For the sched_message diff in VMs tags section:
  sched_message may appears in the UserTemplate section of the VM with a message from the scheduler, for instance when it's not possible to deploy a VM due to insufficient resources etc.
  I tried to make this tag appear creating to much VM on a small dev OpenNebula instance.
  I got this: SCHED_MESSAGE = "Tue Oct 18 16:10:26 2022: Cannot dispatch VM: No host with enough capacity to deploy the VM"

  Then I removed some VMs, and then the scheduler was able to deploy the VM, however the tag remained but was emtpy:
  SCHED_MESSAGE = ""

  At first glance I don't know why SCHED_MESSAGE appears in the acceptance tests.
  You seems to read it in the flattenVMUserTemplate method in this piece of code:

   if _, ok := commonTemplateSchemas()[strings.ToLower(pair.Key())]; !ok {
    	tags[strings.ToLower(pair.Key())] = pair.Value
    	tagsAll[strings.ToLower(pair.Key())] = pair.Value
    }
  

  It appears to me that with your way to do, we should have a list of OpenNebula "possible" keys to ignore when reading tags. But I'm not sure this exists without looking at the OpenNebula source code. At best I know the existence of XSD files like https://github.com/OpenNebula/one/blob/master/share/doc/xsd/vm.xsd

• There is also some diffs on the virtual router but I didn't took a look in depth at what's happening.
  At first glance we could think it's weird to have these resources impacted but: template, virtual_machine, virtual_router_instance_template, virtual_router_instance all shares some pieces of code.
  I tried to share as much code as I could between theses resources to avoid to have several copies of the same code.

  vrouter is a specific virtual router template attribute, but AFAIK it's not the case for env.
  I didn't dig more than this here.

But maybe that you are aware of all these points so what's the status, are you stuck ?
If yes I could try to spend a bit more time to understand in depth and see what's happening

[thijsvandergugten]

Thanks for your response. Below are some concerns, for which I unfortunately do not have an easy solution.

• Regarding the field sched_message, unfortunately I did not see a way to automatically get a list of keys to ignore, so I manually added sched_message and error.
• Regarding the failing test in https://github.com/OpenNebula/terraform-provider-opennebula/blob/1.1.0/opennebula/resource_opennebula_virtual_machine_test.go#L613, I am having some difficulties with hiding the field sched_requirements for the virtual machine if it is defined in the template. A possible solution is: if the field sched_requirements is defined for a virtual machine, add it to the state, regardless of whether if is caused by the virtual-machine-template or not. However, this does not agree with SCHED_REQUIREMENTS and SCHED_DS_REQUIREMENTS are set to null after the VM has been deployed #330 (comment), so here I do not know an easy solution yet.
• Regarding the the failing test in https://github.com/OpenNebula/terraform-provider-opennebula/blob/1.1.0/opennebula/resource_opennebula_virtual_router_test.go#L22, I am having some difficulties with the fact that in the function resourceOpennebulaVirtualRouterInstanceTemplateCreate in https://github.com/OpenNebula/terraform-provider-opennebula/blob/1.1.0/opennebula/resource_opennebula_virtual_router_instance_template.go#L31 there is al reference to tpl.AddPair("VROUTER", "YES"), but this mechanism is not in place in the function resourceOpennebulaVirtualRouterInstanceTemplateUpdate, so here I also do not have an easy solution.

[treywelsh]

The diffs we see are due to keys that are read by the provider but not described in the current resource being read (but added by OpenNebula, inherited, or due to the provider architecture (VROUTER key hidden inside vrouter resources) etc.).

First, I think this PR would need a list of keys to exclude when reading the template section of several resources (the template resource has also a template section it's a bit confusing).
So a list of keys that concern resources vrouter_instance, vrouter_instance_template, template, virtual_machine (they all have some shared code).

Here are some keys to intialize this list:

• potentially some hypervisor related keys: here are some vcenter related virtual machine keys in the user template we may want to exclude: https://github.com/OpenNebula/one/blob/master/share/doc/xsd/vm.xsd#L197
  I don't know if there is other specific keys for other hypervisors.
• There is also OpenNebula related stuffs: SCHED_MESSAGE, ERROR like you said,
  and probably some other keys for which I'm not sure until I read more documentation for each:
  MEMORY_UNIT_COST, INPUTS_ORDER, HYPERVISOR, USER_INPUTS and probably more (I quickly read theses keys in the USER_TEMPLATE section on runnings VMs I saw)
• We also want to exclude VROUTER which is "hidden" inside vrouter template resources management (so we could consider there is no need for the user to be configurable via the tags section).
  This should remove the vrouter = yes diff from the CI failure.

We need to make the more exhaustive list of attribute to make this list of keys to exclude. Each key we'll forget will be a potential future issue created by another user.

Then we'll need to exclude virtual machine template keys inherithed from a template (when the VM has been instantiated from a template via the template_id attribute) when they are not overidden in the VM description.
This should remove the env = prod diff from the CI failure.
It think it's the same thing than with sched_requirements.
To achieve this we could use the template_tags section introduced by #341 which keep a list of inherited tags keys.

If we merge this PR, as you can see, there is a risk that the provider show some diffs for a bunch of users (in case they use an other hypervisor for instance etc.) and we may end up we a bunch of small issues to fix.
In addition, to be consistent we may want to make the tags section work in the same way for all resources.
And we may have the same discussion for each ressource (i.e. does datastore/image/... resources have specific keys ?).

Even if I'm trying to be careful I may still miss some cases.
Feel free to discuss if you have some other ideas, if I'm not clear or if you think I'm wrong.
I think this should be discussed we @frousselet as well as it's not a minor change.

Thank you for this discussion

[thijsvandergugten]

I still need to check the failing tests. It seems that the changes from #362 have caused an extra test-failure, because the parameter template_section does not get imported correctly. I need to check this and the other failures.

[treywelsh]

FYI: in #378 (not merged yet) I modified reading of SCHED_REQUIREMENTS, SCHED_DS_REQUIREMENTS, and DESCRIPTION.
The goal of this PR is to improve the import of sections and attributes described in the VM schema.
This may add a conflict soon with your PR on master branch

[github-actions]

This pull request is stale because it has been open for 30 days with no activity and it is not in a milestone. Remove 'status: stale' label or comment, or this will be closed in 5 days.