identity: fail on missing section sooner #1965

Chris-Peterson444 · 2024-04-09T18:01:00Z

Moves the logic for checking if user-data or identity section is provided (on server) in the autoinstall config to the load_autoinstall_data function. Without this change, the exception thrown in apply_autoinstall_config won't halt the installer until
the postinstall steps (LP: 2060547).

The change is ready but I am keeping this as a draft PR until I can test this on Desktop.

Chris-Peterson444 · 2024-04-09T21:15:09Z

This doesn't quite work for desktop because app.base_model.source.current isn't set correctly until the controllers are started, which only happens after loading the autoinstall data. I think the quickest (and hackiest) way to solve this is to inspect the sources information in /cdrom/casper/install-sources.yaml from the identity controller itself.

ogayot · 2024-04-10T12:50:04Z

I've only tested in dry-run mode but I'm not seeing an obvious difference between the previous and the new behavior. What is it that we are trying to achieve? Exit the subiquity server process before partitioning starts?

Chris-Peterson444 · 2024-04-10T17:18:39Z

Yes. On server, if you omit both the identity section and the user-data section then an error will be emitted when the identity controller loads the autoinstall data, but the installation with proceed until the postinstall stage where it hangs. We want to make sure this error halts the installer immediately.

dbungert · 2024-04-11T16:20:37Z

To elaborate on what Chris said, the failure mode here is really best seen to be believed.

So you supply a trivial cloud-config like so to live-server:

#cloud-config
autoinstall:
  version: 1

An exception is seen in the logs complaining about the lack of user-data or identity, but the entire install section continues, including partitioning of disks and copying to the target system.

THEN, at this point, the install hangs, as the post-install models are not configured, and there isn't even a visual indication on why this is other than that Exception shown all the way before partitioning even started.

My goal here was to move that check earlier, since the time between the problem report and when the install stops is measured in how long it takes for partitioning + rsync to happen.

Chris-Peterson444 · 2024-04-12T00:17:01Z

Yeah, sorry for not being more explicit. So the way I see it this could have regressed the following conditions:

Server autoinstall:
- Includes only Identity section: OK
- Includes only user-data section: OK
- Identity section is explicitly interactive: OK
- Identity section is implicitly interactive (i.e. * ): OK
- reset-partition-only is true: OK
- none of the above scenarios and missing both identity and user-data sections: Error
Desktop autoinstall:
- Provides neither identity or userdata: OK

I was able to confirm all of these cases pass in VM tests. I did find a separate bug in the reset-partition-only handling but I'll file a separate bug about that. Edit: reset-partition bug LP: #2061042

dbungert

Well that was a bit more involved than I would have guessed. Nice work!

Conceptually this is fine, my major feedback is to just relocate some parts to a place where we might remove the duplication, or at least make it more greppable. I would be quite surprised to find some of this in identity.py.

subiquity/server/controllers/identity.py

dbungert · 2024-04-12T00:39:57Z

subiquity/server/controllers/identity.py

