WIP: Use Kubernetes hashutils

[wallrj]

Rather than copying useful code from kubernetes project, we can instead import specific sub-modules from kubernetes, without pulling the entire Kubernetes project into vendor.

But we have to use dep prune to clear out all the unused parts.

dep ensure -v && dep prune -v

The reformatting of Gopkg.lock is an unfortunate side effect of using the latest version of dep.

Release note:

NONE

[jetstack-ci-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: We suggest the following additional approver: wallrj

Assign the PR to them by writing /assign @wallrj in a comment when ready.

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

Needs approval from an approver in each of these OWNERS Files: - **[OWNERS](https://github.com/jetstack/navigator/blob/master/OWNERS)** You can indicate your approval by writing `/approve` in a comment You can cancel your approval by writing `/approve cancel` in a comment

[wallrj]

/test e2e

[jetstack-bot]

@wallrj: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
navigator-quick-verify	fcedfc76cae590c5fc17841a6e073f4fb6c2a9a8	link	/test verify
navigator-e2e-v1-7	fcedfc76cae590c5fc17841a6e073f4fb6c2a9a8	link	/test e2e v1.7
navigator-e2e-v1-8	fcedfc76cae590c5fc17841a6e073f4fb6c2a9a8	link	/test e2e v1.8

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](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://github.com/kubernetes/test-infra/blob/master/commands.md).

[munnerz]

Given the size of this package, what's the need to use the version in k/k given it involves vendoring the entire codebase?

It's worth vendoring large things that may change significantly over time, but this is a small hashing package. Running dep prune takes time, and is not a usual part of our workflow at the moment. Unless there's a significant reason to do so here, I'd rather not accept this PR.

"A little copying is better than a little dependency " - Rob Pike http://go-proverbs.github.io/ On Tue, 2 Jan 2018 at 16:29, Richard Wall notifications@github.com wrote:

/test e2e

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/jetstack/navigator/pull/182#issuecomment-354807103, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMbPyg8nZjSGx6zbxkyy_e3OlYhsQy5ks5tGllRgaJpZM4RQyLc .

[wallrj]

Given the size of this package, what's the need to use the version in k/k given it involves vendoring the entire codebase?

I went looking at ways to create a non-cryptographic hash of the Pilot resource, in response to https://github.com/jetstack/navigator/pull/153#discussion_r156916132

• Initially wrote code to hash the JSON serialised pilot.
• Then looked at ES code and found ComputeNodePoolHash and DeepHashObject.
• ....which looked like Navigator code, but I couldn't understand why it was using spew and why there was an unused collisionCount parameter.
• It wasn't until I realised that this had been copied from Kubernetes that I went and looked at the git history there
• and found they'd avoided using JSON marshalling because it misses out the private attributes of the object being hashed.
• and that the collision count is something to do with Deployment controller bringing up multiple ReplicaSets during a rollout, with pods that have potentially identical hashes.

If DeepCopyObject had instead been imported from Kubernetes, I probably would have used it without bothering investigating....which would have saved some time, but I wouldn't have learned much from it.

It's been interesting to look at the history of DeepHashObject. They've fixed a bunch of bugs since it was written, which, if we had copied the code at an earlier stage, we might affected by:

• https://github.com/kubernetes/kubernetes/issues/6265
• https://github.com/kubernetes/kubernetes/pull/4595
• https://github.com/kubernetes/kubernetes/pull/6681

So that, to me, is the reason that "A little copying" should be avoided, if possible.

And it turns out that the latest dep is super fast. I can run dep ensure && dep prune in less than 30 seconds on my laptop, even when depending on the k8s.io/kubernetes/util/hash package. And the pruning works really well, ensuring that only the hash package files are added to ./vendor

And according to https://github.com/golang/dep/issues/944, pruning will soon be performed by dep ensure so we won't even have to think about it.

I'm not particularly desperate to get this PR merged. But I think it demonstrates that we can re-use code from the kubernetes repo, without having to vendor the entire project.

[munnerz]

I 100% agree that seeing history is useful - we should have a link to the 'source' of the package within that directory, which would have made it much easier for you to find.

I don't however see how if the ES controller used the k/k package already, it would have been easier to discover. On the contrary, it would have put the package you are trying to find (but are not aware of it's existence) further from you (right now it's just in our pkg/ directory).

Regarding bugs - yep agreed. But a link in the README I think would go a long way. dep won't notify us of changes to the upstream package even if we did depend upon it, so it would still have taken some test failures or user reports for us to pick up the bug, which would eventually lead us to our vendored package, at which point we could go look at the recent history and spot the bug there (this is largely the same process by which we'd debug and find a fix if we had depended upon the main upstream repo).

As an aside, some of my other concerns with dep prune are around caching. I will need to dig out the exact issue, but it's already been said that dep prune will invalidate dep verify style commands (which ensure vendor is in sync with Gopkg), which (to me) is a shame.

My tl;dr on this is that I think an additional file in this package that references it's source would suffice, unless there's something else I'm missing here 😄

[jetstack-ci-bot]

@wallrj PR needs rebase

[wallrj]

dep ensure now prunes by default

Let's revisit this when the 0.4.0 release is ready.