Using PATCH instead of PUT to update PipelineActivity #2494
Conversation
This is to avoid http status 409 when other process has updated activity
This is to avoid http status 409 when other process has updated activity
This is to avoid http status 409 when other process has updated activity Fixes #1890
…ivities Conflicts: pkg/kube/activity.go
This is to avoid http status 409 when other process has updated activity
This is to avoid http status 409 when other process has updated activity Fixes #1890
…ivities Conflicts: pkg/kube/activity.go
|
Hi @msvticket. Thanks for your PR. I'm waiting for a jenkins-x member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ccojocar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
|
/retest |
PatchRow now declared in pkg/kube/patch.go Removing excessive imports Wrapping marshall error
|
New changes are detected. LGTM label has been removed. |
|
/test bdd |
1 similar comment
|
/test bdd |
|
/ok-to-test |
|
/test bdd |
|
/test bdd |
|
/test this |
1 similar comment
|
/test this |
|
@msvticket: The following test failed, say
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. |
There was a problem hiding this comment.
I think I had discussed this issue with @davidcurrie at some point.
| @@ -407,6 +407,22 @@ func (k *PromoteStepActivityKey) GetOrCreatePromoteUpdate(activities typev1.Pipe | |||
| return a, s, p, p.Update, created, err | |||
| } | |||
|
|
|||
| // Almost making a full Update, but using Patch to work around problem with sometimes getting http status 409 back | |||
There was a problem hiding this comment.
So this is not really fixing the race condition which the K8s API server helpfully alerted you to with the 409 error. It is just masking it, by clobbering any other changes to the spec that happened to come in from another client. To my mind that is worse. At least without this patch, you can sometimes recover and behave correctly simply by retrying the entire operation.
The real fix would be to review every use of a PUT call, which seems to be wrapped in a generated Update method, and try to replace it with a PATCH. Here is a simple example.
The trouble is that the generated Patch methods, as opposed to Update methods, lack type safety: you need to manually construct a JSON patch body before applying it, which is a lot of work and looks error-prone. In fact the example I linked to seems to be the only such case in this whole repo so far! Compare the manually defined JSON structure
type JsonPatch struct {
ImagePullSecret *[]ImagePullSecret `json:"imagePullSecrets"`
}
type ImagePullSecret struct {
Name string `json:"name"`
}to the definition of the struct field in the official API class:
ImagePullSecrets []LocalObjectReference `json:"imagePullSecrets,omitempty" protobuf:"bytes,3,rep,name=imagePullSecrets"`
// …
type LocalObjectReference struct {
Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
}There was a problem hiding this comment.
I suppose a less cumbersome approach would be to make Update methods take an old and new object, to be called like this:
diff --git a/pkg/jx/cmd/step_collect.go b/pkg/jx/cmd/step_collect.go
index 9d7a90f5..96d020de 100644
--- a/pkg/jx/cmd/step_collect.go
+++ b/pkg/jx/cmd/step_collect.go
@@ -249,11 +249,12 @@ func (o *StepCollectOptions) collectGitURL(sourceURL string) (err error) {
if err != nil {
return err
}
+ oldA := *a
a.Spec.Attachments = append(a.Spec.Attachments, jenkinsv1.Attachment{
Name: classifier,
URLs: urls,
})
- _, err = client.JenkinsV1().PipelineActivities(ns).Update(a)
+ _, err = client.JenkinsV1().PipelineActivities(ns).Update(&oldA, a)
if err != nil {
return err
}The implementation could optimistically send a PUT request, assuming the API server can be relied on to check resourceVersions, and if it gets a 409 then use reflection to compute the minimal diff between the two trees and generate a PATCH request.
Alternately, we could just avoid PATCH alttogether and retry the whole block that failed in a PUT:
diff --git a/pkg/jx/cmd/step_collect.go b/pkg/jx/cmd/step_collect.go
index 9d7a90f5..48b11c69 100644
--- a/pkg/jx/cmd/step_collect.go
+++ b/pkg/jx/cmd/step_collect.go
@@ -245,6 +245,7 @@ func (o *StepCollectOptions) collectGitURL(sourceURL string) (err error) {
Build: build,
},
}
+ for {
a, _, err := key.GetOrCreate(activities)
if err != nil {
return err
@@ -254,10 +255,15 @@ func (o *StepCollectOptions) collectGitURL(sourceURL string) (err error) {
URLs: urls,
})
_, err = client.JenkinsV1().PipelineActivities(ns).Update(a)
- if err != nil {
+ if err == nil {
+ break
+ } else if strings.Contains(err.Error(), "409") {
+ continue
+ } else {
return err
}
}
+ }
return nil
}though I assume there is some more type-safe way of accessing the error code. (Seems there is a StatusError and you can check for StatusReasonConflict or something.)
There was a problem hiding this comment.
I think it's much better to, as you say, clobber the CRD than fail. Especially since at times promotions has failed repeatedly on 409.
I did start a fix that did minimal patches, but I realised that the fix would become unreasonably big and complex.
|
@msvticket Are you planning to still work on this PR? Should we close it for now and try to address the issue in another PR? |
|
I was planning to work on it, but nobody has addressed the issues I raised in "Special notes for the reviewer(s)" |
|
@msvticket I am closing this PR. Please could you send the new solution in a different PR? Thank you |
Submitter checklist
Description
This is to avoid http status 409 when other process has updated activity.
Also simplifying a bit by updating once in any case and not zero, one or two times depending on. The zero case didn't happen anyway as far as I could see.
Special notes for the reviewer(s)
This pull request was first made as #2110. Then James Nord created a new (#2309) one subsuming the first. But that pull request was basically abandoned. So now I try again...
A problem is that FakePipelineActivities.Patch doesn't support other than strategic merge patch which at least is the main reason that some tests fail. See k8s.io/client-go@v8.0.0+incompatible/testing/fixture.go:140
Later versions of k8s.io/client-go supports JSON Patch as well. I tried upgrading but had to give up when I found out that heptio/sonobuoy doesn't support later versions of k8s.io/client-go. It isn't obvious to me how to resolve this.
Which issue this PR fixes
fixes #1890