-
Notifications
You must be signed in to change notification settings - Fork 56
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
refactor: implement warnings as notices #486
Conversation
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.
LGTM. A few comments. Looking forward to the follow-up review that removes the mutex lock on the daemon response code for warnings.
func (s *State) LatestWarningTime() time.Time { | ||
s.reading() | ||
|
||
// TODO(benhoyt): optimise with an in-memory atomic cache when warnings are added (or pruned), |
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.
Are you going to follow up with this PR soon? It seems like then we can finally get rid of the mutex lock in the daemon response code.
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.
Yes, it's my plan to do a follow-up PR shortly after this one.
RepeatAfter: warningRepeatAfter, | ||
}) | ||
if err != nil { | ||
panic(fmt.Sprintf("internal error: error adding warning notice: %v", err)) |
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 would seem to only panic when the message is an empty string. That seems OK.
} | ||
} | ||
|
||
last, err := lastWarningTimestamp() | ||
err = saveCLIState(cmd.socketPath, state) |
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.
I think, at some point, we need to improve saveCLIState
to atomically write the state file (and to include conflict resolution).
internals/cli/cmd_warnings.go
Outdated
w.Flush() | ||
} | ||
|
||
state.WarningsLastListed = warnings[len(warnings)-1].LastRepeated |
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.
Nice detail.
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.
Some questions/comments on this feature in general after testing:
change-update already existed before this PR.
#17991 Per discussion with @hpidcock, update Juju 3.6 to use the just-released Pebble v1.16.0 ([changelog](https://github.com/canonical/pebble/releases/tag/v1.16.0)). Summary of main changes: * Improvements to how services with dependencies are started and stopped (using "lanes"), so that independent services are started in parallel, and dependent services start up serially. Error handling is also improved. Fix in canonical/pebble#437. * A fix for a tricky bug in pebble exec, so it doesn't lose output in interactive mode. Fix in canonical/pebble#466! prdesc Reduce the size of exec tasks by not storing the execSetup (which includes all environment variables) in state. This speeds up (and shrinks the data) when serialising state to disk during state.Unlock. Fix is in canonical/pebble#478. prdesc Ensure Pebble doesn't hang (with no error message) when the state file directory is read-only or otherwise inaccessible. Fix in canonical/pebble#481. * Re-implement warnings using notices. This was always the intention since the notices feature was added (it was designed as a superset of warnings), but we'd never gotten to it. Lots of nice code deletion in canonical/pebble#486. ## QA steps Deploy a K8s charm, like `snappass-test`, and ensure `pebble version --client` on the workload reports v1.16.0.
This deletes the legacy warnings code and re-implements
pebble warnings
andpebble okay
using notices as warnings (as we originally intended). The/v1/warnings
endpoints have been removed, andpebble warnings
now hits/v1/notices?types=warning
.On every request, instead of returning the count of warnings and the latest timestamp, we only return the timestamp of the latest (most recent last-repeated time). This way the server doesn't have to track which notices have been okayed -- the client can compare against its stored timestamp and see if the server-reported timestamp is later.
So there are a few breaking changes in this PR:
Client.WarningsSummary() (count int, timestamp time.Time)
is nowClient.LatestWarningTime() time.Time
, as there's no count anymoreGET /v1/warnings
(list warnings) is gone:pebble warnings
now usesGET /v1/notices?types=warning
POST /v1/warnings
(okay warnings) are gone:pebble okay
now just updates the local state file, and doesn't use the API/client at allClient.Warnings()
andClient.Okay()
are gone; useClient.Notices()
and maintain local client-side state, respectivelyState.Warnf
is still around, but it's now just a helper that callsState.AddNotice
with the right type (warning) and key (message)Currently Pebble itself doesn't record any warnings, so you can't test easily without making a code change. To perform manual tests, I hacked the API endpoint used by
pebble notify
to recordwarning
notices instead ofcustom
ones:Fixes #475.