update connectivity error message #2817

lucasmoura · 2023-10-30T20:29:52Z

Why is this needed?

We want to make connectivity errors clearer and easier to debug for the user. To achieve that, we are now displaying the url that cause the error together with the cause behind the error.

Fixes: #2647

Test Steps

Launch a lxd container
Disable network on container with: lxc config device add NAME eth0 none
Run pro fix USN-4539-1

Expect the following message:

Failed to connect to https://ubuntu.com/security/notices/USN-4539-1.json
[Errno -3] Temporary failure in name resolution

Checklist

I have updated or added any unit tests accordingly
I have updated or added any integration tests accordingly
Changes here need to be documented, and this was done in:

Does this PR require extra reviews?

Yes
No

github-actions · 2023-10-30T20:30:08Z

🌎 This PR changes translatable messages. 🌏

Please select which scenarios apply. For further explanation, please read our policy on message changes.

New messages are being added.
- We will ask translators to take a look and add translations if they have time, but it will not block this PR.
Existing messages are being changed.
- ⚠️ Please add a comment with justification of why messages are being altered.
- If the changes are trivial (e.g. a typo fix), then translations must be preserved.
- If the changes are substantial, then we will ask translators to take a look and update translations if they have time, but it will not block this PR.
Existing messages are being deleted.
- No special action needed.

github-actions · 2023-10-30T20:30:09Z

Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference)

GitHub Issues:

Fixes: Contract server error message should be more specific #2647

Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references)

Documentation: The changes in this PR do not require documentation changes.

👍 this comment to confirm that this is correct.

lucasmoura · 2023-10-30T20:33:19Z

@orndorffgrant @renanrodrigo I am wondering if we should add the HTTP error code if available or if stating the cause is enough for debugging. Let me know what you think

orndorffgrant · 2023-10-31T09:53:59Z

I think the HTTP code doesn't hurt and could help

I'm tempted to suggest we combine UrlError and ConnectivityError into one exception class - but it'd be a little tricky to combine them. In order to maintain backwards compatibility, the new class would need to doubly inherit from UbuntuProError and IOError. And we'd need to keep both names pointing to the same class. What do you think?

lucasmoura · 2023-11-01T17:41:04Z

@orndorffgrant I think that is a good idea, since we only raise ConnectivyError exceptions when an UrlError has already happened. We would need to handle the MRO for the __init__ call, which I don't think is a huge problem. The only issue I see is that it will now take more parameters to instantiate a ConnectivityError exception, due to the IOErrorrequirements, which might affect backwards compatibility. But maybe I am overthinking this.

lucasmoura · 2023-11-08T10:02:56Z

We have decided not to add the HTTP response code on the message, since we only raise a ConnectivityError when the issue is not tied to an HTTP error

renanrodrigo · 2023-11-24T21:53:54Z

uaclient/exceptions.py

+        UbuntuProError.__init__(self, cause_error=cause_error, url=url)
+
+        # Even though we already set those variable through UbuntuProError
+        # we need to set them again to avoid mype warnings


Suggested change

# we need to set them again to avoid mype warnings

# we need to set them again to avoid mypy warnings

orndorffgrant · 2023-11-28T20:17:32Z

uaclient/contract.py

+        except exceptions.ConnectivityError as e:
+            LOG.exception(e.cause_error)
+            raise e


There are a couple places where we catch ConnectivityError, log the message, and then re-raise it. Maybe we should log the error when raising it originally in http/init.py and then remove all the catch-log-reraise blocks - what do you think?

That makes sense, I will update the PR

lucasmoura · 2023-12-15T15:04:54Z

@orndorffgrant updated

orndorffgrant

The tiniest of nits so pre-approving, no need for another review from me

orndorffgrant · 2024-01-03T02:17:23Z

features/unattached_status.feature

+        """
+        environment_vars: \[\]
+        errors:
+        - message: 'Failed to connect to .*


(same comment applies to json version above)

I agree it's easiest to make this a regex but I think we can assert a bit more since we know mostly what the URL will be. I think this should work

Suggested change

- message: 'Failed to connect to .*

- message: 'Failed to connect to https://invalidurl.notcanonical.com/v1/resources?architecture=amd64&kernel=(.*)&series=<release>&virt=lxc

orndorffgrant · 2024-01-03T02:17:55Z

uaclient/actions.py

+    except exceptions.ConnectivityError as e:
+        raise e


We can delete these now since they're empty :D

Suggested change

except exceptions.ConnectivityError as e:

raise e

orndorffgrant · 2024-01-03T02:18:36Z

uaclient/contract.py

+        except exceptions.ConnectivityError as e:
+            raise e


Suggested change

except exceptions.ConnectivityError as e:

raise e

orndorffgrant · 2024-01-03T02:18:49Z

uaclient/contract.py

+        except exceptions.ConnectivityError as e:
+            raise e


Suggested change

except exceptions.ConnectivityError as e:

raise e

orndorffgrant · 2024-01-03T02:19:01Z

uaclient/contract.py

+        except exceptions.ConnectivityError as e:
+            raise e


Suggested change

except exceptions.ConnectivityError as e:

raise e

lucasmoura · 2024-01-03T18:33:55Z

@orndorffgrant updated

orndorffgrant

There are several CI failures. I think they're mostly unrelated but probably worth checking

orndorffgrant · 2024-01-03T20:01:20Z

features/unattached_status.feature

+        """
+        environment_vars: \[\]
+        errors:
+        - message: 'Failed to connect to https://invalidurl.notcanonical.com/v1/resources\?architecture=amd64&kernel=(.*)&series=<release>&virt=lxc


