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.

Sign up for GitHub

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

Remove explicit usage of etcd v2 (api and storage) #13791

Merged
merged 10 commits into from
Aug 22, 2023
Merged

Remove explicit usage of etcd v2 (api and storage) #13791

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6169cbb
Remove explicit usage of etcd v2 (api and storage)
mattlord Aug 15, 2023
3f518bf
Modify topo "dir" (not really, at all) handling
mattlord Aug 15, 2023
8e08725
Use new v3 health http endpoint
mattlord Aug 15, 2023
1cf8f1b
More tweaks and fixes
mattlord Aug 15, 2023
166d0d2
Use proper go client v3 API
mattlord Aug 15, 2023
99d75c5
Reuse topo client in endtoend tests
mattlord Aug 15, 2023
decf5a2
Merge remote-tracking branch 'origin/main' into etcd_v3
mattlord Aug 16, 2023
2287b78
Setup the etcd v3 client when etcd is used :D
mattlord Aug 16, 2023
6c7f195
Move client close to beginning of teardown
mattlord Aug 16, 2023
9453af7
Add note to v18 summary
mattlord Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions examples/common/scripts/etcd-up.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,15 @@
source "$(dirname "${BASH_SOURCE[0]:-$0}")/../env.sh"

cell=${CELL:-'test'}
export ETCDCTL_API=2

# Check that etcd is not already running
curl "http://${ETCD_SERVER}" > /dev/null 2>&1 && fail "etcd is already running. Exiting."

etcd --enable-v2=true --data-dir "${VTDATAROOT}/etcd/" --listen-client-urls "http://${ETCD_SERVER}" --advertise-client-urls "http://${ETCD_SERVER}" > "${VTDATAROOT}"/tmp/etcd.out 2>&1 &
etcd --data-dir "${VTDATAROOT}/etcd/" --listen-client-urls "http://${ETCD_SERVER}" --advertise-client-urls "http://${ETCD_SERVER}" > "${VTDATAROOT}"/tmp/etcd.out 2>&1 &
PID=$!
echo $PID > "${VTDATAROOT}/tmp/etcd.pid"
sleep 5

echo "add /vitess/global"
etcdctl --endpoints "http://${ETCD_SERVER}" mkdir /vitess/global &

echo "add /vitess/$cell"
etcdctl --endpoints "http://${ETCD_SERVER}" mkdir /vitess/$cell &

# And also add the CellInfo description for the cell.
# If the node already exists, it's fine, means we used existing data.
echo "add $cell CellInfo"
Expand Down
14 changes: 8 additions & 6 deletions go/test/endtoend/cluster/topo_process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ func (topo *TopoProcess) Setup(topoFlavor string, cluster *LocalProcessCluster)
case "consul":
return topo.SetupConsul(cluster)
default:
// We still rely on the etcd v2 API for things like mkdir.
// If this ENV var is not set then some tests may fail with etcd 3.4+
// where the v2 API is disabled by default in both the client and server.
os.Setenv("ETCDCTL_API", "2")
return topo.SetupEtcd()
}
}
Expand All @@ -77,7 +73,6 @@ func (topo *TopoProcess) SetupEtcd() (err error) {
"--initial-advertise-peer-urls", topo.PeerURL,
"--listen-peer-urls", topo.PeerURL,
"--initial-cluster", fmt.Sprintf("%s=%s", topo.Name, topo.PeerURL),
"--enable-v2=true",
)

err = createDirectory(topo.DataDirectory, 0700)
Expand Down Expand Up @@ -324,6 +319,9 @@ func (topo *TopoProcess) ManageTopoDir(command string, directory string) (err er
url := topo.VerifyURL + directory
payload := strings.NewReader(`{"dir":"true"}`)
if command == "mkdir" {
if *topoFlavor == "etcd2" { // No need to create the empty prefix key in v3
Copy link
Contributor

@dbussink dbussink Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Given the comment for ManageTopoDir, don't we only use this function for etcd anyway? And can we remove any callers that do mkdir then since that seems superfluous then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's conflicting signals here. It seems like this was at least intended to be used with multiple topo flavors, but then in certain places it assumes etcd2. I'd prefer to keep it generic so that we can leverage that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's test code, so we can keep it like this but it seems surprising that a generic HTTP call would work across different flavors here really, so it's probably etcd specific anyway? I doubt we run these endtoend tests with anything but etcd?

Copy link
Contributor Author

@mattlord mattlord Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The topo.VerifyURL used is different based on the flavor. We do have zookeeper and consul endtoendtests:

test/config.json:               "tabletmanager_zk2": {
test/config.json:                               "vitess.io/vitess/go/test/endtoend/tabletmanager","--topo-flavor=zk2"
test/config.json:               "topo_zk2": {
test/config.json:                       "Args": ["vitess.io/vitess/go/test/endtoend/topotest/zk2", "--topo-flavor=zk2"],

test/config.json:               "tabletmanager_consul": {
test/config.json:                               "vitess.io/vitess/go/test/endtoend/tabletmanager","--topo-flavor=consul"
test/config.json:                       "Shard": "tabletmanager_consul",
test/config.json:               "topo_consul": {
test/config.json:                       "Args": ["vitess.io/vitess/go/test/endtoend/topotest/consul", "--topo-flavor=consul"],
test/config.json:                       "Shard": "vtgate_topo_consul",

And both do use the same cluster package and its topo process:
https://github.com/vitessio/vitess/blob/main/go/test/endtoend/topotest/zk2/main_test.go#L68-L75

https://github.com/vitessio/vitess/blob/main/go/test/endtoend/topotest/consul/main_test.go#L68-L75

return nil
}
req, _ := http.NewRequest("PUT", url, payload)
req.Header.Add("content-type", "application/json")
resp, err := http.DefaultClient.Do(req)
Expand All @@ -332,6 +330,10 @@ func (topo *TopoProcess) ManageTopoDir(command string, directory string) (err er
}
return err
} else if command == "rmdir" {
if *topoFlavor == "etcd2" { // Use etcdctl as the gRPC gateway
cmd := exec.Command("etcdctl", "del", "--prefix", directory)
return cmd.Run()
}
req, _ := http.NewRequest("DELETE", url+"?dir=true", payload)
resp, err := http.DefaultClient.Do(req)
if err == nil {
Expand Down Expand Up @@ -366,7 +368,7 @@ func TopoProcessInstance(port int, peerPort int, hostname string, flavor string,
topo.ListenClientURL = fmt.Sprintf("http://%s:%d", topo.Host, topo.Port)
topo.DataDirectory = path.Join(os.Getenv("VTDATAROOT"), fmt.Sprintf("%s_%d", "topo", port))
topo.LogDirectory = path.Join(os.Getenv("VTDATAROOT"), fmt.Sprintf("%s_%d", "topo", port), "logs")
topo.VerifyURL = fmt.Sprintf("http://%s:%d/v2/keys", topo.Host, topo.Port)
topo.VerifyURL = fmt.Sprintf("http://%s:%d/health", topo.Host, topo.Port)
topo.PeerURL = fmt.Sprintf("http://%s:%d", hostname, peerPort)
return topo
}