-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
[fix] Fixed issues in reconnecting to device after firmware upgrade #235 #236
[fix] Fixed issues in reconnecting to device after firmware upgrade #235 #236
Conversation
705895d
to
0d1b05b
Compare
The test is misssing. Fixes #235
71458ed
to
5844f3e
Compare
5844f3e
to
82ade68
Compare
self._refresh_addresses() | ||
addresses = ', '.join(self.addresses) | ||
self.log( | ||
_( | ||
'Trying to reconnect to device at {addresses} (attempt n.{attempt})...'.format( | ||
addresses=addresses, attempt=attempt | ||
) | ||
), | ||
save=False, | ||
) | ||
self.connect() |
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.
While performing manual testing locally, I got this log for the device
Connection successful, starting upgrade...
Image checksum file not found, proceeding with the upload of the new image...
Sysupgrade test passed successfully, proceeding with the upgrade operation...
Upgrade operation in progress...
Commencing upgrade. Closing all shell sessions.
Image metadata not found
Reading partition table from bootdisk...
Reading partition table from image...
Partition layout has changed. Full image will be written.
SSH connection closed, will wait 150 seconds before attempting to reconnect...
Device not reachable yet, (connection failed).
retrying in 30 seconds...
Device not reachable yet, (connection failed).
retrying in 30 seconds...
Trying to reconnect to device at 192.168.56.2 (attempt n.3)...
Connected! Writing checksum file to /etc/openwisp/firmware_checksum
Upgrade completed successfully.
See Trying to reconnect to device at 192.168.56.2 (attempt n.3)...
shows third attempt. This is because in previous two attempts the device was unreachable and self._refresh_addresses()
raised NoWorkingDeviceConnectionError
which led to the logs getting skipped.
How can we get updated addresses without initiating a connection?
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.
what about:
```python
def _log(addresses, attempt):
self.log(
_(
f'Trying to reconnect to device at {addresses} '
'(attempt n.{attempt})...'.format(
addresses=addresses,
attempt=attempt
)
),
save=False,
)
try:
self._refresh_addresses()
except NoWorkingDeviceConnectionError as error:
_log(error.connection.addresses, attempt)
try:
_log(', '.join(self.addresses), attempt)
self.connect()
except (NoValidConnectionsError, socket.timeout, SSHException) as error:
# etc...
I also tested #233 locally and this code mitigates that problem. |
) | ||
), | ||
save=False, | ||
) | ||
self.connect() |
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.
shouldn't we update this too?
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.
I added the following wrapper methods so we don't need to reference the connection object again and again
openwisp-firmware-upgrader/openwisp_firmware_upgrader/upgraders/openwrt.py
Lines 123 to 130 in 82ade68
def connect(self): | |
return self.connection.connect() | |
def disconnect(self): | |
return self.connection.disconnect() | |
def exec_command(self, *args, **kwargs): | |
return self.connection.connector_instance.exec_command(*args, **kwargs) |
self._refresh_addresses() | ||
addresses = ', '.join(self.addresses) | ||
self.log( | ||
_( | ||
'Trying to reconnect to device at {addresses} (attempt n.{attempt})...'.format( | ||
addresses=addresses, attempt=attempt | ||
) | ||
), | ||
save=False, | ||
) | ||
self.connect() |
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.
what about:
```python
def _log(addresses, attempt):
self.log(
_(
f'Trying to reconnect to device at {addresses} '
'(attempt n.{attempt})...'.format(
addresses=addresses,
attempt=attempt
)
),
save=False,
)
try:
self._refresh_addresses()
except NoWorkingDeviceConnectionError as error:
_log(error.connection.addresses, attempt)
try:
_log(', '.join(self.addresses), attempt)
self.connect()
except (NoValidConnectionsError, socket.timeout, SSHException) as error:
# etc...
try: | ||
self.connect() |
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.
We do not need to perform the connect operation here. self.connection.get_working_connection
will open the SSH connection.
try: | ||
self.connect() | ||
except (NoValidConnectionsError, socket.timeout, SSHException) as error: |
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.
We no longer need to catch these exceptions here. These are handled by DeviceConnection model
class OpenWrt(BaseOpenWrt): | ||
class OpenWrt(object): |
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.
@nemesisdesign since we are not directly backporting these changes to 1.0 branch, we can make this change of not making upgraders inherit SSH connectors.
The test is misssing.
Fixes #235
This may fix also #233 (needs manual testing to confirm).