[APP-7364] [APP-7366] [RSDK-9684] [APP-7154] Add more logging, reduce monitoring loop time, misc small fixes. #56
Conversation
Otterverse
changed the title
Otterverse
changed the title
| @@ -37,7 +37,7 @@ <h2>Smart Machine Setup</h2> | |||
| <div class="form-group"> | |||
| <label for="network">Network</label> | |||
| {{if eq (len .VisibleSSIDs) 0}} | |||
| <input type="text" name="ssid" placeholder="Enter Wifi SSID" id="network" required> | |||
| <input type="text" name="ssid" placeholder="Enter Wifi SSID" id="network" required autocorrect="off" autocapitalize="off"> | |||
There was a problem hiding this comment.
For context these are "on" if omitted and that was not allowing you to connect with a SSID that was lowercased? Nice find if so 😄
There was a problem hiding this comment.
Micheal Lee found/suggested this fix. But yeah, I guess some phones/browsers see an empty text field, and if you type a password that's a normal "word" it auto-capitalizes it, which isn't ideal for passwords.
We don't use a "password" type field here, because I wanted this to be visible before submitting. Otherwise it's way too easy to accidentally typo and then it takes several minutes for the device to try out the thing, fail, and restart a hotspot.
| if err := os.MkdirAll(filepath.Dir(serviceFilePath), 0o755); err != nil { | ||
| return errw.Wrapf(err, "creating directory %s", filepath.Dir(serviceFilePath)) | ||
| } | ||
| // use this later to avoid re-enabling an existing agent service a user might have disabled |
There was a problem hiding this comment.
Want to confirm my understanding...there are users who have Viam agent installed but who don't want it to automatically start up on boot, so they've "disabled" auto start. When they upgrade their Viam agent, the configuration file where they've "disabled" the auto start gets overwritten such that startup on boot becomes true...and that's what you're fixing in this PR.
Copied the following snippet from the ticket - is this the final product (i.e. the user now has to manually restart Viam agent after an upgrade)?
Jan 07 16:11:09 pluto viam-agent[911]: 2025-01-07T22:11:09.496Z INFO viam-agent agent/subsystem.go:355 viam-agent updated from 0.11.0 to 0.12.0
Jan 07 16:11:10 pluto viam-agent[911]: 2025-01-07T22:11:09.515Z INFO viam-agent viamagent/viamagent.go:131 writing systemd service file to /usr/local/lib/systemd/system/viam-agent.service
Jan 07 16:11:10 pluto viam-agent[911]: 2025-01-07T22:11:09.515Z INFO viam-agent viamagent/viamagent.go:146 enabling systemd viam-agent service
Jan 07 16:11:10 pluto viam-agent[911]: 2025-01-07T22:11:10.091Z INFO viam-agent viamagent/viamagent.go:168 Install complete. Please (re)start the service with 'systemctl restart viam-agent' when ready.
There was a problem hiding this comment.
So if someone installed viam-agent, then runs systemctl disable viam-agent it won't start at boot. The problem is that, previously, when we did an upgrade, this same code got called, and it ALWAYS enabled the service again. Now it should only enable it on first/new install.
The snippet is the old behavior, where it shows "enabling systemd viam-agent service" was the problem. That should only happen on new installs now.
| @@ -24,7 +24,7 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| minimalCheckInterval = time.Second * 60 | |||
| minimalCheckInterval = time.Second * 5 | |||
There was a problem hiding this comment.
What's the monitoring loop checking for on a super high level? Is this monitoring for FTDC data in the viam-server? What other things are "monitored" on a regular basis via the agent ?
There was a problem hiding this comment.
This is how often it fetches a new config from the cloud. The cloud itself can say "check again after X time") but that's currently not implemented, so it falls to this.
The main loop basically: check for new config > apply changes/updates (if there) > start subsystems that should be started > check health of subsystems > repeat
So the check interval determins (roughly) the overall timing. At 5 seconds, it may have trouble keeping up if there are slow responses or other work to do, but that should be fine.
| } | ||
| // use this later to avoid re-enabling an existing agent service a user might have disabled | ||
| _, err = os.Stat(serviceFilePath) | ||
| newInstall := err != nil |
There was a problem hiding this comment.
Worth checking for a file does not exist error?
There was a problem hiding this comment.
This covers edge cases like file corruption too. If we can't stat, then it's best to treat it as a new install, IMHO. But a good thought! In many other cases it'd be worth differentiating.
Quick PR for easy fixes.
Needs testing for the linked Jira issues