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

Update Harvester node driver to support multi nics and disks #1051

Merged
merged 3 commits into from Mar 20, 2023

Conversation

futuretea
Copy link
Contributor

@futuretea futuretea commented Jan 5, 2023

Signed-off-by: futuretea Hang.Yu@suse.com

need harvester node driver v0.6.1
rancher v2.6.11-rc4 and rancher v2.7.2-rc2 already include the new Harvester node driver

API changes
Harvester node driver add two new fields disk_info and network_info to support multi nics and disks in
harvester/docker-machine-driver-harvester@37b5b37#diff-1a5e9ad2c9efb72d85e174ec77c9a20a65c98f9f702d72ef615b1b552d1af7c0

In terraform-provider-rancher2, we need to remind the user that the old field is deprecated because we want the user to use the new field (The new field is an extensible field that supports single or multiple disks and networks).

  • disk_size - (Deprecated) Use disk_info instead
  • disk_bus - (Deprecated) Use disk_info instead
  • image_name - (Deprecated) Use disk_info instead
  • disk_info - (Required) A JSON string specifying info for the disks e.g. {\"disks\":[{\"imageName\":\"harvester-public/image-57hzg\",\"bootOrder\":1,\"size\":40},{\"storageClassName\":\"node-driver-test\",\"bootOrder\":2,\"size\":1}]} (string)
  • network_name - (Deprecated) Use network_info instead
  • network_model - (Deprecated) Use network_info instead
  • network_info - (Required) A JSON string specifying info for the networks e.g. {\"interfaces\":[{\"networkName\":\"harvester-public/vlan1\",\"macAddress\":\"\"},{\"networkName\":\"harvester-public/vlan2\",\"macAddress\":\"5a:e7:c5:24:5b:44\"}]} (string)

Related issues
harvester/harvester#2733
#985
#984

Depend PRs
rancher/rancher#40172
rancher/rancher#40171

Test Plan

  1. Setup a Harvester v1.1.1 cluster, refer to https://docs.harvesterhci.io/v1.1/install/iso-install
  2. Add a cloud image a a vlan network to the Harvester by using the following terraform config:
terraform {
  required_version = ">= 0.13"
  required_providers {
    harvester = {
      source  = "harvester/harvester"
      version = "0.6.1"
    }
  }
}

provider "harvester" {
 kubeconfig = "<the kubeconfig file path of the harvester cluster>"
}

resource "harvester_image" "focal-server" {
  name      = "focal-server"
  namespace = "harvester-public"

  display_name = "focal-server-cloudimg-amd64.img"
  source_type  = "download"
  url          = "https://cloud-images.ubuntu.com/focal/current/focal-server-cloudimg-amd64.img"
}

data "harvester_clusternetwork" "mgmt" {
  name = "mgmt"
}

resource "harvester_network" "mgmt-vlan1" {
  name      = "mgmt-vlan1"
  namespace = "harvester-public"

  vlan_id = 1

  route_mode           = "auto"
  route_dhcp_server_ip = ""

  cluster_network_name = data.harvester_clusternetwork.mgmt.name
}
terraform init
terraform apply
  1. Setup a Rancher v2.6-head/v2.7-head cluster
  2. Import Harvester cluster to the Rancher cluster in Virtualization Management use cluster name foo-harvester
    1676562316085
  3. Build terraform-provider-rancher2 from the PR branch
CGO_ENABLED=0 go build -mod=vendor .
  1. install the custom provider as version 0.0.0-master:
REPO="rancher"
NAME="rancher2"
VERSION="0.0.0-master"
PROVIDERS_DIR=$HOME/.terraform.d/plugins/registry.terraform.io/${REPO}/${NAME}
PROVIDER_DIR=${PROVIDERS_DIR}/${VERSION}/linux_amd64
mkdir -p ${PROVIDER_DIR}
cp ./terraform-provider-${NAME} ${PROVIDER_DIR}/terraform-provider-${NAME}_v${VERSION}
  1. Use the following test config
terraform {
  required_providers {
    rancher2 = {
      source = "rancher/rancher2"
      version = "0.0.0-master"
    }
  }
}


provider "rancher2" {
  api_url    = "<>"
  access_key = "<>"
  secret_key = "<>"
  insecure = true
}


data "rancher2_cluster_v2" "foo-harvester" {
  name = "foo-harvester"
}

# Create a new Cloud Credential for an imported Harvester cluster
resource "rancher2_cloud_credential" "foo-harvester" {
  name = "foo-harvester"
  harvester_credential_config {
    cluster_id = data.rancher2_cluster_v2.foo-harvester.cluster_v1_id
    cluster_type = "imported"
    kubeconfig_content = data.rancher2_cluster_v2.foo-harvester.kube_config
  }
}

# Create a new rancher2 machine config v2 using harvester node_driver
resource "rancher2_machine_config_v2" "foo-harvester-v2" {
  generate_name = "foo-harvester-v2"
  harvester_config {
    vm_namespace = "default"
    cpu_count = "2"
    memory_size = "4"
    disk_info = <<EOF
    {
        "disks": [{
            "imageName": "harvester-public/focal-server",
            "size": 40,
            "bootOrder": 1
        }]
    }
    EOF
    network_info = <<EOF
    {
        "interfaces": [{
            "networkName": "harvester-public/mgmt-vlan1",
            "macAddress": ""
        }]
    }
    EOF
    ssh_user = "ubuntu"
    user_data = "I2Nsb3VkLWNvbmZpZwpwYWNrYWdlX3VwZGF0ZTogdHJ1ZQpwYWNrYWdlczoKICAtIHFlbXUtZ3Vlc3QtYWdlbnQKICAtIGlwdGFibGVzCnJ1bmNtZDoKICAtIC0gc3lzdGVtY3RsCiAgICAtIGVuYWJsZQogICAgLSAnLS1ub3cnCiAgICAtIHFlbXUtZ3Vlc3QtYWdlbnQuc2VydmljZQo="
  }
}

resource "rancher2_cluster_v2" "foo-harvester-v2" {
  name = "foo-harvester-v2"
  kubernetes_version = "v1.24.10+rke2r1"
  rke_config {
    machine_pools {
      name = "pool1"
      cloud_credential_secret_name = rancher2_cloud_credential.foo-harvester.id
      control_plane_role = true
      etcd_role = true
      worker_role = true
      quantity = 1
      machine_config {
        kind = rancher2_machine_config_v2.foo-harvester-v2.kind
        name = rancher2_machine_config_v2.foo-harvester-v2.name
      }
    }
    machine_selector_config {
      config = {
        cloud-provider-name = ""
      }
    }
    machine_global_config = <<EOF
cni: "calico"
disable-kube-proxy: false
etcd-expose-metrics: false
EOF
    upgrade_strategy {
      control_plane_concurrency = "10%"
      worker_concurrency = "10%"
    }
    etcd {
      snapshot_schedule_cron = "0 */5 * * *"
      snapshot_retention = 5
    }
    chart_values = ""
  }
}
terraform init
terraform apply

When I apply for the first time, such an error will occur, but it is OK to apply again. Is there anything wrong in my configuration file? Or is it a known problem?

rancher2_cloud_credential.foo-harvester: Creation complete after 2s [id=cattle-global-data:cc-rqgkh]
╷
│ Error: Provider produced inconsistent final plan
│
│ When expanding the plan for rancher2_cluster_v2.foo-harvester-v2 to include new values learned so far during apply,
│ provider "registry.terraform.io/rancher/rancher2" produced an invalid new value for
│ .rke_config[0].machine_pools[0].cloud_credential_secret_name: was cty.StringVal(""), but now
│ cty.StringVal("cattle-global-data:cc-rqgkh").
│
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.
  1. clean resources
terraform destroy
  1. clean the custom provider
rm -r ~/.terraform.d/plugins/

@futuretea futuretea force-pushed the harvester branch 6 times, most recently from dc79809 to 69ea805 Compare January 11, 2023 02:13
@futuretea futuretea force-pushed the harvester branch 2 times, most recently from 0d821c6 to a79af48 Compare January 18, 2023 14:26
@futuretea futuretea changed the title Update Harvester node driver to supoort multi nics and disks Update Harvester node driver to suport multi nics and disks Jan 18, 2023
@futuretea futuretea force-pushed the harvester branch 3 times, most recently from ba9f145 to 5959241 Compare January 19, 2023 04:32
@futuretea futuretea changed the title Update Harvester node driver to suport multi nics and disks Update Harvester node driver to support multi nics and disks Jan 19, 2023
@futuretea futuretea marked this pull request as ready for review January 19, 2023 06:48
@a-blender a-blender self-requested a review February 15, 2023 16:10
Copy link
Contributor

@a-blender a-blender left a comment

Choose a reason for hiding this comment

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

Hi @futuretea

@a-blender
Copy link
Contributor

I'd also like another pair of eyes with Harvester experience on this

@futuretea futuretea marked this pull request as draft February 16, 2023 16:35
@futuretea futuretea marked this pull request as ready for review February 16, 2023 18:17
@futuretea futuretea requested review from a-blender and removed request for slickwarren February 16, 2023 18:17
@futuretea
Copy link
Contributor Author

futuretea commented Feb 16, 2023

@annablender Hi, I updated the instructions and cancelled the review of @slickwarren by mistake. Could you please help me make a request again? Thanks!

@a-blender
Copy link
Contributor

@futuretea Sorry, could you answer my above questions #1051 (review)? Thanks so much

@futuretea
Copy link
Contributor Author

futuretea commented Feb 28, 2023

@a-blender Hi

This PR harvester/harvester#2733 specifies to add a second NIC and disk config to rancher2_machine_config_v2 but it looks here like you are deprecating the other network/disk fields and replacing them with network_info and disk_info. Could you explain this decision?

old field only allow user to add on disk/nic
new field network_info and disk_info can allow user to add multi disks and nics

Has this been tested? If so, please attach the dev test plan or what has already been done to verify this works with the new harvester node driver in rancher.

Yes, I already test it by following the test plan

@lanfon72
Copy link
Member

lanfon72 commented Mar 2, 2023

Notable problem

User Data and Network Data display incorrectly.
image

Verified

  • Able to provision RKE2 cluster with multiple NICs/volumes
  • Able to update RKE2 cluster config after terraform provisioned
  • Able to remove RKE2 cluster that provisioned

Test Information

  • Environment: qemu/KVM 3 nodes
  • Harvester Version: v1.1-a002d078-head
  • ui-source Option: Auto
  • Rancher Version:
    • v2.7-c24fb8b0869a0b445f55b3307c6ed4582e147747-head (Docker Image: 216bef921570)
    • v2.6-a55f968896341f311fc8302b3ed6247da3c13900-head (Docker Image: 122aa2f1b66b)

@bk201 bk201 requested a review from FrankYang0529 March 3, 2023 02:56
Copy link

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

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

Overall LGTM and test case work. Just a minor question, do we want to check whether disk_info and network_info is a valid JSON format and whether their keys are correct?

@futuretea
Copy link
Contributor Author

do we want to check whether disk_info and network_info is a valid JSON format and whether their keys are correct?

@FrankYang0529 Having implemented the check logic on the node driver side.

@futuretea
Copy link
Contributor Author

User Data and Network Data display incorrectly.

@lanfon72 Can you help to file another issue to track this? CC @WuJun2016, userData and networkData passed through terraform-provider-rancher2 may be plaintext, not base64. The UI needs an enhancement to support this case. Thanks.

@lanfon72
Copy link
Member

lanfon72 commented Mar 6, 2023

User Data and Network Data display incorrectly.

@lanfon72 Can you help to file another issue to track this? CC @WuJun2016, userData and networkData passed through terraform-provider-rancher2 may be plaintext, not base64. The UI needs an enhancement to support this case. Thanks.

I have no idea where should I file the issue 😹 (rancher/rancher or harvester/harvester or rancher/dashboard?)

@futuretea
Copy link
Contributor Author

User Data and Network Data display incorrectly.

@lanfon72 Can you help to file another issue to track this? CC @WuJun2016, userData and networkData passed through terraform-provider-rancher2 may be plaintext, not base64. The UI needs an enhancement to support this case. Thanks.

I have no idea where should I file the issue 😹 (rancher/rancher or harvester/harvester or rancher/dashboard?)

harvester/harvester

Copy link

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

Copy link
Contributor

@slickwarren slickwarren left a comment

Choose a reason for hiding this comment

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

just one nit

docs/resources/cluster_v2.md Outdated Show resolved Hide resolved
Signed-off-by: futuretea <Hang.Yu@suse.com>
Signed-off-by: futuretea <Hang.Yu@suse.com>
Signed-off-by: futuretea <Hang.Yu@suse.com>
@bk201
Copy link
Member

bk201 commented Mar 14, 2023

@a-blender @slickwarren Please help review the PR again, we're going to release Harvester v1.1.2 and need this change. Thanks.

Copy link
Contributor

@a-blender a-blender left a comment

Choose a reason for hiding this comment

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

LGTM thank you, this can be merged.

@a-blender a-blender merged commit 7efed7e into rancher:master Mar 20, 2023
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

Successfully merging this pull request may close these issues.

None yet

6 participants