Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

WIP: Use Kubernetes hashutils #182

Closed
wants to merge 3 commits into from
Closed

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Jan 2, 2018

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
Copy link
Contributor

[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:

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

@wallrj
Copy link
Member Author

wallrj commented Jan 2, 2018

/test e2e

@jetstack-bot
Copy link
Collaborator

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

Test name Commit Details Rerun command
navigator-quick-verify fcedfc7 link /test verify
navigator-e2e-v1-7 fcedfc7 link /test e2e v1.7
navigator-e2e-v1-8 fcedfc7 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. 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.

@wallrj wallrj changed the title WIP: Use Kubernetes hashutils Use Kubernetes hashutils Jan 2, 2018
@munnerz
Copy link
Contributor

munnerz commented Jan 2, 2018 via email

@wallrj
Copy link
Member Author

wallrj commented Jan 3, 2018

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 #153 (comment)

  • 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:

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 golang/dep#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
Copy link
Contributor

munnerz commented Jan 8, 2018

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
Copy link
Contributor

@wallrj PR needs rebase

@wallrj
Copy link
Member Author

wallrj commented Jan 24, 2018

dep ensure now prunes by default

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

@wallrj wallrj changed the title Use Kubernetes hashutils WIP: Use Kubernetes hashutils Jan 24, 2018
@munnerz munnerz closed this Mar 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants