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

provider/vsphere: Improved SCSI controller handling #7908

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

dkalleg
Copy link
Contributor

@dkalleg dkalleg commented Aug 2, 2016

Govmomi tries to use the 7th slot in a scsi controller, which is not
allowed. This patch will appropriately select the slot to attach a disk
to as well as determine if a scsi controller is full.

@stack72
Copy link
Contributor

stack72 commented Aug 2, 2016

Hi @dkalleg

How do the acceptance tests look on your side for this?

P.

@dkalleg
Copy link
Contributor Author

dkalleg commented Aug 2, 2016

@stack72 WIP :) Sort of an emergency fix on my end. I plan to add an unit test for this as well, one that proves 7 scsi disks can be added successfully.

You got back to me really quickly.. and I'm sure 0.7.0 is coming quickly.. should I be setting high prio on these tasks? ;)

@stack72
Copy link
Contributor

stack72 commented Aug 2, 2016

We are currently in lock down for 0.7 release :) Only emergency fixes going in right now so if you think this fits, then let me know!

@dkalleg
Copy link
Contributor Author

dkalleg commented Aug 2, 2016

Gotcha. This is important for my org. I'll will have a new test and all tests passing some time tomorrow for sure. aaaaand I'm also working on one more PR to fix #7897 :) I'm pushing to have both in 0.7.0.

@dkalleg dkalleg force-pushed the scsiControllerFix branch from ecfe897 to cbada27 Compare August 2, 2016 21:02
@dkalleg
Copy link
Contributor Author

dkalleg commented Aug 2, 2016

@stack72 Test added, passing on my end.

Govmomi tries to use the 7th slot in a scsi controller, which is not
allowed. This patch will appropriately select the slot to attach a disk
to as well as determine if a scsi controller is full.
@dkalleg dkalleg force-pushed the scsiControllerFix branch from cbada27 to 6431f75 Compare August 2, 2016 21:10
@apparentlymart apparentlymart changed the title Improved SCSI controller handling provider/vsphere: Improved SCSI controller handling Aug 2, 2016
@stack72
Copy link
Contributor

stack72 commented Aug 3, 2016

Hey @dkalleg

Any chance you can drop the test results run in the thread for history?

P.

@dkalleg
Copy link
Contributor Author

dkalleg commented Aug 3, 2016

$ make testacc TEST=./builtin/providers/vsphere/ TESTARGS="-run TestAccVSphereVirtualMachine_diskSCSICapacity"
==> Checking that code complies with gofmt requirements...
/home/dkalleg/goworkspace/bin/stringer
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/02 13:57:22 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/vsphere/ -v -run TestAccVSphereVirtualMachine_diskSCSICapacity -timeout 120m
=== RUN   TestAccVSphereVirtualMachine_diskSCSICapacity
2016/08/02 13:57:50 [DEBUG] template=
resource "vsphere_virtual_machine" "scsiCapacity" {
    name = "terraform-test"

%s
    vcpu = 2
    memory = 1024
    network_interface {
        label = "%s"
        ipv4_address = "%s"
        ipv4_prefix_length = %s
        ipv4_gateway = "%s"
    }
     disk {
%s
        template = "%s"
        iops = 500
    }

    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "one"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "two"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "three"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "four"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "five"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "six"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "seven"
    }
}
2016/08/02 13:57:50 [DEBUG] template config=
resource "vsphere_virtual_machine" "scsiCapacity" {
    name = "terraform-test"

    datacenter = "FTC"
    resource_pool = "Cluster1"

    vcpu = 2
    memory = 1024
    network_interface {
        label = "VM Private"
        ipv4_address = ""
        ipv4_prefix_length = 24
        ipv4_gateway = ""
    }
     disk {
        datastore = "san1"

        template = "DansTfTest/danTestTemplate"
        iops = 500
    }

    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "one"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "two"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "three"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "four"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "five"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "six"
    }
    disk {
        size = 1
        controller_type = "scsi-paravirtual"
        name = "seven"
    }
}
2016/08/02 13:57:50 [WARN] Invalid log level: "all". Defaulting to level: TRACE. Valid levels are: [TRACE DEBUG INFO WARN ERROR]
--- PASS: TestAccVSphereVirtualMachine_diskSCSICapacity (189.97s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/vsphere    190.018s

@stack72
Copy link
Contributor

stack72 commented Aug 4, 2016

This LGTM! Thanks @dkalleg :)

@stack72 stack72 merged commit ed77105 into hashicorp:master Aug 4, 2016
@dkalleg
Copy link
Contributor Author

dkalleg commented Aug 4, 2016

Thanks @stack72 !

@dkalleg dkalleg deleted the scsiControllerFix branch August 4, 2016 19:35
@ghost
Copy link

ghost commented Apr 23, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants