-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[APP-7364] [APP-7366] [RSDK-9684] [APP-7154] Add more logging, reduce monitoring loop time, misc small fixes. #56
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
{{else}} | ||
<div class="select-border"> | ||
<select name="ssid" id="network" required> | ||
|
@@ -52,14 +52,14 @@ <h2>Smart Machine Setup</h2> | |
|
||
<div class="form-group"> | ||
<label for="password">Password</label> | ||
<input type="text" name="password" id="password" placeholder="Password"> | ||
<input type="text" name="password" id="password" placeholder="Password" autocorrect="off" autocapitalize="off"> | ||
</div> | ||
{{end}} | ||
|
||
{{if not .IsConfigured}} | ||
<div class="form-group"> | ||
<label for="viamconfig">Device Config</label> | ||
<textarea type="textarea" name="viamconfig" id="viamconfig" required placeholder="No config found on device. Paste your viam.json file here."></textarea> | ||
<textarea type="textarea" name="viamconfig" id="viamconfig" required placeholder="No config found on device. Paste your viam.json file here." autocorrect="off" autocapitalize="off"></textarea> | ||
</div> | ||
{{end}} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,14 +123,14 @@ func Install(logger logging.Logger) error { | |
return errw.Wrap(err, "getting service file path") | ||
} | ||
|
||
//nolint:gosec | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if someone installed viam-agent, then runs 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Working on getting manual testing setup. Lmk if I'm moving in right direction. Using an RPi5 because agent runs on linux (and I use Mac). Should I be:
Please let me know gaps here. Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validated by disabling the agent and upgrading from a stable version {
"agent": {
"viam-agent": {
"pin_version": "0.12.0"
},
"viam-server": {
"release_channel": "stable",
"attributes": {}
}
}
} to a pinned URL: {
"agent": {
"viam-agent": {
"pin_url": "file:///home/jeep/dev/agent/bin/viam-agent-custom-aarch64"
},
"viam-server": {
"release_channel": "stable",
"attributes": {}
}
}
} After the upgrade and subsequent reboot, I validated the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After re-enable and reboot the viam-agent starts up on its own 👍
|
||
_, err = os.Stat(serviceFilePath) | ||
newInstall := err != nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth checking for a file does not exist error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
logger.Infof("writing systemd service file to %s", serviceFilePath) | ||
//nolint:gosec | ||
if err := os.WriteFile(serviceFilePath, serviceFileContents, 0o644); err != nil { | ||
|
||
newFile, err := agent.WriteFileIfNew(serviceFilePath, serviceFileContents) | ||
if err != nil { | ||
return errw.Wrapf(err, "writing systemd service file %s", serviceFilePath) | ||
} | ||
|
||
|
@@ -143,17 +143,21 @@ func Install(logger logging.Logger) error { | |
} | ||
} | ||
|
||
logger.Infof("enabling systemd viam-agent service") | ||
cmd = exec.Command("systemctl", "daemon-reload") | ||
output, err = cmd.CombinedOutput() | ||
if err != nil { | ||
return errw.Wrapf(err, "running 'systemctl daemon-reload' output: %s", output) | ||
if newFile { | ||
cmd = exec.Command("systemctl", "daemon-reload") | ||
output, err = cmd.CombinedOutput() | ||
if err != nil { | ||
return errw.Wrapf(err, "running 'systemctl daemon-reload' output: %s", output) | ||
} | ||
} | ||
|
||
cmd = exec.Command("systemctl", "enable", "viam-agent") | ||
output, err = cmd.CombinedOutput() | ||
if err != nil { | ||
return errw.Wrapf(err, "running 'systemctl enable viam-agent' output: %s", output) | ||
if newInstall { | ||
logger.Infof("enabling systemd viam-agent service") | ||
cmd = exec.Command("systemctl", "enable", "viam-agent") | ||
output, err = cmd.CombinedOutput() | ||
if err != nil { | ||
return errw.Wrapf(err, "running 'systemctl enable viam-agent' output: %s", output) | ||
} | ||
} | ||
|
||
_, err = os.Stat("/etc/viam.json") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks