Run integration tests against full agent & proxy-server apps #536
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XL
tallclair
force-pushed
the
full-agent-testing
branch
from
3a92b02
to
ad15e61
Compare
|
Opened #538 for test error |
|
/test pull-apiserver-network-proxy-docker-build-amd64 |
I believe fixed with kubernetes/test-infra#31196 |
|
/test pull-apiserver-network-proxy-docker-build-arm64 |
|
/assign |
|
/assign @cheftako |
|
/approve |
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkh52, tallclair The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
| @@ -49,28 +49,32 @@ func NewAgentCommand(a *Agent, o *options.GrpcProxyAgentOptions) *cobra.Command | |||
| Use: "agent", | |||
| Long: `A gRPC agent, Connects to the proxy and then allows traffic to be forwarded to it.`, | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| return a.run(o) | |||
| stopCh := make(chan struct{}) | |||
There was a problem hiding this comment.
Not against the expanding the scope of the stop channel up the stack. However not seeing the value on moving it only this far? If we moved it up to main(), we could improve the graceful shutdown.
There was a problem hiding this comment.
I moved it to here to make stopCh an argument to Run(...), so that tests can call the Run method and shutdown at the end. How does moving it to main() improve graceful shutdown? Actually, it looks like the server command explicitly installs a signal handler for graceful shutdown. Would it be better to just reuse that code here?
There was a problem hiding this comment.
I started to make the signal handler change, but I'd prefer to make it a separate PR to derisk this PR and keep the non-test changes minimal.
There was a problem hiding this comment.
I agree with separate PR, especially since this commit is so large.
(The server signal handler pattern seems applicable to agent. But I think we should be cautious with making changes.)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
tallclair
force-pushed
the
full-agent-testing
branch
from
ad15e61
to
89dd3d3
Compare
k8s-ci-robot
removed
the
lgtm
|
/hold cancel Feedback addressed, 1 open question |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This PR changes the way the proxy-agent & proxy-server are run in integration tests. Previously, the tests constructed the proxy-server and agents from their internal components connected via channels. Now, the tests set up the full proxy-server & proxy-agent servers, connected over local ports (and unix sockets). This approach gives better end-to-end test coverage, and paves the way to running the test suite against out-of-process servers & agents.
Summary of the specific changes:
framework/proxy_server.goandframework/agent.goare overhauled to run the agent/server by theircmd/{agent,server}/appinvocations.cmd/{agent,server}/appto support in-process execution (mostly improvements to shutdown), and exposes some internal state to read from. Ideally, the state would be read from metrics, but since the metrics are shared globals that doesn't work for multiple in-process agents / servers. Exposing the internal state was a smaller lift.Sorry for the large PR, I didn't see a good way of breaking it down.