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 to 3.2.8 #16730

Merged
merged 5 commits into from
Oct 12, 2017
Merged

bump etcd to 3.2.8 #16730

merged 5 commits into from
Oct 12, 2017

Conversation

soltysh
Copy link
Member

@soltysh soltysh commented Oct 6, 2017

This bumps etcd and performs additional cleanups needed for our restoration to work properly.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 6, 2017
@liggitt
Copy link
Contributor

liggitt commented Oct 7, 2017

looks like the v2http package was renamed and is failing tests and build

@liggitt
Copy link
Contributor

liggitt commented Oct 7, 2017

at least, the part we use for NewPeerHandler (etcd-io/etcd@v3.2.1...v3.2.8#diff-d7a47eb75475dba263540eb7f8456e50L152)

@liggitt
Copy link
Contributor

liggitt commented Oct 7, 2017

the etcd bump commit is much bigger than the corresponding diff upstream... why is that?

@@ -56,8 +56,7 @@ preload-remote "github.com/openshift" "origin" "github.com/openshift" "origin" #
preload-remote "k8s.io" "kubernetes" "github.com/openshift" "kubernetes"
preload-remote "github.com/docker" "distribution" "github.com/openshift" "docker-distribution"
preload-remote "github.com/skynetservices" "skydns" "github.com/openshift" "skynetservices-skydns"
preload-remote "github.com/coreos" "etcd" "github.com/openshift" "coreos-etcd"
Copy link
Contributor

Choose a reason for hiding this comment

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

@smarterclayton why did we have this?
@soltysh do we no longer have any upstream commits?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we had those some time ago, but I've double checked the last bump and it was using official version (no carry commits). That's why I've decided to remove it from here. We'll bring it back when needed.

@soltysh
Copy link
Member Author

soltysh commented Oct 9, 2017

the etcd bump commit is much bigger than the corresponding diff upstream... why is that?

I'll double check, but I'm guessing we were missing some tests, or alike.

@soltysh
Copy link
Member Author

soltysh commented Oct 10, 2017

the etcd bump commit is much bigger than the corresponding diff upstream... why is that?

I've double checked and the reasons the bump is bigger than the difference are as follows:

  1. Updated tests, a lot of them. I'm guessing previous bump was done without -t flag, which is enabled by default when invoking hack/godep-save.sh which I've used here.
  2. Removed packages (eg. github.com/coreos/etcd/clientv3/namespace and github.com/coreos/etcd/clientv3/naming), looks like we don't need those anymore.
  3. Generated files (eg. github.com/coreos/etcd/etcdserver/etcdserverpb/rpc.pb.gw.go) removed in this bump.

The tests are green on it, but I'll leave that up to you to decide @smarterclayton @liggitt @deads2k

@soltysh
Copy link
Member Author

soltysh commented Oct 11, 2017

@liggitt I've added that no-op bump commit for 3.2.1. With that in the actual 3.2.8 bump is a subset of etcd-io/etcd@v3.2.1...v3.2.8
I wonder though, why we vendor integration tests for etcd, any particular reason, @smarterclayton ?

@smarterclayton
Copy link
Contributor

smarterclayton commented Oct 11, 2017 via email

@liggitt
Copy link
Contributor

liggitt commented Oct 11, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, soltysh

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2017
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 11, 2017

@soltysh: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/experimental/integration 770903e link /test origin-it

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@soltysh
Copy link
Member Author

soltysh commented Oct 11, 2017

We don't need them.

I'll do one another sweep through Godeps and I'll try removing them, in that case.

@liggitt
Copy link
Contributor

liggitt commented Oct 11, 2017

We don't need them.

I'll do one another sweep through Godeps and I'll try removing them, in that case.

was the content of Godeps.json in this PR the result of hack/save-godeps.sh? If so, I wouldn't worry about it for this PR

@openshift-bot
Copy link
Contributor

/retest

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

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@soltysh
Copy link
Member Author

soltysh commented Oct 12, 2017

was the content of Godeps.json in this PR the result of hack/save-godeps.sh? If so, I wouldn't worry about it for this PR

Yes, that's why I think we somehow rely on them. But maybe we don't any more, I'm not sure how good godep is with figuring out those deps.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 3f94f23 into openshift:master Oct 12, 2017
@soltysh soltysh deleted the etcd_bump branch October 12, 2017 14:31
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants