feat: new wait for idle

[dimaqq]

Pull the new Model.wait_for_idle implementation from #1104

• pull in the unit tests
• pull in the implementation
• new unit tests pass
• all unit tests pass
• feature flag
• run integration tests with and without the feature flag
• implement idle/ready distinction properly
• merge chore: remove juju.loop, deprecated before 3.0 #1242 and rebase

[dimaqq]

@james-garner-canonical I'd appreciate an early review.

I'm expecting integration tests to pass 🎉

There's still some refactoring to be done... and I'm happy to hear your thoughts about that and areas to improve in general.

[james-garner-canonical]

Looks great, so much nicer than the previous implementation. Really cool to have the tests passing on both. Was wait_for_idle tested much previously?

[dimaqq]

@james-garner-canonical the big refactor is done, I hope you'll like the result :)

still a draft, as some unit tests are missing and I have one internal concern, but should be ready for review :)

[james-garner-canonical]

I haven't looked at the tests (though I noticed the addition of freezegun), but I've left some comments on the new implementation. My main concern is that the generator/coroutine business would be a lot easier (for me) to follow if we used a regular class/object instead (see comment on def _loop) -- it took me a while to get my head around. I do like the separation of the time-based checks. Lmk if you want me to look at anything else in particular

[dimaqq]

@james-garner-canonical I've refactored the loop to be an [async] filter.

It turns out that I've misunderstood the definition of "idle".

In short:

• ready units: depends on workload (and possibly agent, machine, etc.)
• idle units: depends on agent only

This means that I have to update the idle.check code.

Edit: done now.

[dimaqq]

@james-garner-canonical idle implemented, most comments addressed, tests pass again. please review :)

[james-garner-canonical]

Exciting updates, glad tests are passing now.

I'm finding it hard to follow the logical changes between versions on this PR, I think primarily because the generator adds a fair bit of mental overhead for me -- it's kind of nicer now (though the new outer async loop construct took me some time to digest), but I still have to put in more mental work to follow the various yield ... breaks and yield ... continues.

For now I've made some requests for changes to make it easier for me to follow what's going on. Sorry if that seems like wasted effort -- it did take me a while, but I kind of had to do it anyway to follow the code.

Let me know if you want me to look closer at specific logical changes (looks like there are changes to the idle_since logic?). I still haven't looked in detail at your tests either, would you like further review there?

Also, what's the latest with running charmers' integration tests on the latest version? Are there still failures with the new wait_for_idle that pass with the old one?

[james-garner-canonical]

Suggestion: simplify this for noobs like me ... btw at first I thought this could be simplified without changing anything else, but after making it to _idle.loop I realised that my original simplification was totally incorrect, which suggests that the current version is a bit tricky to follow ... the below suggestion assumes a later suggestion to refactor _idle.loop to use a class, which I think makes things a lot easier to follow -- it certainly does for me

Suggested change

	async def status_on_demand():
	while True:
	yield _idle.check(
	await self.get_status(),
	apps=apps,
	raise_on_error=raise_on_error,
	raise_on_blocked=raise_on_blocked,
	status=status,
	)
	async for done in _idle.loop(
	status_on_demand(),
	apps=apps,
	wait_for_exact_units=wait_for_exact_units,
	wait_for_units=wait_for_units,
	idle_period=idle_period,
	):
	timing_checker = _idle.TimingChecker()
	while True:
	full_status = await self.get_status()
	check_status = _idle.check(
	raw_status,
	apps=apps,
	raise_on_error=raise_on_error,
	raise_on_blocked=raise_on_blocked,
	status=status,
	)
	done = timing_checker.check(
	check_status,
	apps=apps,
	wait_for_exact_units=wait_for_exact_units,
	wait_for_units=wait_for_units,
	idle_period=idle_period,
	)

[james-garner-canonical]

Suggestion: make it clear up front that we're only interested in unit.machine -- there's no elif/else coming

Suggested change

	if unit.machine:
	machine = full_status.machines[unit.machine]
	assert isinstance(machine, MachineStatus)
	assert machine.instance_status
	if machine.instance_status.status == "error" and raise_on_error:
	raise JujuMachineError(
	f"{unit_name!r} machine {unit.machine!r} has errored: {machine.instance_status.info!r}"
	)
	if not unit.machine:
	continue
	machine = full_status.machines[unit.machine]
	assert isinstance(machine, MachineStatus)
	assert machine.instance_status
	if machine.instance_status.status == "error" and raise_on_error:
	raise JujuMachineError(
	f"{unit_name!r} machine {unit.machine!r} has errored: {machine.instance_status.info!r}"
	)

