Use fnv.New32a() in hash instead adler32 #40297

resouer · 2017-01-23T15:13:00Z

Benchmark results: #39527

NOTE: I leave GetPodTemplateSpecHash as it is since we have unit test to test its un-normal behaviour.

k8s-reviewable · 2017-01-23T15:13:24Z

This change is

0xmichalis · 2017-01-23T17:19:02Z

pkg/controller/deployment/sync.go

@@ -315,7 +315,7 @@ func (dc *DeploymentController) getNewReplicaSet(deployment *extensions.Deployme

 	// new ReplicaSet does not exist, create one.
 	namespace := deployment.Namespace
-	podTemplateSpecHash := fmt.Sprintf("%d", deploymentutil.GetPodTemplateSpecHash(deployment.Spec.Template))
+	podTemplateSpecHash := fmt.Sprintf("%d", deploymentutil.GetPodTemplateSpecHashFnv(deployment.Spec.Template))


We can't make this change for Deployments without providing a way to migrate old replica sets. I have a WIP open for managing hash migration for Deployments: #38714

ack, I just left pkg/controller/deployment as it is and only update other files.

0xmichalis · 2017-01-23T17:19:29Z

pkg/controller/deployment/util/hash_test.go

@@ -144,7 +144,7 @@ func BenchmarkAdler(b *testing.B) {
 	json.Unmarshal([]byte(podSpec), &spec)

 	for i := 0; i < b.N; i++ {
-		GetPodTemplateSpecHash(spec)
+		GetPodTemplateSpecHashFnv(spec)


This is benchmarking adler so it should stay as is.

0xmichalis · 2017-01-25T10:12:02Z

I am not sure if there are any backwards compatibility concerns for the changes in kubelet and the scheduler. @resouer can you assign the right people to review these changes?

resouer · 2017-01-25T13:16:06Z

Yes, I will include node & scheduler guys in.

resouer · 2017-01-25T13:17:22Z

@Random-Liu @wojtek-t Could you please help review if Is this update is safe from your side?

Random-Liu · 2017-01-25T22:11:37Z

pkg/kubelet/container/helpers.go

@@ -91,7 +91,7 @@ func ShouldContainerBeRestarted(container *v1.Container, pod *v1.Pod, podStatus
 // HashContainer returns the hash of the container. It is used to compare
 // the running container with its desired spec.
 func HashContainer(container *v1.Container) uint64 {
-	hash := adler32.New()
+	hash := fnv.New32a()


IIUC, all containers will be restarted after kubelet live upgrade because of this, I'm not sure whether this is acceptable. At least we should document this.

Luckily we are going to rollout CRI this release, which also requires restarting container. :p

@yujuhong /cc @kubernetes/sig-node-pr-reviews

Users can opt-out CRI for this release, but they can't turn this off though. We should use the new hash for CRI, but the old adler for dockertools. This way, users who don't want to upgrade won't be affected, and new CRI users will get it anyway.

Agree. We can rollout the hash change with CRI, and keep the old users not being affected.

@resouer can you leave out the changes to files in pkg/kubelet. We'll handle it in a separate PR. Thanks!

Created a new PR for this: #40495

Thank. I just updated this patch and left pkg/kubelet as is

resouer · 2017-01-28T09:39:47Z

@Kargakis sig-node has worked out #40495 for pkg/kubelet. And for scheduler part, I am sure this update does not break anything since equivalence cache is still WIP, see #36238

0xmichalis · 2017-01-29T18:03:09Z

LGTM

resouer · 2017-01-30T15:01:12Z

Unit tests needs update, will push a new commit.

0xmichalis · 2017-01-31T08:57:56Z

pkg/util/mount/mount_linux.go

@@ -282,7 +282,7 @@ func readProcMounts(mountFilePath string, out *[]MountPoint) (uint32, error) {
 }

 func readProcMountsFrom(file io.Reader, out *[]MountPoint) (uint32, error) {
-	hash := adler32.New()
+	hash := fnv.New32a()


I guess kubelet is using this utility. Mind explaining what's the impact of this change?

This method is used in kubelet to make sure /proc/mounts is steady.

It reads contents of /proc/mounts for multiple times and hash all results to see if any difference happens. There's no cached value of this hash result and it will only be used to compare with the hash result from same method. So I assume it is safe to make this change.

See: https://github.com/kubernetes/kubernetes/blob/master/pkg/util/mount/mount_linux.go#L252-L271

I see, thanks!

0xmichalis · 2017-02-06T18:24:53Z

/lgtm

0xmichalis · 2017-02-06T18:26:33Z

Btw I have updated the "fixes #issue" in the starting post to a simple reference because that issue is not fixed once this PR merges.

resouer · 2017-02-14T07:16:57Z

ping @lavalamp @timothysc any time to approve this fix?

timothysc · 2017-02-14T15:00:54Z

@Kargakis can you approve this one, it appears this issue is one of the bot-victims.

0xmichalis · 2017-02-14T15:09:05Z

I am not in any of the OWNERS files from where approval is required for this one. Maybe test the bot?

/approve

smarterclayton · 2017-02-14T16:45:37Z

/approve

k8s-github-robot · 2017-02-15T06:06:31Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: kargakis, resouer, smarterclayton

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

~~pkg/OWNERS~~ [smarterclayton]
~~pkg/controller/OWNERS~~ [smarterclayton]
~~plugin/pkg/scheduler/OWNERS~~ [smarterclayton]

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

k8s-github-robot · 2017-02-15T11:25:02Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-02-15T11:25:08Z

Automatic merge from submit-queue (batch tested with PRs 40297, 41285, 41211, 41243, 39735)

k8s-ci-robot · 2017-02-15T12:27:23Z

@resouer: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins verification	`3bdc3f2`	link	`@k8s-bot verify test this`

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.

resouer changed the title ~~Fix hash~~ Use fnv.New32a() in hash instead adler32 Jan 23, 2017

k8s-github-robot assigned bprashanth Jan 23, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Jan 23, 2017

0xmichalis reviewed Jan 23, 2017

View reviewed changes

resouer assigned 0xmichalis and unassigned bprashanth Jan 24, 2017

resouer force-pushed the fix-hash branch from f6b9cda to e374a72 Compare January 24, 2017 02:44

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 24, 2017

resouer added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jan 24, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 24, 2017

resouer assigned Random-Liu and wojtek-t Jan 25, 2017

Random-Liu reviewed Jan 25, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2017

resouer force-pushed the fix-hash branch from e374a72 to f70cae3 Compare January 28, 2017 09:34

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2017

resouer force-pushed the fix-hash branch from f70cae3 to 7d7b776 Compare January 30, 2017 15:13

k8s-github-robot unassigned 0xmichalis and Random-Liu Jan 30, 2017

k8s-github-robot assigned dchen1107 and unassigned wojtek-t Jan 30, 2017

apelisse assigned 0xmichalis, Random-Liu, wojtek-t and davidopp Jan 31, 2017

k8s-github-robot assigned smarterclayton and timothysc Jan 31, 2017

0xmichalis reviewed Jan 31, 2017

View reviewed changes

timothysc removed their assignment Feb 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 14, 2017

Use fnv.New32a() in hash instead adler32

3bdc3f2

resouer force-pushed the fix-hash branch from 7d7b776 to 3bdc3f2 Compare February 15, 2017 06:06

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 15, 2017

resouer added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2017

k8s-github-robot merged commit 438054f into kubernetes:master Feb 15, 2017

resouer deleted the fix-hash branch February 15, 2017 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fnv.New32a() in hash instead adler32 #40297

Use fnv.New32a() in hash instead adler32 #40297

resouer commented Jan 23, 2017 •

edited by 0xmichalis

k8s-reviewable commented Jan 23, 2017

0xmichalis Jan 23, 2017

resouer Jan 24, 2017

0xmichalis Jan 23, 2017

0xmichalis commented Jan 25, 2017

resouer commented Jan 25, 2017

resouer commented Jan 25, 2017

Random-Liu Jan 25, 2017

yujuhong Jan 25, 2017

Random-Liu Jan 25, 2017

yujuhong Jan 25, 2017

yujuhong Jan 25, 2017

resouer Jan 28, 2017

resouer commented Jan 28, 2017 •

edited

0xmichalis commented Jan 29, 2017

resouer commented Jan 30, 2017

0xmichalis Jan 31, 2017

resouer Feb 6, 2017 •

edited

0xmichalis Feb 6, 2017

0xmichalis commented Feb 6, 2017

0xmichalis commented Feb 6, 2017 •

edited

resouer commented Feb 14, 2017

timothysc commented Feb 14, 2017

0xmichalis commented Feb 14, 2017

smarterclayton commented Feb 14, 2017

k8s-github-robot commented Feb 15, 2017

k8s-github-robot commented Feb 15, 2017

k8s-github-robot commented Feb 15, 2017

k8s-ci-robot commented Feb 15, 2017

Use fnv.New32a() in hash instead adler32 #40297

Use fnv.New32a() in hash instead adler32 #40297

Conversation

resouer commented Jan 23, 2017 • edited by 0xmichalis

k8s-reviewable commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jan 25, 2017

resouer commented Jan 25, 2017

resouer commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resouer commented Jan 28, 2017 • edited

0xmichalis commented Jan 29, 2017

resouer commented Jan 30, 2017

Choose a reason for hiding this comment

resouer Feb 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Feb 6, 2017

0xmichalis commented Feb 6, 2017 • edited

resouer commented Feb 14, 2017

timothysc commented Feb 14, 2017

0xmichalis commented Feb 14, 2017

smarterclayton commented Feb 14, 2017

k8s-github-robot commented Feb 15, 2017

k8s-github-robot commented Feb 15, 2017

k8s-github-robot commented Feb 15, 2017

k8s-ci-robot commented Feb 15, 2017

resouer commented Jan 23, 2017 •

edited by 0xmichalis

resouer commented Jan 28, 2017 •

edited

resouer Feb 6, 2017 •

edited

0xmichalis commented Feb 6, 2017 •

edited