From a0220aa5f0c89be8f90d6a11d780595801b2d77b Mon Sep 17 00:00:00 2001 From: Jelmer Snoeck Date: Tue, 5 Jun 2018 08:58:41 +0200 Subject: [PATCH 1/2] Correct Syncing behaviour. Kubekit has a patch which fixes an issue where it would try and update an object with it's own status, causing an infinite update loop. By updating kubekit, we can now drop the ShouldSync check. This will give us a better experience by: 1) no need for the ShouldSync checks, which are fragile, 2) the underlying parts of a sync action are now triggered correctly as well, this means that the cluster will always be in a state which is correct. Before, this could have been different --- Gopkg.lock | 8 ++-- pkg/githubrepository/controller.go | 4 +- pkg/imagepolicy/controller.go | 5 +-- pkg/k8sutils/status.go | 66 ------------------------------ pkg/networkpolicy/controller.go | 11 +---- pkg/svc/controller.go | 5 +-- 6 files changed, 9 insertions(+), 90 deletions(-) delete mode 100644 pkg/k8sutils/status.go diff --git a/Gopkg.lock b/Gopkg.lock index 86cb977..a8f5993 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -406,7 +406,7 @@ "kubetest", "patcher" ] - revision = "edf4be11e8b41542ab632040ed5bd141b873ddbe" + revision = "c5b8ec922b301a267942b96a30d279926136fb75" [[projects]] name = "github.com/jessevdk/go-flags" @@ -546,10 +546,10 @@ revision = "86672fcb3f950f35f2e675df2240550f2a50762f" [[projects]] + branch = "master" name = "github.com/spf13/cobra" packages = ["."] - revision = "ef82de70bb3f60c65fb8eebacbb2d122ef517385" - version = "v0.0.3" + revision = "1e58aa3361fd650121dceeedc399e7189c05674a" [[projects]] name = "github.com/spf13/pflag" @@ -1174,6 +1174,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "f46fb83b2d34da66c1e1d9da2bb6a255ff8a58dc65b67c6d8644070d0b312b84" + inputs-digest = "6858dc4035bf27abc2cba9dbe4517a3593b169681638d1d4255977e058b06e0f" solver-name = "gps-cdcl" solver-version = 1 diff --git a/pkg/githubrepository/controller.go b/pkg/githubrepository/controller.go index 18e2d0d..8c25d57 100644 --- a/pkg/githubrepository/controller.go +++ b/pkg/githubrepository/controller.go @@ -117,9 +117,7 @@ func (c *Controller) run(ctx context.Context) { c.syncPolicy(obj) }, UpdateFunc: func(old, new interface{}) { - if ok, err := k8sutils.ShouldSync(old, new); ok && err == nil { - c.syncPolicy(new) - } + c.syncPolicy(new) }, DeleteFunc: func(obj interface{}) { cp := obj.(*v1alpha1.GitHubRepository).DeepCopy() diff --git a/pkg/imagepolicy/controller.go b/pkg/imagepolicy/controller.go index ed31750..b1bb946 100644 --- a/pkg/imagepolicy/controller.go +++ b/pkg/imagepolicy/controller.go @@ -9,7 +9,6 @@ import ( "syscall" "github.com/manifoldco/heighliner/pkg/api/v1alpha1" - "github.com/manifoldco/heighliner/pkg/k8sutils" "github.com/manifoldco/heighliner/pkg/registry" "github.com/manifoldco/heighliner/pkg/registry/hub" @@ -95,9 +94,7 @@ func (c *Controller) run(ctx context.Context) { c.syncPolicy(obj) }, UpdateFunc: func(old, new interface{}) { - if ok, err := k8sutils.ShouldSync(old, new); ok && err == nil { - c.syncPolicy(new) - } + c.syncPolicy(new) }, DeleteFunc: func(obj interface{}) { cp := obj.(*v1alpha1.ImagePolicy).DeepCopy() diff --git a/pkg/k8sutils/status.go b/pkg/k8sutils/status.go deleted file mode 100644 index 44c796e..0000000 --- a/pkg/k8sutils/status.go +++ /dev/null @@ -1,66 +0,0 @@ -package k8sutils - -import ( - "encoding/json" -) - -func getStatuslessData(obj interface{}) ([]byte, error) { - byteData, err := json.Marshal(obj) - if err != nil { - return nil, err - } - - var mapData map[string]interface{} - if err := json.Unmarshal(byteData, &mapData); err != nil { - return nil, err - } - - delete(mapData, "status") - return json.Marshal(mapData) -} - -func getSpec(obj interface{}) ([]byte, error) { - byteData, err := json.Marshal(obj) - if err != nil { - return nil, err - } - - var mapData map[string]interface{} - if err := json.Unmarshal(byteData, &mapData); err != nil { - return nil, err - } - - return json.Marshal(mapData["spec"]) -} - -// ShouldSync will validate two objects and see if there are any updates worth -// syncing. -func ShouldSync(old, new interface{}) (bool, error) { - oldData, err := getStatuslessData(old) - if err != nil { - return false, err - } - - newData, err := getStatuslessData(new) - if err != nil { - return false, err - } - - return string(oldData) != string(newData), nil -} - -// SpecChanges will look at the `spec` in runtime objects and see if they -// differ. -func SpecChanges(old, new interface{}) (bool, error) { - oldData, err := getSpec(old) - if err != nil { - return false, err - } - - newData, err := getSpec(new) - if err != nil { - return false, err - } - - return string(oldData) != string(newData), nil -} diff --git a/pkg/networkpolicy/controller.go b/pkg/networkpolicy/controller.go index 21797f9..d505cef 100644 --- a/pkg/networkpolicy/controller.go +++ b/pkg/networkpolicy/controller.go @@ -8,7 +8,6 @@ import ( "syscall" "github.com/manifoldco/heighliner/pkg/api/v1alpha1" - "github.com/manifoldco/heighliner/pkg/k8sutils" "github.com/jelmersnoeck/kubekit" "github.com/jelmersnoeck/kubekit/patcher" @@ -76,15 +75,7 @@ func (c *Controller) run(ctx context.Context) { c.syncNetworking(obj) }, UpdateFunc: func(old, new interface{}) { - ok, err := k8sutils.ShouldSync(old, new) - if err != nil { - cp := old.(*v1alpha1.NetworkPolicy).DeepCopy() - log.Printf("Error syncing networkpolicy %s: %s:", cp.Name, err) - } - - if ok { - c.syncNetworking(new) - } + c.syncNetworking(new) }, DeleteFunc: func(obj interface{}) { cp := obj.(*v1alpha1.NetworkPolicy).DeepCopy() diff --git a/pkg/svc/controller.go b/pkg/svc/controller.go index 1a10dc7..9296753 100644 --- a/pkg/svc/controller.go +++ b/pkg/svc/controller.go @@ -78,9 +78,7 @@ func (c *Controller) run(ctx context.Context) { c.patchMicroservice(obj) }, UpdateFunc: func(old, new interface{}) { - if ok, err := k8sutils.ShouldSync(old, new); ok && err == nil { - c.patchMicroservice(new) - } + c.patchMicroservice(new) }, DeleteFunc: func(obj interface{}) { svc := obj.(*v1alpha1.Microservice).DeepCopy() @@ -153,6 +151,7 @@ func (c *Controller) patchMicroservice(obj interface{}) error { // new release objects, store them svc.Status.Releases = deployedReleases + // need to specify types again until we resolve the mapping issue svc.TypeMeta = metav1.TypeMeta{ Kind: "Microservice", From 960a4420b4354d959248519b6be868cdc551980e Mon Sep 17 00:00:00 2001 From: Jelmer Snoeck Date: Tue, 5 Jun 2018 10:40:19 +0200 Subject: [PATCH 2/2] Don't set global resync period. By using the init() function, we were overriding the resync period for all controllers whilst we only wanted this behaviour for the GitHub Connector. Moving it into the controller construct ensures that it's only called when we call that constructor. --- pkg/githubrepository/controller.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/githubrepository/controller.go b/pkg/githubrepository/controller.go index 8c25d57..bd562ef 100644 --- a/pkg/githubrepository/controller.go +++ b/pkg/githubrepository/controller.go @@ -26,11 +26,6 @@ import ( cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util" ) -func init() { - // let's not hit rate limits - kubekit.ResyncPeriod = 30 * time.Second -} - // Controller will take care of syncing the internal status of the GitHub Policy // object with the available releases on GitHub. type Controller struct { @@ -58,6 +53,11 @@ const authTokenKey = "GITHUB_AUTH_TOKEN" // NewController returns a new GitHubRepository Controller. func NewController(rcfg *rest.Config, cs kubernetes.Interface, namespace string, cfg Config) (*Controller, error) { + // Let's not hit rate limits. + // This is done here instead of an init function so we don't override the + // global settings for other controllers. + kubekit.ResyncPeriod = 30 * time.Second + rc, err := kubekit.RESTClient(rcfg, &v1alpha1.SchemeGroupVersion, v1alpha1.AddToScheme) if err != nil { return nil, err