I gave a bad suggestion sorry. I think it's good to check at least the domain name, but it turns out we can't rely on the order of the parameters on xenial

Suggested change

- message: 'Failed to connect to https://invalidurl.notcanonical.com/v1/resources\?architecture=amd64&kernel=(.*)&series=<release>&virt=lxc

- message: 'Failed to connect to https://invalidurl.notcanonical.com/v1/resources(.*)

Since ConnectityError is only raised when we detect an UrlError exception, we have decided to merge those exceptios. Additionally, we have also updated the ConnectivityError message to be more explicit on what the actual error is. Fixes: #2647

lucasmoura · 2024-01-04T18:51:07Z

@orndorffgrant I have fixed the unattached_status test and I for the other ones failing on CI, I wasn't able to reproduce them locally, so I am inclined to say that they are all flaky. But let's see what a second run of CI will tell

orndorffgrant · 2024-01-04T21:35:24Z

The bionic fix fail will be fixed by #2897

Merging

lucasmoura force-pushed the update-connection-error-message branch from 3cddf26 to a277214 Compare November 8, 2023 09:38

lucasmoura force-pushed the update-connection-error-message branch from a277214 to 4096039 Compare November 24, 2023 17:03

renanrodrigo approved these changes Nov 24, 2023

View reviewed changes

lucasmoura force-pushed the update-connection-error-message branch from 4096039 to 3bd5310 Compare November 27, 2023 12:43

orndorffgrant reviewed Nov 28, 2023

View reviewed changes

orndorffgrant added the next label Dec 11, 2023

lucasmoura force-pushed the update-connection-error-message branch from 3bd5310 to c41ae38 Compare December 15, 2023 15:04

lucasmoura force-pushed the update-connection-error-message branch from c41ae38 to db0d395 Compare December 15, 2023 19:22

orndorffgrant approved these changes Jan 3, 2024

View reviewed changes

lucasmoura force-pushed the update-connection-error-message branch from db0d395 to 5180fca Compare January 3, 2024 18:33

orndorffgrant reviewed Jan 3, 2024

View reviewed changes

lucasmoura added 2 commits January 4, 2024 15:49

po: run tools/update-pos.sh

919c54b

lucasmoura force-pushed the update-connection-error-message branch from 5180fca to 919c54b Compare January 4, 2024 18:50

orndorffgrant merged commit 009f8f5 into main Jan 4, 2024
24 of 25 checks passed

renanrodrigo deleted the update-connection-error-message branch January 4, 2024 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update connectivity error message #2817

update connectivity error message #2817

lucasmoura commented Oct 30, 2023 •

edited

Loading

github-actions bot commented Oct 30, 2023 •

edited by lucasmoura

Loading

github-actions bot commented Oct 30, 2023

lucasmoura commented Oct 30, 2023

orndorffgrant commented Oct 31, 2023

lucasmoura commented Nov 1, 2023

lucasmoura commented Nov 8, 2023

renanrodrigo Nov 24, 2023

lucasmoura Nov 27, 2023

orndorffgrant Nov 28, 2023

lucasmoura Dec 15, 2023

lucasmoura commented Dec 15, 2023

orndorffgrant left a comment

orndorffgrant Jan 3, 2024

orndorffgrant Jan 3, 2024

orndorffgrant Jan 3, 2024

orndorffgrant Jan 3, 2024

orndorffgrant Jan 3, 2024

lucasmoura commented Jan 3, 2024

orndorffgrant left a comment

orndorffgrant Jan 3, 2024

lucasmoura commented Jan 4, 2024

orndorffgrant commented Jan 4, 2024

	# we need to set them again to avoid mype warnings
	# we need to set them again to avoid mypy warnings

	- message: 'Failed to connect to .*
	- message: 'Failed to connect to https://invalidurl.notcanonical.com/v1/resources?architecture=amd64&kernel=(.*)&series=<release>&virt=lxc

update connectivity error message #2817

update connectivity error message #2817

Conversation

lucasmoura commented Oct 30, 2023 • edited Loading

Why is this needed?

Test Steps

Checklist

Does this PR require extra reviews?

github-actions bot commented Oct 30, 2023 • edited by lucasmoura Loading

github-actions bot commented Oct 30, 2023

lucasmoura commented Oct 30, 2023

orndorffgrant commented Oct 31, 2023

lucasmoura commented Nov 1, 2023

lucasmoura commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmoura commented Dec 15, 2023

orndorffgrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmoura commented Jan 3, 2024

orndorffgrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasmoura commented Jan 4, 2024

orndorffgrant commented Jan 4, 2024

lucasmoura commented Oct 30, 2023 •

edited

Loading

github-actions bot commented Oct 30, 2023 •

edited by lucasmoura

Loading