[james-garner-canonical]

Suggestion: I really like how the loops below all do one thing, much nicer to read and follow than the original wait_for_idle. I notice that the first few only do anything if raise_on_error, while the next couple only do something if raise_on_blocked. How about something like:

    if raise_on_error:
        _check_for_errors(full_status, apps, units)
    if raise_on_blocked:
        _check_for_blocked(full_status, apps, units)

[james-garner-canonical]

Minor suggestion: in the CheckStatus definition, use = field(default_factory=set). Reasoning: I think rv = CheckStatus() will look more natural to readers (like me =))

[james-garner-canonical]

Suggestion: I really think that a class to encapsulate the state, and a single synchronous method call call with returns, is so much easier to follow than the async generator, and it seems like it's pretty straightforwardly logically equivalent -- just a lot easier to follow for noobs like me

Suggested change

	async def loop(
	foo: AsyncIterable[CheckStatus | None],
	*,
	apps: AbstractSet[str],
	wait_for_exact_units: int | None = None,
	wait_for_units: int,
	idle_period: float,
	) -> AsyncIterable[bool]:
	"""The outer, time-dependents logic of a wait_for_idle loop."""
	idle_since: dict[str, float] = {}
	async for status in foo:
	logger.info("wait_for_idle iteration %s", status)
	now = time.monotonic()
	if not status:
	yield False
	continue
	expected_idle_since = now - idle_period
	# FIXME there's some confusion about what a "busy" unit is
	# are we ready when over the last idle_period, every time sampled:
	# a. >=N units were ready (possibly different each time), or
	# b. >=N units were ready each time
	for name in status.units:
	if name in status.idle_units:
	idle_since[name] = min(now, idle_since.get(name, float("inf")))
	else:
	idle_since[name] = float("inf")
	if busy := {n for n, t in idle_since.items() if t > expected_idle_since}:
	logger.info("Waiting for units to be idle enough: %s", busy)
	yield False
	continue
	for app_name in apps:
	ready_units = [
	n for n in status.ready_units if n.startswith(f"{app_name}/")
	]
	if len(ready_units) < wait_for_units:
	logger.info(
	"Waiting for app %r units %s >= %s",
	app_name,
	len(status.ready_units),
	wait_for_units,
	)
	yield False
	break
	if (
	wait_for_exact_units is not None
	and len(ready_units) != wait_for_exact_units
	):
	logger.info(
	"Waiting for app %r units %s == %s",
	app_name,
	len(ready_units),
	wait_for_exact_units,
	)
	yield False
	break
	else:
	yield True
	class TimingChecker:
	"""The outer, time-dependent logic of a wait_for_idle loop."""
	def __init__(self):
	self.idle_since: dict[str, float] = {}
	def check(
	status: CheckStatus,
	*
	apps: AbstractSet[str],
	wait_for_exact_units: int | None = None,
	wait_for_units: int,
	idle_period: float,
	) -> bool:
	logger.info("wait_for_idle iteration %s", status)
	now = time.monotonic()
	if not status:
	return False
	expected_idle_since = now - idle_period
	# FIXME there's some confusion about what a "busy" unit is
	# are we ready when over the last idle_period, every time sampled:
	# a. >=N units were ready (possibly different each time), or
	# b. >=N units were ready each time
	for name in status.units:
	if name in status.idle_units:
	self.idle_since[name] = min(now, idle_since.get(name, float("inf")))
	else:
	self.idle_since[name] = float("inf")
	if busy := {n for n, t in self.idle_since.items() if t > expected_idle_since}:
	logger.info("Waiting for units to be idle enough: %s", busy)
	return False
	for app_name in apps:
	ready_units = [
	n for n in status.ready_units if n.startswith(f"{app_name}/")
	]
	if len(ready_units) < wait_for_units:
	logger.info(
	"Waiting for app %r units %s >= %s",
	app_name,
	len(status.ready_units),
	wait_for_units,
	)
	return False
	if (
	wait_for_exact_units is not None
	and len(ready_units) != wait_for_exact_units
	):
	logger.info(
	"Waiting for app %r units %s == %s",
	app_name,
	len(ready_units),
	wait_for_exact_units,
	)
	return False
	return True

[dimaqq]

I've pushed a separate branch with this refactor only: #1245 PTAL

[dimaqq]

closing in favour of #1245