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.

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

Add systemd notify support to Agent. Resolves: #7028 #9802

Merged
merged 5 commits into from Aug 29, 2022

Conversation

aphorise
Copy link
Contributor

Not sure if there is a more elegant way of reading Systemd configuration so as to only display message conditional to Vault unit file reads:

[Service]
Type=notify

command/agent.go Outdated Show resolved Hide resolved
@kalafut kalafut removed their request for review September 4, 2020 04:32
}

// Notify systemd that the server is ready (if applicable)
c.notifySystemd(systemd.SdNotifyReady)
Copy link
Contributor Author

@aphorise aphorise Sep 14, 2021

Choose a reason for hiding this comment

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

I tried adding this a bit later after the run condition which may be more valid to NotifyReady as and when it's actually ready - however since the time it may take for that is not as easily determinable then getting an earlier start notification coupled with any follow-up stop is probably better than just getting NotifyStopping without ever seeing NotifyReady

@ncabatoff
Copy link
Contributor

Changes seem fine. Since this isn't practical to test using Go tests, can you summarize what manual testing you did?

@aphorise
Copy link
Contributor Author

aphorise commented Nov 24, 2021

@ncabatoff thank you for the review.

The manual tests performed were to provision a Vault-agent and to confirm the expected emittance - observing the following tests:

  1. Vault agent being started in a valid / proper sense where only NotifyReady is expected.
  2. Vault agent - later being stopped where only NotifyStopping is received.
  3. Vault agent being started / with some erroneous condition where it emits NotifyReady attempting to progress as far as possible before crashing / stopping when NotifyStopping is then emitted.

@ncabatoff
Copy link
Contributor

Thanks! Could you merge in latest main please?

@jdoss
Copy link

jdoss commented May 3, 2022

Any chance to get this rebased and merged?

@ncabatoff ncabatoff changed the title Server systemd notify added similar to Consul. Resolves: #7028 Add systemd notify support to Agent. Resolves: #7028 May 3, 2022
@ncabatoff
Copy link
Contributor

Hi @aphorise, sorry I forgot about this. Could you merge in main again please, then I'll merge this?

@aphorise
Copy link
Contributor Author

aphorise commented Jul 1, 2022

hey @ncabatoff done - hope this is okay. Thanks.

@aphorise
Copy link
Contributor Author

@ncabatoff will this make it into the next release?

@aphorise aphorise requested review from mladlow and calvn August 15, 2022 13:49
// Let the lease cache know this is a shutdown; no need to evict
// everything
if leaseCache != nil {
leaseCache.SetShuttingDown(true)
}
return nil
case <-ctx.Done():
c.notifySystemd(systemd.SdNotifyStopping)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also do this in the case above, when we were asked to shutdown?

Copy link
Contributor Author

@aphorise aphorise Aug 24, 2022

Choose a reason for hiding this comment

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

hey @ncabatoff I'm not comprehending - does systemd.SdNotifyStopping need to be included in any other case other than these two places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If any one of the two signals are the only mode of exit (outside Windows) then I think these two later here would suffice and void any repeats - but I'm not entirely sure TBH.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's simpler just to add the notifySystemd call to the end of the func, or to a defer.

Copy link
Contributor Author

@aphorise aphorise Aug 24, 2022

Choose a reason for hiding this comment

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

Is there any chance that an end or defer function may take longer or not ever reached? - just thinking aloud in case of processor monitors or other cases an agent may be forced quit and never actually ending.

Anyway @ncabatoff any chance you can do a PR if you think a defer may be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Defer will always fire when the func returns, and it shouldn't be any slower than doing it just before a return statement.

@ncabatoff
Copy link
Contributor

Since the tests weren't running in the PR I ran agent_test locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants