/kubernetes

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 vSphere Cloud Provider simulator based tests #55918

Merged
merged 2 commits into from Jan 19, 2018

Conversation

Reviewers

@abrarshivani abrarshivani

Assignees

@cheftako cheftako

@thockin thockin

Labels
Projects
None yet
Milestone
No milestone
9 participants
@dougm @dims @abrarshivani @lavalamp @thockin @k8s-ci-robot @k8s-merge-robot @jbeda @cheftako
@dougm
Member

dougm commented Nov 17, 2017

What this PR does / why we need it:

Initial set of vSphere Cloud Provider functional tests against the vCenter simulator, provides test coverage without having to run against a real vCenter instance.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

The vsphere simulator recently moved from vmware/vic to govmomi, I had discussed the idea of introducing it for testing with vSphere Cloud Provider maintainers. These tests provide 90%+ coverage for vclib/datacenter.go, but we can expand further of course.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none size/L cncf-cla: yes needs-ok-to-test labels Nov 17, 2017

@k8s-merge-robot k8s-merge-robot assigned jbeda and lavalamp Nov 17, 2017

@dougm dougm changed the title from Vcp vcsim to Add vSphere Cloud Provider simulator based tests Nov 17, 2017

@dims

This comment has been minimized.

Sign in to view
Member

dims commented Nov 17, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test label Nov 17, 2017

@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Nov 20, 2017

/retest
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Nov 27, 2017

Test failures are fixed by #56136 , but after rebase currently fails locally due to #56393 - I'll rebase again when that is fixed.

@k8s-merge-robot k8s-merge-robot added the needs-rebase label Nov 28, 2017

@k8s-merge-robot k8s-merge-robot removed the needs-rebase label Dec 12, 2017

@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Dec 12, 2017

@rohitjogvmw @tusharnt anyone from the VCP team available to review?
@abrarshivani

abrarshivani reviewed Dec 14, 2017
View changes

Thanks, @dougm for adding this.

pkg/cloudprovider/providers/vsphere/vclib/datacenter_test.go
vc := &VSphereConnection{GoVmomiClient: c}
_, err = GetDatacenter(ctx, vc, "enoent")

This comment has been minimized.

Sign in to view
Copy link
@abrarshivani

abrarshivani Dec 14, 2017

Member

I think it would better to add constants here instead of directly using strings. Do we have govmomi constants for the strings?

This comment has been minimized.

Sign in to view
Copy link
@dougm

dougm Dec 14, 2017

Member

Sure we can add a constant for this name, but would be in this test rather than govmomi (see below).

This comment has been minimized.

Sign in to view
Copy link
@abrarshivani

abrarshivani Dec 15, 2017

Member

Instead of adding constant in this file it would be better to add in here "pkg/cloudprovider/providers/vsphere/vclib/constants.go" so other tests can use it
pkg/cloudprovider/providers/vsphere/vclib/datacenter_test.go
t.Error("expected error")
}
dc, err := GetDatacenter(ctx, vc, "DC0")

This comment has been minimized.

Sign in to view
Copy link
@abrarshivani

abrarshivani Dec 14, 2017

Member

ditto

This comment has been minimized.

Sign in to view
Copy link
@dougm

dougm Dec 14, 2017

Member

In this case DC0 is expected to exist. I can change the tests to avoid the string literal names, just makes the test code more verbose. For example, instead of hardcoding DCO and LocalDS_0: https://github.com/vmware/govmomi/blob/68f3d8490ce3e7967e0ec283cd13bee465887c78/simulator/virtual_machine_test.go#L54-L71

This comment has been minimized.

Sign in to view
Copy link
@abrarshivani

abrarshivani Dec 15, 2017

Member

It is safer to avoid string literals. We can add util functions to avoid repetition of the same code across multiple tests.
pkg/cloudprovider/providers/vsphere/vclib/datacenter_test.go
vc := &VSphereConnection{GoVmomiClient: c}
_, err = GetDatacenter(ctx, vc, "enoent")
if err == nil {

This comment has been minimized.

Sign in to view
Copy link
@abrarshivani

abrarshivani Dec 14, 2017 •
edited

Member

Can you please explain why enoent should return err as nil? Is this handled differently in the simulator? Since I cannot find it here https://github.com/vmware/govmomi/blob/master/vcsim/README.md/#usage

This comment has been minimized.

Sign in to view
Copy link
@dougm

dougm Dec 14, 2017

Member

It is the opposite, these tests are making sure that enoent returns an error that is not nil. Look at the line below, calls t.Error if err == nil. The name enoent does not exist anywhere in the inventory, the tests are expecting these calls to fail, providing coverage of the error paths in datacenter.go.

This comment has been minimized.

Sign in to view
Copy link
@abrarshivani

abrarshivani Dec 15, 2017

Member

Sounds good to me
pkg/cloudprovider/providers/vsphere/vclib/datacenter_test.go
t.Error(err)
}
vmdk := ds.Path(avm.Name + "/disk1.vmdk")

This comment has been minimized.

Sign in to view
Copy link
@abrarshivani

abrarshivani Dec 14, 2017

Member

I would prefer variable name such as disk1Path

This comment has been minimized.

Sign in to view
Copy link
@dougm

dougm Dec 14, 2017

