Bug 1810438: oVirt: Add missing piece for gathering bootstrap IP info #4524

eslutsky · 2021-01-07T07:45:39Z

during gather-bootstrap logs need to be gathered from the bootstrap node using ssh,
oVirt engine collects node IP information using the qemu-agent,
we are using oVirt SDK to retrieve this information and choose the correct IP which allows ssh connection.

Signed-off-by: Evgeny Slutsky eslutsky@redhat.com

eslutsky · 2021-01-07T07:48:36Z

pkg/terraform/gather/ovirt/ip.go

+		return "", errors.Errorf("failed to lookup bootstrap managed object reference")
+	}
+
+	ip, err := findVirtualMachineIP(client, vmid,"192.168.0.0/16")


this line is temp for testing , we need to decide about it ..

we can examine the api_vip from the config and prefer the choose the bootstrap address in the same network.

this is very similar to what is done for choosing the node-ip: https://github.com/openshift/baremetal-runtimecfg/blob/5ba9497f109f6d3170ecc7f0f879e93090cf8eda/pkg/utils/addresses.go#L160-L165

dougsland

Looks good but I have some suggestions.

pkg/terraform/gather/ovirt/ip.go

dougsland · 2021-01-11T15:03:11Z

pkg/terraform/gather/ovirt/ip.go

+			}
+		}
+	}
+


IMO, from line 44 until 73 deserve to be a helper function.

I would do 44-58 getreportedDevices(...) and leave the rest in the main function "findVirtualMachineIP"

pkg/terraform/gather/ovirt/ip.go

dougsland · 2021-01-11T15:05:34Z

pkg/terraform/gather/ovirt/ip.go

+	br, err := terraform.LookupResource(tfs, "module.bootstrap", "ovirt_vm", "bootstrap")
+
+	if err != nil {
+		return "", errors.Wrap(err, "failed to lookup bootstrap")


Can we have more details about the vm? Like vmname or vmID? The same apply to the messages bellow.

in this stage we dont have the vmID

I think bootstrap is bootstrap, remember that it will only be called if the bootstrap phase of the install failed :)

Gal-Zaidman

Code looks good

pkg/terraform/gather/ovirt/ip.go

Gal-Zaidman · 2021-01-13T14:40:58Z

pkg/terraform/gather/ovirt/ip.go

+			}
+		}
+	}
+


I would do 44-58 getreportedDevices(...) and leave the rest in the main function "findVirtualMachineIP"

Gal-Zaidman · 2021-01-13T14:43:58Z

pkg/terraform/gather/ovirt/ip.go

+	br, err := terraform.LookupResource(tfs, "module.bootstrap", "ovirt_vm", "bootstrap")
+
+	if err != nil {
+		return "", errors.Wrap(err, "failed to lookup bootstrap")


I think bootstrap is bootstrap, remember that it will only be called if the bootstrap phase of the install failed :)

Gal-Zaidman · 2021-01-13T14:54:09Z

pkg/terraform/gather/ovirt/ip.go

+
+func findVirtualMachineIP(c *ovirtsdk4.Connection,moRefValue string) (string, error) {
+
+	vmsService := c.SystemService().VmsService()


I think tou can do it in one shot with:
vmResp , err := c.SystemService().VmsService().VmService(moRefValue).Get().Send()
if err != nil {
return "",fmt.Errorf("Failed to find VM, by id %v, reason: %v\n",moRefValue, err)
}
VM := vmResp.MustVm()

Gal-Zaidman · 2021-01-19T08:41:38Z

/test e2e-ovirt

Gal-Zaidman · 2021-01-19T09:52:55Z

/test e2e-ovirt

openshift-ci-robot · 2021-01-19T13:15:57Z

@eslutsky: This pull request references Bugzilla bug 1810438, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1810438: oVirt: Add missing piece for gathering bootstrap IP info

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.

openshift-ci-robot · 2021-01-19T13:20:02Z

@eslutsky: This pull request references Bugzilla bug 1810438, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1810438: oVirt: Add missing piece for gathering bootstrap IP info

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.

Gal-Zaidman · 2021-01-19T13:24:46Z

pkg/terraform/gather/ovirt/ip.go

+	}
+
+	return reportedDeviceSlice, nil
+


remove extra line :)

pkg/terraform/gather/ovirt/ip.go

Gal-Zaidman · 2021-01-19T13:25:58Z

pkg/terraform/gather/ovirt/ip.go

+			for _, ip := range ips.Slice() {
+				ipres := ip.MustAddress()
+				if ipres != "" {
+					if checkPortIsOpen(ipres, "22") {


Should we use a const var?

Gal-Zaidman · 2021-01-19T13:30:49Z

pkg/terraform/gather/ovirt/ip.go

+	}
+	defer client.Close()
+
+	br, err := terraform.LookupResource(tfs, "module.bootstrap", "ovirt_vm", "bootstrap")


Why not look for the bootstrap VM in ovirt without terraform? less deps == less problems :)

You can use the same logic as https://github.com/openshift/cluster-api-provider-ovirt/blob/master/pkg/cloud/ovirt/clients/machineservice.go#L275-L291

I think since we operate here externally to the cluster using the id is more reliable, plus it's similar to what other vendors are doing [1].

[1]

installer/pkg/terraform/gather/vsphere/ip.go

Line 58 in 644a93e

moid, found, err := unstructured.NestedString(br.Instances[0].Attributes, "moid")

Gal-Zaidman · 2021-01-20T08:10:44Z

/lgtm

Gal-Zaidman · 2021-01-20T08:11:09Z

/assign @abhinavdahiya @staebler

staebler

Mostly just nits.

pkg/terraform/gather/ovirt/ip.go

Gal-Zaidman · 2021-01-25T08:45:00Z

/lgtm

eslutsky · 2021-01-25T13:49:37Z

/retest

staebler

One typo. Otherwise it looks good.
/lgtm cancel
/approve

staebler · 2021-01-25T16:05:48Z

pkg/terraform/gather/ovirt/ip.go

+			}
+		}
+	}
+	return "", fmt.Errorf("coudlnt find usable bootstrap IP address for vm id: %s", moRefValue)


Suggested change

return "", fmt.Errorf("coudlnt find usable bootstrap IP address for vm id: %s", moRefValue)

return "", fmt.Errorf("could not find usable bootstrap IP address for vm id: %s", moRefValue)

openshift-ci-robot · 2021-01-25T16:08:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

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:

~~OWNERS~~ [staebler]

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

assuming that the engine already got the ip from the qemu agent. Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>

openshift-ci · 2021-01-26T09:38:25Z

@eslutsky: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-workers-rhel7	`0b6e8ff`	link	`/test e2e-aws-workers-rhel7`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Gal-Zaidman · 2021-01-26T10:10:00Z

/lgtm

openshift-ci-robot · 2021-01-26T10:17:06Z

@eslutsky: All pull requests linked via external trackers have merged:

openshift/installer#4524

Bugzilla bug 1810438 has been moved to the MODIFIED state.

In response to this:

Bug 1810438: oVirt: Add missing piece for gathering bootstrap IP info

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.

eslutsky changed the title ~~oVirt: Add missing piece for gathering bootsrap IP info~~ WIP: oVirt: Add missing piece for gathering bootstrap IP info Jan 7, 2021

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 7, 2021

openshift-ci-robot requested review from mtnbikenc and patrickdillon January 7, 2021 07:46

eslutsky commented Jan 7, 2021

View reviewed changes

eslutsky force-pushed the bootstrap_ip branch 5 times, most recently from 528ddac to 72bf1c3 Compare January 11, 2021 14:12

dougsland reviewed Jan 11, 2021

View reviewed changes

openshift-ci-robot assigned Gal-Zaidman Jan 13, 2021

eslutsky force-pushed the bootstrap_ip branch from 72bf1c3 to 58b7367 Compare January 13, 2021 10:37

Gal-Zaidman reviewed Jan 13, 2021

View reviewed changes

eslutsky force-pushed the bootstrap_ip branch 3 times, most recently from 66eaf1b to 2aaf9b3 Compare January 18, 2021 19:57

eslutsky requested a review from Gal-Zaidman January 18, 2021 19:59

eslutsky force-pushed the bootstrap_ip branch from 2aaf9b3 to 91cb66a Compare January 19, 2021 10:03

eslutsky changed the title ~~WIP: oVirt: Add missing piece for gathering bootstrap IP info~~ Bug 1810438: oVirt: Add missing piece for gathering bootstrap IP info Jan 19, 2021

openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 19, 2021

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 19, 2021

Gal-Zaidman reviewed Jan 19, 2021

View reviewed changes

pkg/terraform/gather/ovirt/ip.go Outdated Show resolved Hide resolved

Gal-Zaidman reviewed Jan 19, 2021

View reviewed changes

eslutsky force-pushed the bootstrap_ip branch from 91cb66a to b719f0f Compare January 20, 2021 07:44

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021

openshift-ci-robot assigned abhinavdahiya and staebler Jan 20, 2021

staebler reviewed Jan 20, 2021

View reviewed changes

eslutsky force-pushed the bootstrap_ip branch from b719f0f to f428896 Compare January 24, 2021 09:09

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2021

eslutsky force-pushed the bootstrap_ip branch 3 times, most recently from 92035a3 to bbb8417 Compare January 25, 2021 08:22

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2021

eslutsky requested a review from staebler January 25, 2021 08:59

staebler reviewed Jan 25, 2021

View reviewed changes

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2021

oVirt: Add missing piece for gathering bootsrap IP info

0b6e8ff

assuming that the engine already got the ip from the qemu agent. Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>

eslutsky force-pushed the bootstrap_ip branch from bbb8417 to 0b6e8ff Compare January 26, 2021 08:13

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2021

openshift-merge-robot merged commit 96de17b into openshift:master Jan 26, 2021


		func findVirtualMachineIP(c *ovirtsdk4.Connection,moRefValue string) (string, error) {

		vmsService := c.SystemService().VmsService()

	return "", fmt.Errorf("coudlnt find usable bootstrap IP address for vm id: %s", moRefValue)
	return "", fmt.Errorf("could not find usable bootstrap IP address for vm id: %s", moRefValue)

Bug 1810438: oVirt: Add missing piece for gathering bootstrap IP info #4524

Bug 1810438: oVirt: Add missing piece for gathering bootstrap IP info #4524

Conversation

eslutsky commented Jan 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougsland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gal-Zaidman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gal-Zaidman commented Jan 19, 2021

Gal-Zaidman commented Jan 19, 2021

openshift-ci-robot commented Jan 19, 2021

openshift-ci-robot commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gal-Zaidman commented Jan 20, 2021

Gal-Zaidman commented Jan 20, 2021

staebler left a comment

Choose a reason for hiding this comment

Gal-Zaidman commented Jan 25, 2021

eslutsky commented Jan 25, 2021

staebler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 25, 2021

openshift-ci bot commented Jan 26, 2021 • edited

Gal-Zaidman commented Jan 26, 2021

openshift-ci-robot commented Jan 26, 2021

eslutsky commented Jan 7, 2021 •

edited

openshift-ci bot commented Jan 26, 2021 •

edited