-
Notifications
You must be signed in to change notification settings - Fork 112
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.
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
[6.2.z] refactored vm.py #4412
[6.2.z] refactored vm.py #4412
Conversation
@rplevka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbidarkar, @abalakh and @oshtaier to be potential reviewers. |
robottelo/vm.py
Outdated
result = ssh.command( | ||
u'ping -c 1 {0}.local'.format(self._target_image), | ||
u'for i in {{1..60}}; do ping -c1 {0}.local && break; sleep 1;' | ||
u' done; echo $?'.format(self._target_image), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will move the timeout logic onto the hypervisor itself, so it keeps pinging the host for availability until ready or timeout passes. This speeds up things a bit since we don't wait a constant time (full minute) everytime. After testing, the host typically boots up whithin ~30seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good that we may save time here, but with my past experience at times certain services take more time to pass by for it to be able to work properly. so I feel let's keep a watch if client tests start failing for weird reasons.
For now I think we can give a try.
Update: Also I feel this could be a problem with RHEL7 as with systemd, services get started in parallel, so services which are waiting for/on network to be availble , may fail to work properly as they are still trying to come up. Also we may be waiting here for very short period instead of the previous 60 secs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could help may be, if we face any issue. https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kbidarkar I was thinking about trying to ssh in a loop instead of pinging the host. That would ensure we cut the loop after we are really able to connect to tcp/22.
your other comment is an interesting read, however i would avoid this solution if we can, as it involves changing the settings on the virtual host, which would require changes to the base image. We would loose flexibility with this as we might decide to dynamically deploy systems with various system, not necessarily using pre-built base images, etc.
what do you think?
@@ -291,7 +289,8 @@ def register_contenthost(self, org, activation_key=None, lce=None, | |||
if force: | |||
cmd += u' --force' | |||
result = self.run(cmd) | |||
if result.return_code == 0: | |||
if (u'The system has been registered with ID' in | |||
u''.join(result.stdout)): | |||
self._subscribed = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part has already existed, so why not use it.
@@ -558,11 +558,12 @@ def test_positive_usage_limit(self): | |||
vm1.install_katello_ca() | |||
result = vm1.register_contenthost( | |||
self.org['label'], new_ak['name']) | |||
self.assertEqual(result.return_code, 0) | |||
self.assertTrue(vm1._subscribed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the _subscribed
attribute since it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rplevka , you are accessing a private attr instead of doing so and as you are refactoring VirtualMachine you can add a property in VirtualMachine class:
class VirtualMachine(object):
...
@property
def subscribed(self):
return self._subscribed
and use it as:
...
self.assertTrue(vm1.subscribed)
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ldjebran, great point. Will do that.
APT |
42cc9db
to
351f024
Compare
32b5ded
to
33fb6b5
Compare
self.provisioning_server | ||
) | ||
if result.return_code != 0: | ||
raise VirtualMachineError( | ||
'Failed to fetch virtual machine IP address information') | ||
output = ''.join(result.stdout) | ||
self.ip_addr = output.split('(')[1].split(')')[0] | ||
ssh_check = ssh.command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will continuously check for ssh availability
@kbidarkar could you review this once again? |
@rplevka travis CI is failing test_vm |
@rochacbruno just fixed it, let's wait for the re-run |
fixed cli.activation_key test cases - updated the assertions
0f5da54
to
e630032
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
partly addresses #4153