+        if (
+            source_config is not None
+            and (id := source_config.get("id")) is not None
+            and "server" not in id


I'm wary of checking the string set on the id, it's semi free form.
What's the motivation for this section? It looks like we can drop it and get the same outcome? If I'm missing something let me know.

in subiquity/server/controllers/source.py in load_autoinstall_data we get the source id with a .get("id") on the autoinstall data and do an exact match on the IDs in the catalog via get_matching_source in subiquity/models/source.py so I felt that it wasn't that bad of a comparison to just check that server wasn't in the name.

In the autoinstall case if the id the user provides doesn't match any sources in the catalog then we get a KeyError anyways so I wasn't super concerned with weird outcomes if the user provides something unrealistic.

*but this section matches the source controller/model logic of letting autoinstall inform us of the source id

Chris-Peterson444 · 2024-04-12T01:11:06Z

Well that was a bit more involved than I would have guessed. Nice work!

Right? And thanks!

Another thing I had in mind about keeping the logic in the identity controller: we probably want to remove this logic at some point in the future and put it back into the load_autoinstall function when we can just throw from there and expect an immediate halt to the overall install. Edit: re-reading your comment now I see what you're saying. I'll move the bits around tomorrow!

Chris-Peterson444 · 2024-04-12T18:08:20Z

Moved to separate utility functions owned by the respective controllers, as requested. There is still the open question about the source controller logic.

subiquity/server/controllers/filesystem.py

dbungert · 2024-05-03T19:44:52Z

subiquity/server/controllers/source.py

+
+        # Check the sources available the same way the source controller does
+        # but return a bool for if the variant is desktop
+        path = "/cdrom/casper/install-sources.yaml"


let's move the existing magic path and reference that common definition, please, or maybe this is obsolete given the previous comment

Yeah obsolete but I still think it's worth moving the magic var definition somewhere global

dbungert · 2024-05-03T19:54:48Z

subiquity/server/controllers/source.py

+        source_config = self.app.autoinstall_config.get(self.autoinstall_key)
+        if (
+            source_config is not None
+            and (id := source_config.get("id")) is not None


I think I'm vetoing this approach, as it relies on behavior in a string that is theoretically an arbitrary value.
What I want instead is for the SourceController.start logic to move earlier in the sequence - out of start entirely actually - so that we can answer this question before SourceController.start() has been called. Then we should be able to just get the info needed. What do you think?

When I originally looked at this bug I think I had reservations about moving the logic in SourceController.start to somewhere sooner, but looking at it again now I don't see why we can't. We can probably move that logic to SourceController.load_autoinstall_data. I'll give it a shot.

Moves the logic for checking if user-data or identity section is provided (on server) in the autoinstall config to the load_autoinstall_data function. Without this change, the exception thrown in apply_autoinstall_config won't halt the installer until the postinstall steps (LP: 2060547).

dbungert

I have suggested a few tweaks but this seems fine. What further work do you imagine for this draft PR? What sort of VM testing has been done at this point?

subiquity/server/controllers/filesystem.py

The identity controller shouldn't own the logic for determining if the filesystem controller will only install the reset partition. Creates a utility function that can be called by the identity controller to determine if only installing the reset partition.

The current check for Desktop in the identity controller doesn't work because the variant isn't set until SourceController.start is called. Move this logic to earlier in the sequence so that the variant information can be referenced by later-loaded controllers.

Chris-Peterson444 · 2024-05-22T20:53:31Z

Thanks for the suggestions! Currently I am running the following VM tests before I mark this ready:

Server autoinstall:
- Includes only Identity section, successful autoinstall
- Includes only user-data section, successful autoinstall
- Identity section is explicitly interactive, successful (interactive) autoinstall
- Identity section is implicitly interactive (i.e. * ), successful (interactive) autoinstall
- reset-partition-only is true, successful autoinstall
- none of the above scenarios and missing both identity and user-data sections, installer halts and displays error
Desktop autoinstall:
- Provides neither identity or userdata, successful autoinstall and does user creation on first boot

Chris-Peterson444 · 2024-05-22T21:45:43Z

The above tests all pass

Chris-Peterson444 · 2024-05-22T21:45:47Z

The above tests all pass

Chris-Peterson444 marked this pull request as ready for review April 12, 2024 00:17

dbungert reviewed Apr 12, 2024

View reviewed changes

Chris-Peterson444 force-pushed the autoinstall-bug-lp-2060547 branch 2 times, most recently from d29991b to 1f11807 Compare April 12, 2024 18:05

dbungert requested changes May 3, 2024

View reviewed changes

Chris-Peterson444 added 2 commits May 8, 2024 09:28

identity: refactor requirements tests

7fedee5

Chris-Peterson444 force-pushed the autoinstall-bug-lp-2060547 branch from 1f11807 to 31b6d3e Compare May 8, 2024 21:20

Chris-Peterson444 marked this pull request as draft May 8, 2024 22:16

Chris-Peterson444 force-pushed the autoinstall-bug-lp-2060547 branch from 31b6d3e to 90d4a0e Compare May 8, 2024 23:02

dbungert approved these changes May 22, 2024

View reviewed changes

subiquity/server/controllers/filesystem.py Outdated Show resolved Hide resolved

subiquity/server/controllers/filesystem.py Outdated Show resolved Hide resolved

subiquity/server/controllers/filesystem.py Outdated Show resolved Hide resolved

Chris-Peterson444 added 2 commits May 22, 2024 13:48

Chris-Peterson444 force-pushed the autoinstall-bug-lp-2060547 branch from 90d4a0e to 68ae5a4 Compare May 22, 2024 20:49

Chris-Peterson444 marked this pull request as ready for review May 22, 2024 21:45

Chris-Peterson444 merged commit c5131fc into canonical:main May 22, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

identity: fail on missing section sooner #1965

identity: fail on missing section sooner #1965

Chris-Peterson444 commented Apr 9, 2024

Chris-Peterson444 commented Apr 9, 2024

ogayot commented Apr 10, 2024

Chris-Peterson444 commented Apr 10, 2024

dbungert commented Apr 11, 2024

Chris-Peterson444 commented Apr 12, 2024 •

edited

dbungert left a comment

dbungert Apr 12, 2024

Chris-Peterson444 Apr 12, 2024

Chris-Peterson444 Apr 12, 2024

Chris-Peterson444 commented Apr 12, 2024 •

edited

Chris-Peterson444 commented Apr 12, 2024

dbungert May 3, 2024

Chris-Peterson444 May 6, 2024

dbungert May 3, 2024

Chris-Peterson444 May 6, 2024

dbungert left a comment

Chris-Peterson444 commented May 22, 2024

Chris-Peterson444 commented May 22, 2024

Chris-Peterson444 commented May 22, 2024

identity: fail on missing section sooner #1965

identity: fail on missing section sooner #1965

Conversation

Chris-Peterson444 commented Apr 9, 2024

Chris-Peterson444 commented Apr 9, 2024

ogayot commented Apr 10, 2024

Chris-Peterson444 commented Apr 10, 2024

dbungert commented Apr 11, 2024

Chris-Peterson444 commented Apr 12, 2024 • edited

dbungert left a comment

Choose a reason for hiding this comment

dbungert Apr 12, 2024

Choose a reason for hiding this comment

Chris-Peterson444 Apr 12, 2024

Choose a reason for hiding this comment

Chris-Peterson444 Apr 12, 2024

Choose a reason for hiding this comment

Chris-Peterson444 commented Apr 12, 2024 • edited

Chris-Peterson444 commented Apr 12, 2024

dbungert May 3, 2024

Choose a reason for hiding this comment

Chris-Peterson444 May 6, 2024

Choose a reason for hiding this comment

dbungert May 3, 2024

Choose a reason for hiding this comment

Chris-Peterson444 May 6, 2024

Choose a reason for hiding this comment

dbungert left a comment

Choose a reason for hiding this comment

Chris-Peterson444 commented May 22, 2024

Chris-Peterson444 commented May 22, 2024

Chris-Peterson444 commented May 22, 2024

Chris-Peterson444 commented Apr 12, 2024 •

edited

Chris-Peterson444 commented Apr 12, 2024 •

edited