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 option to run vtcombo and vttest local cluster with real topo server #9176

Merged
merged 4 commits into from
Nov 17, 2021

Conversation

zhongr3n
Copy link
Contributor

@zhongr3n zhongr3n commented Nov 9, 2021

Signed-off-by: Zhong Ren zhong@zhongren.me

Description

Add option to run vtcombo and vttest local cluster with real topo server

  • We want to share the topo server across multiple components for local functional tests. Because vtcombo starts its own process, there's no easy way to share the in-memory topo server across different components
  • This change allow vtcombo to use an actual topo server endpoint
  • Tested with local consul server
consul agent -dev

go run ~/stripe/vitess/go/cmd/vttestserver/main.go -remote_topo_implementation=consul -remote_topo_global_server_address=localhost:8500 -remote_topo_global_root=root

Related Issue(s)

N/A

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

N/A

@mattlord mattlord added Component: vttestserver Type: Enhancement Logical improvement (somewhere between a bug and feature) release notes labels Nov 9, 2021
Copy link
Contributor

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! I can definitely see how this will be useful. I could put it to good use myself for some consul specific testing :-)

I had a few nits, but all very minor things. Do you think it's worth adding a unit test to be sure this doesn't break down the road?

go/cmd/vtcombo/main.go Outdated Show resolved Hide resolved
go/cmd/vtcombo/main.go Show resolved Hide resolved
go/vt/vttest/local_cluster.go Outdated Show resolved Hide resolved
go/vt/vttest/local_cluster.go Outdated Show resolved Hide resolved
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Looks good overall.
I might be reading this wrong, but I don't see any actual tests exercising the new options. We need at least one.

@zhongr3n
Copy link
Contributor Author

Thanks for reviewing folks. Added a unit test using consul external topo, and addressed other feedback.

go/vt/topo/test/testing.go Outdated Show resolved Hide resolved
cmd, configFilename, serverAddr := test.StartConsul(t, "")
defer func() {
// Alerts command did not run successful
if err := cmd.Process.Kill(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

if we use consul agent -dev i think we could do consul leave for a graceful shutdown and don't need to call os.Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried consul leave and it adds 10 seconds for grace shutdown. keeping process.kill

Signed-off-by: Zhong Ren <zhong@zhongren.me>
@zhongr3n
Copy link
Contributor Author

looking into test failure

Signed-off-by: Zhong Ren <zhong@zhongren.me>
@5antelope
Copy link
Member

LGTM

@deepthi deepthi merged commit cd7bcfb into vitessio:main Nov 17, 2021
@deepthi
Copy link
Member

deepthi commented Nov 17, 2021

Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: vttestserver Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants