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.

Sign up for GitHub

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

Jump to bottom

Bug 1979038: oVirt: include master IPs when running must gather #5039

Merged

openshift-merge-robot merged 1 commit into openshift:master from eslutsky:gather_master_data

Jul 8, 2021

Contributor

eslutsky commented Jun 29, 2021

gather bootstrap command will collect logs from the VMs only if the boostrap VM still exists.
this change adds the control planes VMs IP information, resulting the master logs
to be collected as well.

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

openshift-ci bot added the do-not-merge/work-in-progress label

openshift-ci bot requested review from e-tienne and patrickdillon

June 29, 2021 08:25

eslutsky mentioned this pull request

ovirt: split terraform into stages #5034

Merged

eslutsky force-pushed the gather_master_data branch 8 times, most recently from 37a43c9 to 5c4f71d Compare

June 29, 2021 11:23

ghost suggested changes

View reviewed changes

pkg/terraform/gather/ovirt/ip.go Outdated


		reportedDeviceSlice, err := getReportedDevices(c, moRefValue)
		client, err := ovirt.NewConnection()

ghost Jun 29, 2021

Why are we creating a new oVirt connection here?

Contributor Author

eslutsky Jun 29, 2021

moved to the connection one level up (outside the for loop)

pkg/terraform/gather/ovirt/ip.go

               				ipres, hasAddress := ip.Address()
               				if hasAddress {
               					if checkPortIsOpen(ipres, bootstrapSSHPort) {
-              						logrus.Debugf("ovirt vm id: %s , found usable bootstrap IP %s", moRefValue, ipres)
+              						logrus.Debugf("ovirt vm id: %s , found usable  IP Address: %s", moRefValue, ipres)

ghost Jun 29, 2021

Is this log useful to the user anyhow or is this just debug code?

Contributor Author

eslutsky Jun 29, 2021

its for debug

pkg/terraform/gather/ovirt/ip.go Outdated

+              	defer client.Close()
+              	reportedDeviceSlice, err := getReportedDevices(client, moRefValue)
               	if err != nil {
               		return "", err

ghost Jun 29, 2021

This error should possibly be wrapped so we have context.

Contributor Author

eslutsky Jun 29, 2021

done

pkg/terraform/gather/ovirt/ip.go Outdated

    
              	}

              	if len(br.Instances) == 0 {

              		return "", errors.New("no bootstrap instance found")

              		return []string{""}, errors.New(fmt.Sprintf("no %s instance found",name))

ghost Jun 29, 2021

Returning an empty IP seems like a workaround for something. It seems that this should not be handled here. If yes, we should add comments explaining what this does and why it is ok to return an empty string IP.

Contributor Author

eslutsky Jun 29, 2021

okay , in case of error we can return nil instead

Contributor Author

eslutsky Jun 29, 2021

done

pkg/terraform/gather/ovirt/ip.go

+              // BootstrapIP returns the ip address for bootstrap host.
+              func BootstrapIP(tfs *terraform.State) (string, error) {
+              	ips, err := lookupVMResources(tfs, "module.bootstrap", "bootstrap")
+              	return ips[0], err

ghost Jun 29, 2021

Add error handling here if ips has no entries.

ghost Jun 29, 2021

After the last update this will cause a panic.

Contributor Author

eslutsky Jun 29, 2021

done

pkg/terraform/gather/ovirt/ip.go Outdated

+              		}
+              		ip, err := findVirtualMachineIP(vmid)
+              		if err != nil {
+              			return []string{""}, errors.Wrapf(err, "failed to lookup %s ipv4 address",name)

ghost Jun 29, 2021

Same as above.

Contributor Author

eslutsky Jun 29, 2021

done

eslutsky force-pushed the gather_master_data branch 3 times, most recently from 75cfb4e to a112ffe Compare

June 29, 2021 12:47

ghost suggested changes

View reviewed changes

pkg/terraform/gather/ovirt/ip.go Outdated

-              func findVirtualMachineIP(c *ovirtsdk4.Connection, moRefValue string) (string, error) {
-              	reportedDeviceSlice, err := getReportedDevices(c, moRefValue)
+              func findVirtualMachineIP(moRefValue string,client *ovirtsdk4.Connection) (string, error) {

ghost Jun 29, 2021

This change seems unnecessary. Not a big deal, but still.

pkg/terraform/gather/ovirt/ip.go

+              // BootstrapIP returns the ip address for bootstrap host.
+              func BootstrapIP(tfs *terraform.State) (string, error) {
+              	ips, err := lookupVMResources(tfs, "module.bootstrap", "bootstrap")
+              	return ips[0], err

ghost Jun 29, 2021

After the last update this will cause a panic.

eslutsky force-pushed the gather_master_data branch 5 times, most recently from 16663ca to f370157 Compare

June 29, 2021 17:41


          oVirt: include master IPs when running must gather

d19e8c6

`gather bootstrap` command will collect logs from the VMs only if the boostrap VM still exists.
this change adds the control planes VMs IP information, resulting the master logs
to be collected as well.

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

eslutsky force-pushed the gather_master_data branch from f370157 to d19e8c6 Compare

June 29, 2021 17:49

eslutsky requested a review from a user

June 29, 2021 17:51

ghost approved these changes

View reviewed changes

eslutsky changed the title ~~WIP: oVirt: include master IPs when running must gather~~ oVirt: include master IPs when running must gather

openshift-ci bot removed the do-not-merge/work-in-progress label

Contributor Author

eslutsky commented Jul 4, 2021

/assign @crawford

openshift-ci bot assigned crawford

eslutsky changed the title ~~oVirt: include master IPs when running must gather~~ Bug 1979038: oVirt: include master IPs when running must gather

openshift-ci bot added the bugzilla/severity-medium label

Contributor

openshift-ci bot commented Jul 4, 2021

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

3 validation(s) were run on this bug

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

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (mburman@redhat.com), skipping review request.

In response to this:

Bug 1979038: oVirt: include master IPs when running must gather

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 bot added the bugzilla/valid-bug label

Contributor

Gal-Zaidman commented Jul 5, 2021

/assign @staebler

Required for #5034

openshift-ci bot assigned staebler

Member

jhixson74 commented Jul 8, 2021

/lgtm
/approve

openshift-ci bot assigned jhixson74

openshift-ci bot added the lgtm label

Contributor

openshift-ci bot commented Jul 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhixson74

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~~ [jhixson74]

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

openshift-ci bot added the approved label

Contributor

openshift-bot commented Jul 8, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments

Contributor

openshift-bot commented Jul 8, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jul 8, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jul 8, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jul 8, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Jul 8, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-ci bot commented Jul 8, 2021

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-crc	`d19e8c6`	link	`/test e2e-crc`

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.

Contributor

openshift-bot commented Jul 8, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot merged commit fe9a17d into openshift:master

Contributor

openshift-ci bot commented Jul 8, 2021

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

openshift/installer#5039

Bugzilla bug 1979038 has been moved to the MODIFIED state.

In response to this:

Bug 1979038: oVirt: include master IPs when running must gather

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment