confd: set IITO startup/failure conditions (fixes status LEDs) - #1587
confd: set IITO startup/failure conditions (fixes status LEDs)#1587pjator wants to merge 1 commit into
Conversation
|
When Currently, a condition is set when startup is applied: Line 844 in 279fc86 But there are no corresponding conditions for the failure cases. I suggest we add those and then update the IITO configuration to match. |
wkz
left a comment
wkz
left a comment
There was a problem hiding this comment.
Let's fix the root cause so that it (1) fixes all boards and (2) also fixes the error cases when startup fails to apply cleanly.
|
Fixed per your suggestion. confd now sets run/startup/success after a clean bootstrap and run/failure/success/run/failure/failure in handle_startup_failure() (fail-secure + panic paths). The board-specific workaround is dropped. Verified on hardware: LED lights on boot, eMMC visible. |
pjator
changed the title
The product and common iitod.json configurations reference finit
conditions /run/finit/cond/run/startup/success and
/run/finit/cond/run/failure/{success,failure} to drive status LEDs,
but nothing ever creates them, so the LEDs stay dark on every boot.
confd already signals usr/bootstrap when startup applies; add the
matching conditions for a clean startup and for the failure paths
(fail-secure fallback and the fatal runlevel 9 case), which fixes
LED behaviour on all boards.
Co-authored-by: Jarvis (Hermes Agent) <jarvis@woxblom.com>
There was a problem hiding this comment.
Thanks for taking another stab at this! Sorry about the slew of comments 😅 - you picked a pretty core piece of code for your first contribution.
In addition to these changes, I think handle_startup_failure() might need some additional changes. Let's wait and see what @troglobit thinks.
Also, the IITO config needs to be updated to reference the new condition names.
| char path[128]; | ||
| char *p; | ||
|
|
||
| snprintf(path, sizeof(path), "/run/finit/cond/%s", cond); |
There was a problem hiding this comment.
| snprintf(path, sizeof(path), "/run/finit/cond/%s", cond); | |
| snprintf(path, sizeof(path), "/run/finit/cond/usr/%s", cond); |
I understand the impulse to use the old condition names from IITO's config. However, the run/ namespace is reserved by Finit to signal the status of run jobs under its control. The usr/ namespace is allocated for general use by the system.
| /* mkdir -p parent directories (e.g. /run/finit/cond/run/startup) */ | ||
| for (p = path + strlen("/run/finit/cond/"); *p; p++) { | ||
| if (*p == '/') { | ||
| *p = '\0'; | ||
| mkdir(path, 0755); | ||
| *p = '/'; | ||
| } | ||
| } |
There was a problem hiding this comment.
| /* mkdir -p parent directories (e.g. /run/finit/cond/run/startup) */ | |
| for (p = path + strlen("/run/finit/cond/"); *p; p++) { | |
| if (*p == '/') { | |
| *p = '\0'; | |
| mkdir(path, 0755); | |
| *p = '/'; | |
| } | |
| } |
Finit guarantees that /run/finit/cond/usr exists (via a tmpfiles snippet). So with the suggested change above, together with limiting condition names to a single level, we can safely skip this.
| symlink("/run/finit/cond/reconf", "/run/finit/cond/usr/bootstrap"); | ||
|
|
||
| /* Signal IITO that startup applied cleanly (status LEDs) */ | ||
| set_finit_cond("run/startup/success"); | ||
|
|
There was a problem hiding this comment.
| symlink("/run/finit/cond/reconf", "/run/finit/cond/usr/bootstrap"); | |
| /* Signal IITO that startup applied cleanly (status LEDs) */ | |
| set_finit_cond("run/startup/success"); | |
| set_finit_cond("bootstrap"); |
Let's move the existing condition to use the new helper, but I don't think we can be sure that startup has cleanly applied at this point. I have marked that point in a separate comment.
| if (r != SR_ERR_OK) | ||
| WARN("Failed to sync startup datastore: %s", sr_strerror(r)); | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
| set_finit_cond("startup-config-ok"); | |
| return 0; |
I believe this is the point where we want to signal a successful application of startup-config
| int r; | ||
|
|
||
| ERROR("Failed loading startup-config, reverting to Fail Secure mode!"); | ||
|
|
There was a problem hiding this comment.
| set_finit_cond("startup-config-error"); |
Whatever happens beyond this point, I think we should leave a marker that we could not apply startup-config.
| /* Nuclear option: wipe everything */ | ||
| systemf("rm -f /etc/sysrepo/data/*startup* /etc/sysrepo/data/*running* /dev/shm/sr_*"); | ||
| set_finit_cond("run/failure/failure"); | ||
| return; |
There was a problem hiding this comment.
@troglobit: Should we not emit the CRITICAL ERROR: ... in this scenario as well? The system is still without any defined config in this case, no?
| ERROR("Failed loading failure-config, aborting!"); | ||
| banner_append("CRITICAL ERROR: Logins are disabled, no credentials available"); | ||
| systemf("initctl -nbq runlevel 9"); | ||
| set_finit_cond("run/failure/failure"); |
There was a problem hiding this comment.
| set_finit_cond("run/failure/failure"); | |
| set_finit_cond("failure-config-error"); |
Depending on @troglobit's input, this path may have to be merged with the earlier error path. But I think at think point we want to signal "we tried, but failed, to apply failure-config.
| } | ||
|
|
||
| banner_append("ERROR: Corrupt startup-config, system has reverted to default login credentials"); | ||
| set_finit_cond("run/failure/success"); |
There was a problem hiding this comment.
| set_finit_cond("run/failure/success"); | |
| set_finit_cond("failure-config-ok"); |
For symmetry with the other condition names.
|
@wkz Thanks for the thorough review — it's really helpful, and I agree with the direction (usr/ namespace, single-level condition names, and updating the IITO config accordingly). I'll wait for @troglobit's input before pushing an updated version, so we can address everything in one pass and match the structure you'd prefer. I'll follow up once he's had a chance to weigh in. |
2 participants
iitod.json expects /run/finit/cond/run/startup/success before the LAN and status LEDs light up, but nothing ever creates it. Add a small finit service (after confd/ready) that sets the condition on every boot.
Description
Checklist
Tick relevant boxes, this PR is-a or has-a: