update fetch_externally_routable_ips to use any network device #314
Conversation
|
|
|
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. |
|
This LGTM, but I fired off a run that previously failed to be sure. I'll approve once it's done. |
|
There's some conflicts with requirements btw. |
imcdo
force-pushed
the
use_any_network_device
branch
from
bc8c9cd
to
5bfb59c
Compare
There was a problem hiding this comment.
Seeing how we ran into issues with other changes in the past, I'm not convinced we want such changes in 0.7 or 0.8 tbh.
Like we're deprecating something so that warrants a version change. 0.9 is probably ok, but might be 0.10?
We can discuss this offline too.
| if not is_aws: | ||
| raise NotImplementedError("Windows is only supported in AWS.") | ||
|
|
||
| def fetch_externally_routable_ip(self, is_aws=None): |
There was a problem hiding this comment.
So do we support windows in aws now?
There was a problem hiding this comment.
in the past we only supported windows in aws for vagrant clusters, whats not made clear is this code path is only executed from the vagrant cluster file, so its not a great representation of whats actually supported. This pr makes things a bit more generic.
|
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 |
|
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. |
|
@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 |
…uentinc#314) * update fetch_externally_routable_ips * pr comments * update-vagrant * spelling fix * fix tests * make network device methods available and also fix test * pin xmltodict * remove deprecation
…uentinc#314) * update fetch_externally_routable_ips * pr comments * update-vagrant * spelling fix * fix tests * make network device methods available and also fix test * pin xmltodict * remove deprecation
instead of expecting a explicit network device, use a generic one based on the devices found on the remote machine.