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

information returned by node-info from recent PR #1042 is incorrect #1074

Open
memetb opened this issue Feb 23, 2024 · 1 comment
Open

information returned by node-info from recent PR #1042 is incorrect #1074

memetb opened this issue Feb 23, 2024 · 1 comment

Comments

@memetb
Copy link

memetb commented Feb 23, 2024

System Information

Linux distribution

any

Terraform version

terraform -v
Terraform v1.5.7
on linux_amd64

Provider and libvirt versions

terraform-provider-libvirt -version

Description of Issue/Question

Setup

I recently implemented a very similar feature in PR #1073 without realizing that #1042 had very recently been merged. I'm happy to close my PR however the output of the node info is incorrect and less expansive than it could.

Example:

// main.tf
terraform {
  required_version = ">= 0.13"
  required_providers {
    libvirt = {
      #      source  = "dmacvicar/libvirt"
      source = "terraform.local/local/libvirt"
      version = "1.0.0"
    }
  }
}

provider "libvirt" {
  uri = "qemu+ssh://${var.target}/system?sshauth=privkey&no_verify"
}

data "libvirt_node_info" "host" {
}


output node_info {
  value = data.libvirt_node_info.host
}

produces

Changes to Outputs:
  + node_info = {
      + cpu_cores_per_socket = 2
      + cpu_cores_total      = 2
      + cpu_model            = "aarch64"  # Incorrect
      + cpu_sockets          = 1
      + cpu_threads_per_core = 1
      + id                   = "1052227631"
      + memory_total_kb      = 2229728
      + numa_nodes           = 1
    }

By contrast, this is the output of the node info as implemented in PR #1073

Changes to Outputs:
  + node_info = {
      + arch                      = "aarch64"
      + cores                     = 2
      + dies                      = 1
      + features                  = [
          + "fp",
          + "asimd",
          + "evtstrm",
          + "aes",
          + "pmull",
          + "sha1",
          + "sha2",
          + "crc32",
          + "cpuid",
        ]
      + id                        = "d4e1bf70-6374-45b7-a970-fb584a1b4062" # this is the underlying libvirt native id
      + live_migration_support    = true
      + live_migration_transports = [
          + "tcp",
          + "rdma",
        ]
      + model                     = "cortex-a72"  # missing in PR 1042
      + sockets                   = 1
      + threads                   = 1
      + topology                  = [] # not yet implemented but can be expanded on
      + vendor                    = "ARM"  # missing in PR 1042
    }

The implementation for #1073 libvirt/data_source_libvirt_nodeinfo.go pulls and parses the direct XML dump from virt.

I believe it to be more expressive and open to upgrading, especially given that the actual topology can be decoded with a bit extra effort.

I am working on two other PR's right now (#1072 and #1059). I'd be happy to rework #1073 to fix the above. My only ask would be to name the parameters as I've done (i.e. cores not cpu_cores_per_socket) mainly because I've used the sames names as libvirt capabilities schema

@michaelbeaumont @rustydb @dmacvicar

@muresan
Copy link
Contributor

muresan commented Apr 23, 2024

Hi,

at the time I implemented node-info using https://libvirt.org/html/libvirt-libvirt-host.html#virNodeInfo which returns what you saw (with incorrect data) and I did not see that virConnectGetCapabilities exists. Since a release was not cut that includes this format, I'd say feel free to update it to the capabilities structure.

Alternatively, you can add a libvirt_node_capabilities data source, which would map to the virConnectGetCapabilities better, if someone is already using the current node_info (doubt it).

Edit: what I needed at the time was a way to determine total memory on the host and cpu cores, in order to make decisions about the number of cpu cores and memory allocated to VMs. On second thought I would recommend implementing an additional data source named libvirt_node_capabilities because memory doesn't "fit" in the capabilities object and to better map 1:1 what libvirt has with what the provider providess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants