Add V1 controller and spec support to the shipper #59
Conversation
fearful-symmetry
added
Team:Elastic-Agent-Data-Plane
fearful-symmetry
requested review from
kvch and
faec
and removed request for
a team
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
For clarification on the plan with the shipper integrating with the different control protocol versions:
|
| &devtools.BuildVariableSources{ | ||
| BeatVersion: "./version/version.go", | ||
| GoVersion: ".go-version", | ||
| DocBranch: "./docs/version.asciidoc", |
There was a problem hiding this comment.
I wouldn't think we need a doc version, because there are no docs here.
Do we need this with V2 as well? If so we should just modify the tooling to make this optional.
There was a problem hiding this comment.
These are all required by the Package target, and it looks like we blindly copied them all into elastic-agent from beats. Currently unsure if it's worth it to make our own mage package target separate from beats and agent in the scope of this, since the build system is kind of a snarl. The good news is that the V2 build system removes most of this cruft, so I'm kind of mentally treating this as a temporary holdover until then.
| // OnConfig is called by the agent on a requested config change | ||
| func (client *AgentClient) OnConfig(in string) { | ||
| client.log.Infof("Got config update, (re)starting the shipper") | ||
| _ = client.client.Status(proto.StateObserved_CONFIGURING, "configuring from OnConfig", nil) |
There was a problem hiding this comment.
Assuming this is an RPC call, I'm curious what state the system is left in if this fails. Do we just get this again? Is the behavior more obvious in V2?
There was a problem hiding this comment.
Yah, status calls are more organized in V2. None of the beats V1 code checks the error, and I'm kind of operating under the assumption that if the RPC fails, we have bigger problems than updating the status.
|
Thanks for making this work! Unless we decide to promote this to main I am hesitant to review it too thoroughly. I am interested to see what this looks like under V2 as there seems like lots of tricky things that can happen around Start/Stop calls and configuration reloading. |
server/server.go
Outdated
| serv.grpcServer.GracefulStop() | ||
| serv.queue.Close() | ||
| // Don't try to gracefully stop monitoring until we deal with expvar | ||
| //s.monHandler.End() |
There was a problem hiding this comment.
is this still pending? It's not obvious to me from the comment what still needs to happen for this line to be activated
There was a problem hiding this comment.
Yah, I think we might just want to remove the expvar End() for now, since there isn't really an easy workaround for it.
This adds support for the V1 build system and controller into the shipper. This is a breaking change that will live on its own branch, and it changes the config and startup so the shipper can only run under agent, as I assume there's no standalone use case for this.
There's two parts to this:
agentcan runmage package. There seems to be a lot of cruft and legacy tooling in thepackagetargets that require a variety of extra files, targets, etc, and we may want to clean those up at a later date.TODO: