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

Update Spec to include ServerToAgentCommand #64

Merged
merged 9 commits into from
Mar 22, 2022

Conversation

dsvanlani
Copy link
Contributor

@dsvanlani dsvanlani commented Feb 22, 2022

Resolves #22

  • Add ServerToAgentCommand into ServerToAgentMessage.
  • Documents ServerToAgentCommand

Verified

This commit was signed with the committer’s verified signature.
virajjasani Viraj Jasani
* Updated ServerToAgent message protobuf, ServerCapabilities enum

* Formatting typo, fixed AgentCapabilites, closed open questions

* Documented entire ServerToAgentCommand message

* Update specification.md

Co-authored-by: Andy Keller <andykellr@users.noreply.github.com>

Co-authored-by: Andy Keller <andykellr@users.noreply.github.com>
@dsvanlani dsvanlani requested a review from a team February 22, 2022 16:40
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 22, 2022

CLA Signed

The committers are authorized under a signed CLA.

specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
@@ -2423,7 +2453,7 @@ TBD
* Do we need to define OpenTelemetry semantic conventions for reporting typical
collection Agent-specific metrics (e.g. input/processing/output data rates,
throughput, latency, etc)?
* Do we need a capability for the Server to order the Agent to restart?
* ~~Do we need a capability for the Server to order the Agent to restart?~~ Added.
Copy link
Member

Choose a reason for hiding this comment

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

👍

specification.md Outdated
Comment on lines 368 to 384
```protobuf
// ServerToAgentCommand is sent from the server to the agent to request that the agent
// perform a command.
message ServerToAgentCommand {
enum CommandType {
// The agent should restart. This request will be ignored if the agent does not
// support restart.
Restart = 0;

// The agent should shutdown. This request will be ignored if the agent does not
// support shutdown. Shutdown is permanent and the agent will no longer be running
// or connected to the management server.
Shutdown = 1;
}
CommandType type = 1;
}
```
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to a separate section like all other messages are?

dsvanlani and others added 2 commits February 24, 2022 16:09
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@tigrannajaryan
Copy link
Member

@open-telemetry/opamp-spec-approvers please review.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

specification.md Outdated
// The agent should shutdown. This request will be ignored if the agent does not
// support shutdown. Shutdown is permanent and the agent will no longer be running
// or connected to the management server.
Shutdown = 1;
Copy link
Member

Choose a reason for hiding this comment

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

I thought a bit more about this and it is not quite clear how this can be implemented by an Agent. Typically the Agent is installed in a way that autostarts it on system start, right? So do we expect the shutdown to disable the autostart? Is this even doable in typical use-cases when the user that the Agent runs with is not the root?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually when there is a working supervisor connected to OpAMP, this could signal that the collector should stop and free up some resources, leaving only the supervisor running. This could be used to pause collection of metrics without modifying the configuration. To resume collection of metrics, a restart command would start the collector.

@tigrannajaryan
Copy link
Member

@dsvanlani Discussed this in the workgroup. There does not seem to be a consensus around what it means to "shutdown" an Agent. Can you clarify the semantics of the operation or perhaps remove it from this PR, so that we can merge "restart" command quickly and continue thinking about shutdown in a follow up PR.

@dsvanlani
Copy link
Contributor Author

@tigrannajaryan I removed the "shutdown" command from this PR. Happy to wait for further discussion.

@tigrannajaryan
Copy link
Member

@dsvanlani please rebase from main and resolve the conflicts so that I can merge.

dsvanlani and others added 3 commits March 22, 2022 10:54
* Add support for detached signatures (open-telemetry#69)

Resolves open-telemetry#65

- Added signature field to DownloadableFile message.
- Added a short explanation about how the signature field can be used
  for detached signatures.

* Set instance_uid by Server on conflict or request for generation (open-telemetry#63)

This is a first approach towards open-telemetry#56 

It assumes that Agent's `instance_uid` can always be updated by Server (since resolving potential issues with conflicts is important).

There's a gap with multiplexed websocket connections which perhaps could be handled by adding Agent capability flag indicating if overrides can be accepted or not

Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Przemek Maciolek <58699843+pmm-sumo@users.noreply.github.com>
@dsvanlani
Copy link
Contributor Author

@tigrannajaryan I went ahead and rebased to cleanup the commit history - I'm not sure why the UI isn't reflecting the single commit.

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

Successfully merging this pull request may close these issues.

Remote Agent restart capability
4 participants