From e9c1d15e47ff9a4b7cd444650f6064ea9f892e0f Mon Sep 17 00:00:00 2001 From: msvticket Date: Thu, 25 Oct 2018 10:36:27 +0200 Subject: [PATCH 1/4] Using PATCH instead of PUT to update PipelineActivity This is to avoid http status 409 when other process has updated activity --- pkg/kube/activity.go | 10 ++++++++-- pkg/kube/activity_functions.go | 24 ++++++------------------ 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/pkg/kube/activity.go b/pkg/kube/activity.go index cb13fba4c8..cc35d95c9a 100644 --- a/pkg/kube/activity.go +++ b/pkg/kube/activity.go @@ -1,6 +1,7 @@ package kube import ( + "encoding/json" "fmt" "reflect" "strconv" @@ -15,6 +16,7 @@ import ( "github.com/jenkins-x/jx/pkg/gits" "github.com/jenkins-x/jx/pkg/log" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" ) type PipelineActivityKey struct { @@ -184,14 +186,18 @@ func (k *PipelineActivityKey) GetOrCreate(activities typev1.PipelineActivityInte a = defaultActivity } oldSpec := a.Spec - spec := &a.Spec + spec := &defaultActivity.Spec updateActivitySpec(k, spec) if create { answer, err := activities.Create(a) return answer, true, err } else { if !reflect.DeepEqual(&a.Spec, &oldSpec) { - answer, err := activities.Update(a) + patch, err := json.Marshal(defaultActivity) + if err != nil { + return defaultActivity, create, nil + } + answer, err := activities.Patch(a.Name, types.MergePatchType, patch) return answer, false, err } return a, false, nil diff --git a/pkg/kube/activity_functions.go b/pkg/kube/activity_functions.go index 10ef3b67fc..8d475fd528 100644 --- a/pkg/kube/activity_functions.go +++ b/pkg/kube/activity_functions.go @@ -48,15 +48,9 @@ func StartPromotionPullRequest(a *v1.PipelineActivity, s *v1.PipelineActivitySte Time: time.Now(), } } - if a.Spec.WorkflowStatus != v1.ActivityStatusTypeRunning { - a.Spec.WorkflowStatus = v1.ActivityStatusTypeRunning - } - if a.Spec.Status != v1.ActivityStatusTypeRunning { - a.Spec.Status = v1.ActivityStatusTypeRunning - } - if p.Status != v1.ActivityStatusTypeRunning { - p.Status = v1.ActivityStatusTypeRunning - } + a.Spec.WorkflowStatus = v1.ActivityStatusTypeRunning + a.Spec.Status = v1.ActivityStatusTypeRunning + p.Status = v1.ActivityStatusTypeRunning return nil } @@ -71,15 +65,9 @@ func StartPromotionUpdate(a *v1.PipelineActivity, s *v1.PipelineActivityStep, ps Time: time.Now(), } } - if a.Spec.WorkflowStatus != v1.ActivityStatusTypeRunning { - a.Spec.WorkflowStatus = v1.ActivityStatusTypeRunning - } - if a.Spec.Status != v1.ActivityStatusTypeRunning { - a.Spec.Status = v1.ActivityStatusTypeRunning - } - if p.Status != v1.ActivityStatusTypeRunning { - p.Status = v1.ActivityStatusTypeRunning - } + a.Spec.WorkflowStatus = v1.ActivityStatusTypeRunning + a.Spec.Status = v1.ActivityStatusTypeRunning + p.Status = v1.ActivityStatusTypeRunning return nil } From a98fcb1e532c0ef6a819377c01f7711c7abaa8c2 Mon Sep 17 00:00:00 2001 From: msvticket Date: Mon, 29 Oct 2018 17:47:19 +0100 Subject: [PATCH 2/4] Using PATCH instead of PUT to update PipelineActivity This is to avoid http status 409 when other process has updated activity Fixes #1890 --- pkg/kube/activity.go | 74 ++++++++++++++++++--------------------- pkg/kube/activity_test.go | 23 +++++++++++- 2 files changed, 57 insertions(+), 40 deletions(-) diff --git a/pkg/kube/activity.go b/pkg/kube/activity.go index cc35d95c9a..a9373eb025 100644 --- a/pkg/kube/activity.go +++ b/pkg/kube/activity.go @@ -3,18 +3,17 @@ package kube import ( "encoding/json" "fmt" - "reflect" "strconv" "strings" "time" - "github.com/jenkins-x/jx/pkg/util" + "k8s.io/apimachinery/pkg/api/errors" - "github.com/ghodss/yaml" "github.com/jenkins-x/jx/pkg/apis/jenkins.io/v1" typev1 "github.com/jenkins-x/jx/pkg/client/clientset/versioned/typed/jenkins.io/v1" "github.com/jenkins-x/jx/pkg/gits" "github.com/jenkins-x/jx/pkg/log" + "github.com/jenkins-x/jx/pkg/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -182,24 +181,19 @@ func (k *PipelineActivityKey) GetOrCreate(activities typev1.PipelineActivityInte } a, err := activities.Get(name, metav1.GetOptions{}) if err != nil { - create = true - a = defaultActivity + if errors.IsNotFound(err) { + create = true + a = defaultActivity + } else { + return defaultActivity, create, err + } } - oldSpec := a.Spec - spec := &defaultActivity.Spec + spec := &a.Spec updateActivitySpec(k, spec) if create { answer, err := activities.Create(a) return answer, true, err } else { - if !reflect.DeepEqual(&a.Spec, &oldSpec) { - patch, err := json.Marshal(defaultActivity) - if err != nil { - return defaultActivity, create, nil - } - answer, err := activities.Patch(a.Name, types.MergePatchType, patch) - return answer, false, err - } return a, false, nil } } @@ -413,6 +407,28 @@ func (k *PromoteStepActivityKey) GetOrCreatePromoteUpdate(activities typev1.Pipe return a, s, p, p.Update, created, err } +// PatchRow is a json patch structure +type PatchRow struct { + Op string `json:"op"` + Path string `json:"path"` + Value interface{} `json:"value"` +} + +// Almost making a full Update, but using Patch to work around problem with sometimes getting http status 409 back +func patchSpec(activities typev1.PipelineActivityInterface, activity *v1.PipelineActivity) (err error) { + things := make([]PatchRow, 1) + things[0].Value = activity.Spec + things[0].Op = "add" + things[0].Path = "/spec" + + patchBytes, err := json.Marshal(things) + if err != nil { + return + } + _, err = activities.Patch(activity.Name, types.JSONPatchType, patchBytes) + return +} + func (k *PromoteStepActivityKey) OnPromotePullRequest(activities typev1.PipelineActivityInterface, fn PromotePullRequestFn) error { if !k.IsValid() { return nil @@ -421,21 +437,16 @@ func (k *PromoteStepActivityKey) OnPromotePullRequest(activities typev1.Pipeline log.Warn("Warning: no PipelineActivities client available!") return nil } - a, s, ps, p, added, err := k.GetOrCreatePromotePullRequest(activities) + a, s, ps, p, _, err := k.GetOrCreatePromotePullRequest(activities) if err != nil { return err } - p1 := *p err = fn(a, s, ps, p) if err != nil { return err } - p2 := *p - if added || !reflect.DeepEqual(p1, p2) { - _, err = activities.Update(a) - } - return err + return patchSpec(activities, a) } func (k *PromoteStepActivityKey) OnPromoteUpdate(activities typev1.PipelineActivityInterface, fn PromoteUpdateFn) error { @@ -446,11 +457,10 @@ func (k *PromoteStepActivityKey) OnPromoteUpdate(activities typev1.PipelineActiv log.Warn("Warning: no PipelineActivities client available!") return nil } - a, s, ps, p, added, err := k.GetOrCreatePromoteUpdate(activities) + a, s, ps, p, _, err := k.GetOrCreatePromoteUpdate(activities) if err != nil { return err } - p1 := asYaml(a) if k.ApplicationURL != "" { ps.ApplicationURL = k.ApplicationURL } @@ -461,21 +471,7 @@ func (k *PromoteStepActivityKey) OnPromoteUpdate(activities typev1.PipelineActiv if k.ApplicationURL != "" { ps.ApplicationURL = k.ApplicationURL } - p2 := asYaml(a) - - if added || p1 == "" || p1 != p2 { - _, err = activities.Update(a) - } - return err -} - -func asYaml(activity *v1.PipelineActivity) string { - data, err := yaml.Marshal(activity) - if err == nil { - return string(data) - } - log.Warnf("Failed to marshal PipelineActivity to YAML %s: %s", activity.Name, err) - return "" + return patchSpec(activities, a) } func (k *PromoteStepActivityKey) matchesPreview(step *v1.PipelineActivityStep) bool { diff --git a/pkg/kube/activity_test.go b/pkg/kube/activity_test.go index 2f7b9b427b..5184685107 100644 --- a/pkg/kube/activity_test.go +++ b/pkg/kube/activity_test.go @@ -1,7 +1,10 @@ package kube_test import ( + "encoding/json" "fmt" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" "strconv" "testing" "time" @@ -44,7 +47,7 @@ func (m *MockPipelineActivityInterface) Get(name string, options metav1.GetOptio if ok { return a, nil } - return nil, fmt.Errorf("No such PipelineActivity %s", name) + return nil, errors.NewNotFound(schema.GroupResource{}, name) } func (m *MockPipelineActivityInterface) List(opts metav1.ListOptions) (*v1.PipelineActivityList, error) { @@ -61,7 +64,25 @@ func (m *MockPipelineActivityInterface) Watch(opts metav1.ListOptions) (watch.In return nil, fmt.Errorf("TODO") } +// SpecPatchRow is a json patch structure +type SpecPatchRow struct { + Op string `json:"op"` + Path string `json:"path"` + Value v1.PipelineActivitySpec `json:"value"` +} + func (m *MockPipelineActivityInterface) Patch(name string, pt types.PatchType, data []byte, subresources ...string) (result *v1.PipelineActivity, err error) { + if pt == types.JSONPatchType { + patch := make([]SpecPatchRow, 1) + err := json.Unmarshal(data, &patch) + if err != nil { + return nil, err + } + if len(patch) == 1 && patch[0].Op == "add" && patch[0].Path == "/spec" { + m.Activities[name].Spec = patch[0].Value + return m.Activities[name], nil + } + } return nil, fmt.Errorf("TODO") } From 89a4e2d3c2663d8559dd7b1ef27493e5d817d5c2 Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 22 Nov 2018 16:41:52 +0000 Subject: [PATCH 3/4] fix bad merge on my part --- pkg/kube/activity.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/kube/activity.go b/pkg/kube/activity.go index a9373eb025..092f822b17 100644 --- a/pkg/kube/activity.go +++ b/pkg/kube/activity.go @@ -9,6 +9,8 @@ import ( "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/errors" + "github.com/jenkins-x/jx/pkg/apis/jenkins.io/v1" typev1 "github.com/jenkins-x/jx/pkg/client/clientset/versioned/typed/jenkins.io/v1" "github.com/jenkins-x/jx/pkg/gits" From caa3a8f84344b54a9d5bf974b505f0f43432fec8 Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Wed, 28 Nov 2018 12:07:15 +0100 Subject: [PATCH 4/4] fix: remove unused import --- pkg/kube/activity.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/kube/activity.go b/pkg/kube/activity.go index 092f822b17..e398be77cb 100644 --- a/pkg/kube/activity.go +++ b/pkg/kube/activity.go @@ -7,15 +7,12 @@ import ( "strings" "time" - "k8s.io/apimachinery/pkg/api/errors" - - "k8s.io/apimachinery/pkg/api/errors" - "github.com/jenkins-x/jx/pkg/apis/jenkins.io/v1" typev1 "github.com/jenkins-x/jx/pkg/client/clientset/versioned/typed/jenkins.io/v1" "github.com/jenkins-x/jx/pkg/gits" "github.com/jenkins-x/jx/pkg/log" "github.com/jenkins-x/jx/pkg/util" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" )