search

confluentinc / ducktape

System integration and performance tests
11 stars 93 forks source link

update fetch_externally_routable_ips to use any network device #314

Closed imcdo closed 2 years ago

imcdo commented 2 years ago

instead of expecting a explicit network device, use a generic one based on the devices found on the remote machine.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

stan-is-hate commented 2 years ago

A more meta question is this - what specific issue is this resolving and do we want it in all branches or maybe make it 0.9.x only?

imcdo commented 2 years ago

A more meta question is this - what specific issue is this resolving and do we want it in all branches or maybe make it 0.9.x only?

seems that some people use this in their test, and when changing ec2 instance types/base amis the network device name changes and is rarely actually these two devices. Realistically people should only use the externally routable ip that is set in their cluster (hence the deprecation) but this fix is for people with current tests that use it and who are trying to upgrade their hardware.

lbradstreet commented 2 years ago

This LGTM, but I fired off a run that previously failed to be sure. I'll approve once it's done.

lbradstreet commented 2 years ago

There's some conflicts with requirements btw.

imcdo commented 2 years ago

the expectation before had tied the hardware to vagrant clusters, and this pr unties them, so I believe its worth wile to merge this in old branches as imo ducktape shouldn't make any assumptions about hardware in any version

stan-is-hate commented 2 years ago

I totally support the motivation behind this change, ducktape shouldn't make assumptions about hardware indeed. However, I don't want us making potentially breaking changes in the patch versions, especially since ducktape is used in other projects outside of confluent as well. I may be wrong here, of course, so let's continue this conversation offline.

lbradstreet commented 2 years ago

@imcdo I tested this change with my branch where the IP discovery and interface discovery were previously failing and it works well and fixes the issues. For some of the tests where I need to detect the default interface, I ended up doing node.account.get_external_accessible_network_devices()[0] which is slightly awkward but works fine.