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

*: Bump etcd-mixin to 2b79442d8e9f #827

Merged
merged 1 commit into from Jun 26, 2020

Conversation

wking
Copy link
Member

@wking wking commented Jun 25, 2020

Generated with:

$ make jsonnet/vendor  # to install jb
$ jb --version
v0.3.1
$ (cd jsonnet && jb install https://github.com/coreos/etcd/Documentation/etcd-mixin)
GET https://github.com/coreos/etcd/archive/2b79442d8e9fc54b1ac27e7e230ac0e4c132a054.tar.gz 200
$ touch jsonnet/jsonnetfile.lock.json  # recover from any previous flubbed generation which might have left timestamps in place convincing Make it didn't need to rebuild bindata.go
$ make generate-in-docker

This pulls in:

$ git --no-pager log --oneline f1eca4e1fa5de962ff8079af836bb390e88d1f4c..2b79442d8e9fc54b1ac27e7e230ac0e4c132a054 -- Documentation/etcd-mixin
2c4877064 Documentation/etcd-mixin: Use etcd_mvcc_db_total_size_in_bytes metric
68c5f6066 Documentation/etcd-mixin: Set unique UID for Grafana dashboard
322c38e16 Documentation/etcd-mixin: Fix etcdHighNumberOfLeaderChanges (#11448)

The first two of those are etcd-io/etcd#11768. The last is etcd-io/etcd#11448, and as the discussion there points out, the rate>3 approach is effectively "never fire" (because we are unlikely to ever have more than three elections per second).

Generated with:

  $ make jsonnet/vendor  # to install jb
  $ jb --version
  v0.3.1
  $ (cd jsonnet && jb install https://github.com/coreos/etcd/Documentation/etcd-mixin)
  GET https://github.com/coreos/etcd/archive/2b79442d8e9fc54b1ac27e7e230ac0e4c132a054.tar.gz 200
  $ touch jsonnet/jsonnetfile.lock.json  # recover from any previous flubbed generation which might have left timestamps in place convincing Make it didn't need to rebuild bindata.go
  $ make generate-in-docker

This pulls in:

  $ git --no-pager log --oneline f1eca4e1fa5de962ff8079af836bb390e88d1f4c..2b79442d8e9fc54b1ac27e7e230ac0e4c132a054 -- Documentation/etcd-mixin
  2c4877064 Documentation/etcd-mixin: Use etcd_mvcc_db_total_size_in_bytes metric
  68c5f6066 Documentation/etcd-mixin: Set unique UID for Grafana dashboard
  322c38e16 Documentation/etcd-mixin: Fix etcdHighNumberOfLeaderChanges (#11448)

The first two of those are [1].  The last is [2], and as the
discussion there points out, the rate>3 approach is effectively "never
fire" (because we are unlikely to ever have more than three elections
per second).

Also interesting, is that f1eca4e1fa is in etcd's release-3.4 branch,
while I'm pinning the master branch.  The current coreos release-3.4
tip is etcd-io/etcd@31e49a4df30, which has no etcd-mixin changes since
f1eca4e1fa.  My impression is that mixin changes are unlikely to be
backported to release branches, and also unlikely to depend on the
intricacies of the underlying etcd version, so I'm tracking master
instead of release-3.4 in this commit.  The move to f1eca4e1fa had
landed here via 5c251d9 (jsonnet/jsonnetfile.lock.json: jb update,
2020-04-17, openshift#760).

[1]: etcd-io/etcd#11768
[2]: etcd-io/etcd#11448
@wking
Copy link
Member Author

wking commented Jun 25, 2020

Also interesting, is that etcd-io/etcd@f1eca4e1fa is in etcd's release-3.4 branch, while I'm pinning the master branch. The current coreos release-3.4 tip is etcd-io/etcd@31e49a4df30, which has no etcd-mixin changes since etcd-io/etcd@f1eca4e1fa. My impression is that mixin changes are unlikely to be backported to release branches, and also unlikely to depend on the
intricacies of the underlying etcd version, so I'm tracking master instead of release-3.4 in this commit. The move to etcd-io/etcd@f1eca4e1fa had landed here via 5c251d9 (#760). And seems like #760 (accidentally?) rolled back #591, which had picked up the rate -> increase fix.

@brancz
Copy link
Contributor

brancz commented Jun 26, 2020

Do we want to wait for etcd-io/etcd#12080 to land before we move ahead here?

@brancz
Copy link
Contributor

brancz commented Jun 26, 2020

Otherwise this lgtm

@wking
Copy link
Member Author

wking commented Jun 26, 2020

I dunno what the turnaround time for etcd PRs usually is. Waiting a week here is no big deal; if it's expected to take longer, probably merge and I'll file a later follow-up?

@brancz
Copy link
Contributor

brancz commented Jun 26, 2020

If we have green CI here I'm fine with a follow up as well.

/retest

@brancz
Copy link
Contributor

brancz commented Jun 26, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, wking

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 91b6c20 into openshift:master Jun 26, 2020
@wking
Copy link
Member Author

wking commented Jul 8, 2020

Do we want to wait for etcd-io/etcd#12080 to land before we move ahead here?

I've filed #844 to pull that in, now that it's landed upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants