Creating deep copy functions for k8s watcher #1986

aanm · 2017-11-07T14:15:13Z

This PR adds the proper deepcopy implementations in the structures that need to be deep copied before handling them on the kubernetes watcher callbacks.

Fixes #1885

Implemented deep copy functionality when receiving events from kubernetes watcher #1885

houndci-bot · 2017-11-07T14:15:25Z

pkg/labels/doc.go

+
+// +k8s:deepcopy-gen=package
+
+// Package api defines the API of the Cilium network policy interface


package comment should be of the form "Package labels ..."

houndci-bot · 2017-11-07T14:15:26Z

pkg/k8s/apis/cilium.io/v2/register.go

+}
+
+var (
+	SchemeBuilder runtime.SchemeBuilder


exported var SchemeBuilder should have comment or be unexported

houndci-bot · 2017-11-07T14:15:26Z

pkg/labels/doc.go

+
+// +k8s:deepcopy-gen=package
+
+// Package api defines the API of the Cilium network policy interface


package comment should be of the form "Package labels ..."

houndci-bot · 2017-11-07T14:15:27Z

pkg/k8s/apis/cilium.io/v2/register.go

+}
+
+var (
+	SchemeBuilder runtime.SchemeBuilder


exported var SchemeBuilder should have comment or be unexported

rlenglet · 2017-11-07T21:19:56Z

daemon/k8s_watcher.go

+		logfields.K8sAPIVersion:                 oldk8sNP.TypeMeta.APIVersion,
+		logfields.K8sNetworkPolicyName + ".old": oldk8sNP.ObjectMeta.Name,
+		logfields.K8sNamespace + ".old":         oldk8sNP.ObjectMeta.Namespace,
+		logfields.K8sNetworkPolicyName + ".new": newk8sNP.ObjectMeta.Name,


IMHO you should avoid creating new log field keys on the fly. When those logs are fed into an ELK stack, this will prevent correlating all the messages for this namespace. You should consistently use the same keys.
I'd suggest only renaming one pair of keys, e.g. keeping only the + ".old", or better log 2 messages with the different values.

This comment applies to the other places below where you create new keys like here.

I'll do that later #2006

ianvernon

LGTM! Thanks for taking care of this.

ianvernon · 2017-11-09T23:58:07Z

Just remembered this - can you please document how to generate DeepCopy functions?

tgraf · 2017-11-10T02:14:51Z

vendor.conf

 k8s.io/kubernetes v1.5.1
 github.com/sasha-s/go-deadlock master
 github.com/petermattis/goid 0ded85884ba5c4c9267fcd5a149061f7c3455eee
 github.com/google/gops 25312cafb9a191a6d377c480a4666beec3105e4a
 github.com/kardianos/osext ae77be60afb1dcacde03767a8c37337fad28ac14
 github.com/kr/text 7cafcd837844e784b526369c9bce262804aebc60
 github.com/kr/pretty cfb55aafdaf3ec08f0db22699ab822c50091b1c4
-github.com/optiopay/kafka 02fdf3fd241af9fe0b226577627e24b7f9a4c8e5 https://github.com/cilium/kafka
+github.com/optiopay/kafka 41e2198304524d5f0e0f47724832bcd385229902 https://github.com/cilium/kafka


Why are you updating kafka in this commit?

02fd no longer exists, I had to update it to 41e21 which is the last one in https://github.com/cilium/kafka

tgraf · 2017-11-10T02:15:17Z

vendor.conf

 github.com/golang/snappy 553a641470496b2327abcac10b36396bd98e45c9
 github.com/kevinburke/ssh_config db49ba357de1f26c56dac48a5de39c65785bf24a
 github.com/onsi/ginkgo 11459a886d9cd66b319dac7ef1e917ee221372c9
 github.com/onsi/gomega 283ed655c6b8238afecd5bea6434bc9b03129f2f
+github.com/googleapis/gnostic v0.1.0


Are these all new dependencies for k8s?

houndci-bot · 2017-11-10T08:58:44Z

pkg/k8s/apis/cilium.io/v2/zz_generated.deepcopy.go

+func (in *CiliumNetworkPolicyList) DeepCopyObject() runtime.Object {
+	if c := in.DeepCopy(); c != nil {
+		return c
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2017-11-10T08:58:44Z

pkg/k8s/apis/cilium.io/v2/zz_generated.deepcopy.go

+func (in *CiliumNetworkPolicy) DeepCopyObject() runtime.Object {
+	if c := in.DeepCopy(); c != nil {
+		return c
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

- Add DeepCopy methods needed for kubernetes CRDs. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

aanm added the wip label Nov 7, 2017

aanm requested a review from a team November 7, 2017 14:15

aanm requested review from a team as code owners November 7, 2017 14:15

aanm requested review from a team November 7, 2017 14:15

aanm requested a review from a team as a code owner November 7, 2017 14:15

houndci-bot reviewed Nov 7, 2017

View reviewed changes

aanm force-pushed the creating-deep-copy-in-k8s-watcher-2 branch from 17ccda8 to 9600030 Compare November 7, 2017 14:53

rlenglet approved these changes Nov 7, 2017

View reviewed changes

aanm force-pushed the creating-deep-copy-in-k8s-watcher-2 branch 2 times, most recently from f4c8423 to c31c22f Compare November 9, 2017 22:53

aanm mentioned this pull request Nov 9, 2017

Rename key pairs .old vs .new in k8s_watcher.go #2006

Closed

2 tasks

cilium deleted a comment from houndci-bot Nov 9, 2017

ianvernon approved these changes Nov 9, 2017

View reviewed changes

tgraf requested changes Nov 10, 2017

View reviewed changes

aanm force-pushed the creating-deep-copy-in-k8s-watcher-2 branch from c31c22f to 096cb81 Compare November 10, 2017 08:58

houndci-bot reviewed Nov 10, 2017

View reviewed changes

aanm added 4 commits November 10, 2017 10:04

adding generated DeepCopy

868e510

- Add DeepCopy methods needed for kubernetes CRDs. Signed-off-by: André Martins <andre@cilium.io>

vendor: upgrade k8s to 1.8.2

e3be028

Signed-off-by: André Martins <andre@cilium.io>

refatoring after upgrading kubernetes to 1.8.2

27170ed

Signed-off-by: André Martins <andre@cilium.io>

k8s: add missing deep copying cilium network policies before handling

af10b7f

Signed-off-by: André Martins <andre@cilium.io>

aanm force-pushed the creating-deep-copy-in-k8s-watcher-2 branch from 096cb81 to af10b7f Compare November 10, 2017 09:20

aanm requested a review from tgraf November 10, 2017 12:46

aanm added the stable/needs-backport label Nov 10, 2017

nebril approved these changes Nov 10, 2017

View reviewed changes

tgraf approved these changes Nov 10, 2017

View reviewed changes

tgraf merged commit 28fc671 into master Nov 10, 2017

tgraf deleted the creating-deep-copy-in-k8s-watcher-2 branch November 10, 2017 15:05

tgraf removed the stable/needs-backport label Dec 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating deep copy functions for k8s watcher #1986

Creating deep copy functions for k8s watcher #1986

aanm commented Nov 7, 2017 •

edited

houndci-bot Nov 7, 2017

houndci-bot Nov 7, 2017

houndci-bot Nov 7, 2017

houndci-bot Nov 7, 2017

rlenglet Nov 7, 2017

aanm Nov 9, 2017

ianvernon left a comment

ianvernon commented Nov 9, 2017

tgraf Nov 10, 2017

aanm Nov 10, 2017

tgraf Nov 10, 2017

aanm Nov 10, 2017

houndci-bot Nov 10, 2017

houndci-bot Nov 10, 2017


		// +k8s:deepcopy-gen=package

		// Package api defines the API of the Cilium network policy interface

Creating deep copy functions for k8s watcher #1986

Creating deep copy functions for k8s watcher #1986

Conversation

aanm commented Nov 7, 2017 • edited

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

ianvernon left a comment

Choose a reason for hiding this comment

ianvernon commented Nov 9, 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

aanm commented Nov 7, 2017 •

edited