Skip to content
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.

Sign up for GitHub

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

Optimize & fix unit removal #967

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Optimize & fix unit removal #967

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d1e7a5c
Fix unit.destroy() to use unit tag instead of name
cderici Oct 20, 2023
92562c2
Add integration test for unit.destroy()
cderici Oct 20, 2023
bcc9bac
Optimize model.destroy_units()
cderici Oct 20, 2023
db3b379
Make keyword arguments explicit in call
cderici Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 19 additions & 21 deletions juju/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2157,32 +2157,30 @@ async def destroy_unit(self, unit_id, destroy_storage=False, dry_run=False, forc
"""Destroy units by name.

"""
connection = self.connection()
app_facade = client.ApplicationFacade.from_connection(connection)

# Get the corresponding unit tag
unit_tag = tag.unit(unit_id)
if unit_tag is None:
log.error("Error converting %s to a valid unit tag", unit_id)
return JujuUnitError("Error converting %s to a valid unit tag", unit_id)

log.debug(
'Destroying unit %s', unit_id)

return await app_facade.DestroyUnit(units=[{
'unit-tag': unit_tag,
'destroy-storage': destroy_storage,
'force': force,
'max-wait': max_wait,
'dry-run': dry_run,
}])
await self.destroy_units(unit_id, destroy_storage, dry_run, force, max_wait)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the last 4 args be kwargs? I don't know exactly how python handles variadic positional / ket word args, but this seems a little fragile to me. It seems like later args could be misinterpreted as unit_names with only a small change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, this call needs to be fixed, but changing the args on the definition would mean changing the API in a run-by, which a lot of users would be upset about. That needs to be done in a major release.

I also realized we're not returning the result here (mostly because this is called only for its side-effect, but nevertheless). I'll fix it, thanks for the heads up 👍


async def destroy_units(self, *unit_names, destroy_storage=False, dry_run=False, force=False, max_wait=None):
"""Destroy several units at once.

"""
for u in unit_names:
await self.destroy_unit(u, destroy_storage, dry_run, force, max_wait)
connection = self.connection()
app_facade = client.ApplicationFacade.from_connection(connection)

units_to_destroy = []
for unit_id in unit_names:
unit_tag = tag.unit(unit_id)
if unit_tag is None:
log.error("Error converting %s to a valid unit tag", unit_id)
raise JujuUnitError("Error converting %s to a valid unit tag", unit_id)
units_to_destroy.append({
'unit-tag': unit_tag,
'destroy-storage': destroy_storage,
'force': force,
'max-wait': max_wait,
'dry-run': dry_run,
})
log.debug('Destroying units %s', unit_names)
return await app_facade.DestroyUnit(units=units_to_destroy)

def download_backup(self, archive_id, target_filename=None):
"""Download a backup archive file.
Expand Down
9 changes: 7 additions & 2 deletions juju/unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ def get_subordinates(self):
return [u for u_name, u in self.model.units.items() if u.is_subordinate and
u.principal_unit == self.name]

async def destroy(self):
async def destroy(self, destroy_storage=False, dry_run=False, force=False, max_wait=None):
"""Destroy this unit.

"""
Expand All @@ -111,7 +111,12 @@ async def destroy(self):
log.debug(
'Destroying %s', self.name)

return await app_facade.DestroyUnit(units=[{"unit-tag": self.name}])
return await app_facade.DestroyUnit(units=[{"unit-tag": self.tag,
'destroy-storage': destroy_storage,
'force': force,
'max-wait': max_wait,
'dry-run': dry_run,
}])
remove = destroy

async def get_public_address(self):
Expand Down
16 changes: 16 additions & 0 deletions tests/integration/test_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,3 +280,19 @@ async def test_subordinate_units(event_loop):
assert n_unit.principal_unit == 'ubuntu/0'
assert u_unit.principal_unit == ''
assert [u.name for u in u_unit.get_subordinates()] == [n_unit.name]


@base.bootstrapped
async def test_destroy_unit(event_loop):
async with base.CleanModel() as model:
app = await model.deploy(
'juju-qa-test',
application_name='test',
num_units=3,
)
# wait for the units to come up
await model.block_until(lambda: app.units, timeout=480)

await app.units[0].destroy()
await asyncio.sleep(5)
assert len(app.units) == 2