[LP#2068597] Metallb deployments can get stuck in a config-changed busy loop #41
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
addyess
force-pushed
the
bug/lp2068597/stop-retrying-during-apifailures
branch
2 times, most recently
from
28cd9dc to
18f96cd
Compare
addyess
force-pushed
the
bug/lp2068597/stop-retrying-during-apifailures
branch
from
18f96cd to
fc992ee
Compare
addyess
force-pushed
the
bug/lp2068597/stop-retrying-during-apifailures
branch
from
eb06f62 to
2cb36be
Compare
eaudetcobello
requested changes
Jun 7, 2024
src/charm.py
Outdated
Comment on lines
54
to
55
| if str_to_test.count("/") != 1: | ||
| return False |
There was a problem hiding this comment.
nothing scarier here than using regex for ip address matching. I'll let python check it. See if you like my change here.
i want to make sure it's a ip_network and NOT an ip_address. The python library accepts a bare address to be processed as a "network" even when it's not in CIDR notation
assert ipaddress.ip_network("1.2.3.4") == ipaddress.ip_network("1.2.3.4/32")HOWEVER, using ip_address totally raises a ValueError
>>> ipaddress.ip_address("1.2.3.4/32")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/ubuntu/.pyenv/versions/3.12.3/lib/python3.12/ipaddress.py", line 54, in ip_address
raise ValueError(f'{address!r} does not appear to be an IPv4 or IPv6 address')
ValueError: '1.2.3.4/32' does not appear to be an IPv4 or IPv6 address
```
src/charm.py
Outdated
Comment on lines
78
to
86
| def _block_on_forbidden(unit: ops.model.Unit): | ||
| try: | ||
| yield | ||
| except (ApiError, ManifestClientError) as ex: | ||
| http = ex.args[1] if isinstance(ex, ManifestClientError) else ex | ||
| if http.status.code == 403: | ||
| unit.status = BlockedStatus("API Access Forbidden, deploy with --trust") | ||
| else: | ||
| raise |
There was a problem hiding this comment.
Cool pattern, had not seen before.
tests/unit/test_charm.py
Outdated
| @@ -168,9 +175,10 @@ def test_update_status(harness, lk_manifests_client, lk_charm_client): | |||
| harness.begin() | |||
|
|
|||
| # With nothing mocked, all resources will appear as missing | |||
| harness.charm._stored.configured = True | |||
| harness.charm.model.unit.status = expected = BlockedStatus("unchanged on missing") | |||
There was a problem hiding this comment.
Unchanged on missing? Is that the best wording?
There was a problem hiding this comment.
i just tried to make this more clear. I just wanted some text that clearly indicates the charm code isn't changing the blocked message
eaudetcobello
approved these changes
Jun 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LP#2068597
Overview
When metallb doesn't have permissions because of lack of
trust, the charm can get stuck trying to apply a configuration that will never finish causing the config-change hook to perpetually run.Changes
stopto quick retrying after a certain number of timesblockedstatusupdate_statushandler declares everything is readytrust