Skip to content

Conversation

@mrnold
Copy link
Contributor

@mrnold mrnold commented Feb 14, 2024

Add a test to create and start a virtual machine.

Signed-off-by: Matthew Arnold <marnold@redhat.com>
This is needed on cloud clusters that don't use host virtualization or
nested virtualization. Assume it is needed in CI.

Signed-off-by: Matthew Arnold <marnold@redhat.com>
Also minor factoring of data volume creation, and add a place for the VM
creation test.

Signed-off-by: Matthew Arnold <marnold@redhat.com>
This clones the CirrOS data volume and attaches it to a new VM, then
cleans everything up once it goes running.

Signed-off-by: Matthew Arnold <marnold@redhat.com>
@openshift-ci openshift-ci bot requested review from hhpatel14 and kaovilai February 14, 2024 02:43
return health == "healthy"
}

// Check if KVM emulation is enabled.

Choose a reason for hiding this comment

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

what is KVM?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's referring to this qemu KVM . Read more at wiki

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KVM here means kernel virtual machine, which is a kernel feature that accelerates virtual machines using the virtualization extensions provided by the CPU. Most cloud clusters do not have direct access to the CPU, and can't use KVM. So this pull request turns on an emulation mode that is much slower, but will work on our CI instances.

I took this out by mistake in a previous commit.

Signed-off-by: Matthew Arnold <marnold@redhat.com>
return false
}
if !ok {
log.Printf("No KVM emulation annotation (%s) listed on HCO!", emulationAnnotation)

Choose a reason for hiding this comment

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

should return false here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It will as it will hit line no 248.

err = v.EnsureEmulation(10 * time.Second)
Expect(err).To(BeNil())

err = v.EnsureDataVolume("openshift-cnv", "cirros-dv", "https://download.cirros-cloud.net/0.6.2/cirros-0.6.2-x86_64-disk.img", "128Mi", 5*time.Minute)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do something like?

//"io/ioutil"
//"net/http"
//"strings"
//"fmt"
func getLatestCirrosImageURL() (string, error) {
    cirrosVersionURL := "https://download.cirros-cloud.net/version/released"
    
    resp, err := http.Get(cirrosVersionURL)
    if err != nil {
        return "", err
    }
    defer resp.Body.Close()
    
    body, err := ioutil.ReadAll(resp.Body)
    if err != nil {
        return "", err
    }
    
    latestCirrosVersion := strings.TrimSpace(string(body))
    
    imageURL := fmt.Sprintf("https://download.cirros-cloud.net/%s/cirros-%s-x86_64-disk.img", latestCirrosVersion, latestCirrosVersion)
    
    return imageURL, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, do you want to open a pull request to https://github.com/mrnold/oadp-operator/tree/ci-create-vm? Or should I just copy and paste this?

Copy link
Contributor

Choose a reason for hiding this comment

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

copy-paste is fine

Copy link
Contributor

@mpryc mpryc left a comment

Choose a reason for hiding this comment

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

/lgtm

Feel free to update if you like the idea of getting latest cirros img.

return nil
}

func (v *VirtOperator) ensureVm(namespace, name, source string, timeout time.Duration) error {

Choose a reason for hiding this comment

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

here the logic is right? It will check if VM exists (without checking status), but if it fails, then it will check status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not, it doesn't look like I thought this through. The existence check is really there to speed up development of the CI test itself, since I don't have it tear down and recreate everything from scratch on every run. The status check polling is for the newly created VM. I can probably take out the existence check, since Kubernetes will complain if the VM is already there when the test tries to create it.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2024
Signed-off-by: Matthew Arnold <marnold@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2024
Copy link

@mateusoliveira43 mateusoliveira43 left a comment

Choose a reason for hiding this comment

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

From tests log, it is working as expected 😄

@mpryc
Copy link
Contributor

mpryc commented Feb 15, 2024

/lgtm

The only test which we are interested in this PR is around kubevirt-aws, which passes. The code does not have any further comments from my side:

$ demystifier https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_oadp-operator/1330/pull-ci-openshift-oadp-operator-master-4.14-e2e-test-kubevirt-aws/1757841604539846656
INFO[0000] Test Demystifier starts its journey           >>> start_demystifier_timestamp=1707998188
INFO[0000] Using log from                                >>> location="https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_oadp-operator/1330/pull-ci-openshift-oadp-operator-master-4.14-e2e-test-kubevirt-aws/1757841604539846656/artifacts/e2e-test-kubevirt-aws/e2e/build-log.txt"
Test Summary Table:
---------------------------------------------------------------------------------------------------
| Test Name                                | Num Attempts    | Num Failed  | Average Run Time     |
---------------------------------------------------------------------------------------------------
| should verify virt installation          | 1               | 0           | 0s                   |
| should create and boot a virtual machine | 1               | 0           | 1m46.047s            |
---------------------------------------------------------------------------------------------------
INFO[0000] Test Demystifier finishes its journey         >>> end_demystifier_timestamp=1707998189

@mpryc
Copy link
Contributor

mpryc commented Feb 15, 2024

/retest

We can override the failing ones, but maybe this time will pass as those are unknown flakes.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2024
@kaovilai
Copy link
Member

/override ci/prow/4.12-e2e-test-azure
/override ci/prow/4.13-e2e-test-azure
/override ci/prow/4.14-e2e-test-azure
per @weshayutin request.

@openshift-ci
Copy link

openshift-ci bot commented Feb 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43, mpryc, mrnold, weshayutin

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2024
@openshift-ci
Copy link

openshift-ci bot commented Feb 15, 2024

@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/4.12-e2e-test-azure, ci/prow/4.13-e2e-test-azure, ci/prow/4.14-e2e-test-azure

In response to this:

/override ci/prow/4.12-e2e-test-azure
/override ci/prow/4.13-e2e-test-azure
/override ci/prow/4.14-e2e-test-azure
per @weshayutin request.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kaovilai
Copy link
Member

/override ci/prow/4.12-e2e-test-azure
/override ci/prow/4.13-e2e-test-azure

@openshift-ci
Copy link

openshift-ci bot commented Feb 15, 2024

@kaovilai: Cannot get commit statuses for PR #1330 in openshift/oadp-operator

In response to this:

/override ci/prow/4.12-e2e-test-azure
/override ci/prow/4.13-e2e-test-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@weshayutin weshayutin merged commit 0cbd8db into openshift:master Feb 15, 2024
mrnold added a commit to mrnold/oadp-operator that referenced this pull request Jun 3, 2024
)

* Remove unused timeouts.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

* Add ability to enable KVM emulation.

This is needed on cloud clusters that don't use host virtualization or
nested virtualization. Assume it is needed in CI.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

* Clone base data volume before creating a VM.

Also minor factoring of data volume creation, and add a place for the VM
creation test.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

* Add a test to start a virtual machine.

This clones the CirrOS data volume and attaches it to a new VM, then
cleans everything up once it goes running.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

* Clean up base CirrOS disk before removing HCO.

I took this out by mistake in a previous commit.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

* Address some review comments.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

---------

Signed-off-by: Matthew Arnold <marnold@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. CI lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants