Do not crash when ip is not found
ClosedPublic
Actions

Authored by mkrizek on Feb 2 2016, 12:50 PM.

Details

Reviewers

Maniphest Tasks

T698: Execution crashes when connecting to a minion

Summary

This will at least prevent from the mysterious TypeError. We still need to investigate why it's failing to find ip. So far, this has been happening only on qa12 (one of the prod host clients).

Test Plan

None really :/

Diff Detail

Lint

Lint Skipped

Unit

Unit Tests Skipped

mkrizek retitled this revision from to Do not crash when ip is not found.Feb 2 2016, 12:50 PM

mkrizek updated this object.

mkrizek edited the test plan for this revision. (Show Details)

mkrizek added a reviewer: tflink.

mkrizek set the repository for this revision to rTCLOUD testcloud.

mkrizek added a project: testcloud.

mkrizek added a task: T698: Execution crashes when connecting to a minion.

The approach loks good to me but if you're changing that function, can you add a try/catch with error around the methods which call it in testcloud/cli.py?

Also, a small nitpick on the exact wording of the exception message thrown

testcloud/cli.py
337	I'd also like to see the timeout referenced in the essage - something along the lines of "Could not find VM\'s ip before timeout"

This revision now requires changes to proceed.Feb 2 2016, 1:24 PM

In D737#13989, @tflink wrote:

The approach loks good to me but if you're changing that function, can you add a try/catch with error around the methods which call it in testcloud/cli.py?

Don't we just want to raise the exception? The message of the exception is clear on what happened. Looking at the code, it's done like that (just `raise) in the whole cli.py. We don't catch DomainNotFoundError that find_vm_ip` raises either. Or am I misunderstanding?

Good point on mentioning the timeout in the message though.

In D737#13994, @mkrizek wrote:

In D737#13989, @tflink wrote:

The approach loks good to me but if you're changing that function, can you add a try/catch with error around the methods which call it in testcloud/cli.py?

Don't we just want to raise the exception? The message of the exception is clear on what happened. Looking at the code, it's done like that (just `raise) in the whole cli.py. We don't catch DomainNotFoundError that find_vm_ip` raises either. Or am I misunderstanding?

You're right - there's no point in deviating from what was already there.

Good point on mentioning the timeout in the message though.

That would be nice, though

mkrizek updated this revision to Diff 1891.Feb 3 2016, 8:31 AM

mkrizek removed rTCLOUD testcloud as the repository for this revision.

tflink accepted this revision.Feb 3 2016, 8:52 AM

This revision is now accepted and ready to land.Feb 3 2016, 8:52 AM

Pull request with this patch sent to testcloud github. Closing.

Revision Contents

			Path
M			testcloud/cli.py (4 lines)

Diff 1891

View Options

testcloud/cli.py

Context not available.
16	from . import image	16		from . import image
17	from . import instance	17		from . import instance
18	from . import util	18		from . import util
19	from .exceptions import DomainNotFoundError, TestcloudCliError	19		from .exceptions import DomainNotFoundError, TestcloudCliError, TestcloudInstanceError
20		20
21	config_data = config.get_config()	21		config_data = config.get_config()
22		22
Context not available.
333	break	333		break
334		334
335	sleep(.2)	335		sleep(.2)
		336		else:
		337		raise TestcloudInstanceError('Could not find VM\'s ip before timeout')
			tflinkUnsubmitted Not Done I'd also like to see the timeout referenced in the essage - something along the lines of "Could not find VM\'s ip before timeout"
336		338
337	return vm_ip	339		return vm_ip
Context not available.

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	1890		Feb 2 2016, 12:48 PM	★	★
Diff 2	1891		Feb 3 2016, 8:31 AM	★	★