Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added: reload server config by sending it a message #4307

Merged
merged 8 commits into from Jul 26, 2023
Merged

Added: reload server config by sending it a message #4307

merged 8 commits into from Jul 26, 2023

Conversation

levb
Copy link
Contributor

@levb levb commented Jul 13, 2023 •
edited

  • Link to issue, e.g. Resolves #NNN
  • Documentation added (if applicable)
  • Tests added
  • Branch rebased on top of current main (git pull --rebase origin main)
  • Changes squashed to a single commit (described here)
  • Build is green in Travis CI
  • You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #

Changes proposed in this pull request:

  • Reload server's configuration by sending a message to $SYS.REQ.SERVER.{server-id}.RELOAD

@levb levb requested a review from neilalexander July 13, 2023 22:53
@neilalexander
Copy link
Member

Just as a heads up, CI build phase is red because of server/events_test.go:2586: File is not gofmt-ed with -s (gofmt).

Looks like the right direction for sure, not sure what makes sense for a return value here. Maybe Derek has an opinion.

Can you please extend the test to also check that a non-system account can't hit this?

@derekcollison
Copy link
Member

I will take a look this weekend.

Definitely get your editor setup to auto run go-fmt on save for sure.

@levb
Copy link
Contributor Author

levb commented Jul 15, 2023

oops about gofmt, @neilalexander will add the test, there's probably one already for sysSubscribe, will look.

@derekcollison
Copy link
Member

Should return the error of reload to the caller.

@levb
Copy link
Contributor Author

levb commented Jul 17, 2023

@derekcollison it's Reload() error, so nil then (for the api response .Data)

@levb
Copy link
Contributor Author

levb commented Jul 17, 2023

@neilalexander I added a test to ensure that a regular user can not reload. The failure is kinda silent (NextMsg fails with "no responder available"), not sure if it's something that we'd care to change and how to go about it. For other (monitoring) APIs it's appropriate.

@levb levb marked this pull request as ready for review July 17, 2023 16:38
@levb levb requested a review from a team as a code owner July 17, 2023 16:38
@derekcollison derekcollison self-requested a review July 17, 2023 17:04
derekcollison
derekcollison reviewed Jul 17, 2023
View reviewed changes
server/events.go Show resolved Hide resolved
server/events_test.go Outdated Show resolved Hide resolved
server/events.go Show resolved Hide resolved
server/events_test.go Outdated Show resolved Hide resolved
@levb
Copy link
Contributor Author

levb commented Jul 17, 2023

@derekcollison @neilalexander what should I do for documenting this?

@bruth
Copy link
Member

bruth commented Jul 17, 2023

@levb I will handle documentation and ask questions if I have any.

Also, in case there are more changes coming.. if you rebase against main then you will only have the PR Travis build now 😄

@levb levb marked this pull request as draft July 19, 2023 12:48
@levb levb marked this pull request as ready for review July 20, 2023 12:07
@levb levb requested a review from derekcollison July 20, 2023 12:07
@levb
Copy link
Contributor Author

levb commented Jul 20, 2023

The consistent test failures were (both) caused by test data mismatches; fixed now. It still took a few runs to turn green, but now no consistent failures, so I think I got them all.

derekcollison
derekcollison requested changes Jul 22, 2023
View reviewed changes
server/events_test.go Show resolved Hide resolved
server/events_test.go Outdated Show resolved Hide resolved
@levb levb requested a review from derekcollison July 24, 2023 18:50
derekcollison
derekcollison approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - One minor question

server/events_test.go Outdated Show resolved Hide resolved
ripienaar
ripienaar requested changes Jul 26, 2023
View reviewed changes
server/events.go Outdated
// Listen for requests to reload the server configuration.
subject = fmt.Sprintf(serverReloadReqSubj, s.info.ID)
if _, err := s.sysSubscribe(subject, s.noInlineCallback(s.reloadConfig)); err != nil {
s.Errorf("Error setting up internal tracking: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this error log can probably be improved, I dont think internal tracking will tell people what this is about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ripienaar thx for catching the incomplete cut&paste, improved the message.

@levb levb requested a review from ripienaar July 26, 2023 15:49
ripienaar
ripienaar approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@derekcollison derekcollison merged commit f4f5a84 into dev Jul 26, 2023
2 checks passed
@derekcollison derekcollison deleted the lev-reload-by-message branch July 26, 2023 23:34
@jarretlavallee jarretlavallee mentioned this pull request Dec 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ripienaar ripienaar ripienaar approved these changes

@derekcollison derekcollison derekcollison approved these changes

@neilalexander neilalexander Awaiting requested review from neilalexander

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@levb @neilalexander @derekcollison @bruth @ripienaar