source: do not fire a configured event again if nothing changed #1814
Conversation
When handling a POST request to /source, Subiquity sends a 'source configured' event. This signals other controllers / models that they need to restart their tasks that depend on the source being used. However, if the user of the installer goes back all the way to the source page and submits it again without changing the settings, there should be no reason to restart the machinery. If a call to source ends up doing no modification to the model (i.e., not changing the source used or the search_drivers setting), we now avoid emitting the 'source configured' event ; except if the model has not been configured yet. Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
ogayot
force-pushed
the
noop-no-emit-source-configured
branch
from
a838239 to
fff2f65
Compare
| try: | ||
| new_source = self.model.get_matching_source(source_id) | ||
| except KeyError: | ||
| # TODO going forward, we should probably stop silently ignoring |
There was a problem hiding this comment.
Yes, that's a good point, I don't like the silent ignore. At minimum let's log.
This scenario is not an autoinstall one - that's handled above with load_autoinstall_data() / start(), so I think the only cases where this could happen are
- client bug
- server bug
- hardcoded source value in an automated test.
I suspect we should log and raise here, what do you think?
There was a problem hiding this comment.
Yes I agree. I'm going to add the logging statement.
As for re-raising, IMO it depends if we feel like that this PR might make things better for mantic. If we ponder landing this to ubuntu/mantic, I definitely don't want to re-raise. But going forward, I think we should!
IIRC, there might be a race condition in some scenarios between:
- subiquity reading the source catalog
- receiving a POST to /source.
There was a problem hiding this comment.
ah yes, I'm presuming the read of the source catalog has happened here.
OK. Let's go with the error level log for now.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
When handling a POST request to /source, Subiquity sends a 'source configured' event. This signals other controllers / models that they need to restart their tasks that depend on the source being used.
However, if the user of the installer goes back all the way to the source page and submits it again without changing the settings, there should be no reason to restart the machinery.
If a call to source ends up doing no modification to the model (i.e., not changing the source used or the search_drivers setting), we now avoid emitting the 'source configured' event ; except if the model has not been configured yet.
NOTE: this is theoretically more of an optimization than a bug fix (unless it significantly speeds up the install) and may or may not be suitable for a merge to ubuntu/mantic.
LP: #2039082