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

Automated cherry pick of #11194 on release-3.4 #11200

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Oct 3, 2019

Cherry pick of #11194 on release-3.4.

#11194: etcdctl: fix member add command

@jingyih jingyih changed the title Automated cherry pick of #11194 Automated cherry pick of #11194 on release-3.4 Oct 3, 2019
@gyuho
Copy link
Contributor

gyuho commented Oct 3, 2019

Hmm, are test failures related? Is your 3.4 branch latest?

@jingyih
Copy link
Contributor Author

jingyih commented Oct 3, 2019

Integration test failure is not related:

--- FAIL: TestBalancerUnderNetworkPartitionWatchFollower (3.27s)
    network_partition_test.go:266: took too long to detect leader lost

Haven't seen this e2e test failure before:

=== RUN   TestV3MetricsInsecure
--- FAIL: TestV3MetricsInsecure (0.94s)

=== RUN   TestV3MetricsSecure
--- FAIL: TestV3MetricsSecure (1.62s)

@jingyih
Copy link
Contributor Author

jingyih commented Oct 3, 2019

e2e tests also failed for recent commits on release-3.4. Let me take a look.

@gyuho
Copy link
Contributor

gyuho commented Oct 3, 2019

Ah, my commit should have broke it. Can we update this in 3.4 branch? https://github.com/etcd-io/etcd/blob/master/tests/semaphore.test.bash#L17

Try version 3.4.1?

@jingyih
Copy link
Contributor Author

jingyih commented Oct 3, 2019

Ah, my commit should have broke it. Can we update this in 3.4 branch? https://github.com/etcd-io/etcd/blob/master/tests/semaphore.test.bash#L17

With this version override, cluster version test fails because in the test we do not trim ver

{"/metrics", fmt.Sprintf(`etcd_cluster_version{cluster_version="%s"} 1`, ver)},

@gyuho
Copy link
Contributor

gyuho commented Oct 3, 2019

right, let me quick-fix

@gyuho
Copy link
Contributor

gyuho commented Oct 3, 2019

Should be fixed via c91a6bf.

@gyuho gyuho merged commit dae0a72 into etcd-io:release-3.4 Oct 3, 2019
@jingyih
Copy link
Contributor Author

jingyih commented Oct 3, 2019

Should be fixed via c91a6bf.

I actually found some inconsistency in our code base. Should the cluster version be "3.4" or 3.4.0"?

In some places cluster version is trimmed via:

func Cluster(v string) string {

Sometimes (for example in metrics endpoint) we keep the patch version:

ClusterVersionMetrics.With(prometheus.Labels{"cluster_version": ver.String()}).Set(1)

@gyuho
Copy link
Contributor

gyuho commented Oct 4, 2019

The patch version is truncated because the metrics is "cluster_version". Maybe we can just strip out the patch version for this metrics (e.g. version.Cluster(ver.String()))? It's pretty confusing right now :0

@jingyih
Copy link
Contributor Author

jingyih commented Oct 4, 2019

Sounds good. Let's use "major.minor" for the "cluster_version" metrics.

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

Successfully merging this pull request may close these issues.

2 participants