Member

sure, I'll change that.
@abrarshivani

This comment has been minimized.

Sign in to view
Member

abrarshivani commented Dec 14, 2017

/test pull-kubernetes-e2e-gce
@abrarshivani

This comment has been minimized.

Sign in to view
Member

abrarshivani commented Dec 14, 2017 •
edited

@dougm just executing command make test is enough to run the tests, right?
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Dec 14, 2017

@abrarshivani yes, make test will run these tests.
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Dec 15, 2017

@abrarshivani I've moved the string literals to named constants.
@abrarshivani

This comment has been minimized.

Sign in to view
Member

abrarshivani commented Dec 15, 2017

/test pull-kubernetes-unit
@abrarshivani

This comment has been minimized.

Sign in to view
Member

abrarshivani commented Dec 15, 2017

LGTM
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Dec 16, 2017

/retest
@abrarshivani

This comment has been minimized.

Sign in to view
Member

abrarshivani commented Dec 18, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot assigned abrarshivani Dec 18, 2017

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 18, 2017

@kubernetes kubernetes deleted a comment from k8s-merge-robot Dec 18, 2017

@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Dec 18, 2017

Need review for the Godeps + vendor/ changes, can one of the owners PTAL? @brendandburns @dchen1107 @jbeda @lavalamp @smarterclayton @thockin

@k8s-merge-robot k8s-merge-robot added needs-rebase and removed lgtm needs-rebase labels Dec 19, 2017

@k8s-merge-robot k8s-merge-robot added the needs-rebase label Jan 8, 2018

dougm added some commits Nov 17, 2017

@dougm
Update vmware/govmomi godeps
09da53c
@dougm
Add vSphere Cloud Provider simulator based tests 
Initial set of vcsim based tests.
Loading status checks…
a8180d6

@k8s-merge-robot k8s-merge-robot removed the needs-rebase label Jan 8, 2018

@lavalamp lavalamp assigned cheftako and thockin and unassigned jbeda, lavalamp and abrarshivani Jan 8, 2018

@lavalamp

This comment has been minimized.

Sign in to view
Member

lavalamp commented Jan 8, 2018

Assigning to @thockin and @cheftako, one of them should know the current req's for cloud provider changes.

@prashima prashima referenced this pull request Jan 9, 2018

Merged

Renews cached NodeInfo with new vSphere connection #57978

2 of 2 tasks complete
@thockin

This comment has been minimized.

Sign in to view
Member

thockin commented Jan 18, 2018

@cheftako the move-out can't happen soon enough :)

I'm going to approve this since it is tests and not functionality.

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 18, 2018

@k8s-ci-robot

This comment has been minimized.

Sign in to view
Contributor

k8s-ci-robot commented Jan 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrarshivani, dougm, thockin

Associated issue requirement bypassed by: thockin

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved label Jan 18, 2018

@k8s-merge-robot

This comment has been minimized.

Sign in to view
Contributor

k8s-merge-robot commented Jan 18, 2018

/test all

Tests are more than 96 hours old. Re-running tests.
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Jan 18, 2018

/test pull-kubernetes-e2e-kops-aws
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Jan 18, 2018

/test pull-kubernetes-verify
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Jan 18, 2018

/test pull-kubernetes-e2e-kops-aws
@k8s-merge-robot

This comment has been minimized.

Sign in to view
Contributor

k8s-merge-robot commented Jan 18, 2018

/test all

Tests are more than 96 hours old. Re-running tests.
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Jan 18, 2018

/retest
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Jan 18, 2018

/test pull-kubernetes-e2e-kops-aws
1 similar comment
@dougm

This comment has been minimized.

Sign in to view
Member

dougm commented Jan 18, 2018

/test pull-kubernetes-e2e-kops-aws
@k8s-merge-robot

This comment has been minimized.

Sign in to view
Contributor

k8s-merge-robot commented Jan 18, 2018

/test all

Tests are more than 96 hours old. Re-running tests.
@k8s-merge-robot

This comment has been minimized.

Sign in to view
Contributor

k8s-merge-robot commented Jan 19, 2018

/test all [submit-queue is verifying that this PR is safe to merge]
@k8s-merge-robot

This comment has been minimized.

Sign in to view
Contributor

k8s-merge-robot commented Jan 19, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit c1f2da7 into kubernetes:master Jan 19, 2018
12 of 13 checks passed

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation dougm authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@dougm dougm referenced this pull request Jan 19, 2018

Merged

Add vSphere Cloud Provider vclib tests #58534

k8s-merge-robot added a commit that referenced this pull request Jan 24, 2018

@k8s-merge-robot
Merge pull request #58534 from dougm/vclib-tests 
Automatic merge from submit-queue (batch tested with PRs 58756, 58758, 58725, 52799, 58534). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Add vSphere Cloud Provider vclib tests

**What this PR does / why we need it**:
Additional vSphere Cloud Provider functional tests against vcsim, providing more test coverage without having to run against a real vCenter instance.

Follow up to #55918

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:

**Special notes for your reviewer**:

This set of tests focuses on Datastore, Folder and VirtualMachine types.  A couple of TODOs depend on changes to vcsim, I will follow up on those.

**Release note**:

```release-note
NONE
```
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
b40581c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment