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

Upgraded to a version of etcd compatible with newer grpc #2376

Merged
merged 5 commits into from
Mar 2, 2021

Conversation

mhutchinson
Copy link
Contributor

Trillian has been stuck on grpc < 1.30 - #2195. This uses the v3 version of etcd which isn't using the experimental (and now deleted) grpc APIs

I've deleted the prometheus etcdiscover tool in this commit. This can be re-added if we believe it's used, and we can find a way to test it. The upgrade path for this looks non-trivial.

Checklist

Trillian has been stuck on grpc < 1.30 - google#2195. This uses the v3 version of etcd which isn't using the experimental (and now deleted) grpc APIs

I've deleted the prometheus etcdiscover tool in this commit. This can be re-added if we believe it's used, and we can find a way to test it. The upgrade path for this looks non-trivial.
@mhutchinson mhutchinson requested a review from pav-kv March 1, 2021 16:35
@mhutchinson mhutchinson requested a review from a team as a code owner March 1, 2021 16:35
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #2376 (cc3b499) into master (f5dc80c) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2376      +/-   ##
==========================================
+ Coverage   65.94%   65.99%   +0.05%     
==========================================
  Files         108      108              
  Lines        7869     7869              
==========================================
+ Hits         5189     5193       +4     
+ Misses       2134     2130       -4     
  Partials      546      546              
Impacted Files Coverage Δ
quota/etcd/etcdqm/etcdqm.go 88.23% <ø> (ø)
quota/etcd/quotaapi/quota_server.go 92.66% <ø> (ø)
quota/etcd/storage/quota_storage.go 82.41% <ø> (ø)
util/election2/etcd/election.go 71.92% <ø> (ø)
log/operation_manager.go 89.41% <0.00%> (+2.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5dc80c...cc3b499. Read the comment docs.

@pav-kv
Copy link
Contributor

pav-kv commented Mar 1, 2021

"integration_etcd" step fails because of grpc/naming. Is this fixable?

// See the License for the specific language governing permissions and
// limitations under the License.

// The etcdiscover binary monitors etcd to track the set of instances that
Copy link
Contributor

@pav-kv pav-kv Mar 1, 2021

Choose a reason for hiding this comment

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

I can't remember why this tool was made. Some CT operators use Prometheus (Let's Encrypt, as per #2105).
@pgporada Could you confirm you don't use this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not run etcdiscover. In our environment we have kubernetes prometheus-operator find everything with some specific etcd label. Thank you for asking and looking out for us!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems promising then. I thought we might need this tool but was just deleting it to test whether the rest of the changes would fly first. If we don't know of any users, then maybe deleting it and then allowing it to be resurrected by someone with motivation & knowledge isn't a bad call.

@Martin2112 @AlCutter ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was definitely used at one point to hook up monitoring. I thought it was used in our cloud deployment as well. Maybe things have changed under it and it no longer is.

monitoring/prometheus/etcdiscover/main.go Show resolved Hide resolved
@pav-kv
Copy link
Contributor

pav-kv commented Mar 2, 2021

After submitting, could you please check that Trillian&etcd can be upgraded in ct-go accordingly? There is a dependency if I remember correctly.

@pav-kv
Copy link
Contributor

pav-kv commented Mar 2, 2021

Also, please indicate the etcd&grpc upgrade in the CHANGELOG.

@mhutchinson mhutchinson merged commit 074ffbb into google:master Mar 2, 2021
@mhutchinson mhutchinson deleted the etcdup branch March 2, 2021 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants