-
Notifications
You must be signed in to change notification settings - Fork 159
Add possibility for a client to cancel a build… #510
Conversation
ac1b453
to
7ba6f0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks for adding this. Two questions:
1.) Is it normally common practice for a client to be able to update k8s resources' status this way? I'm thinking of something like Job
, which runs to completion, but which if you want to cancel it you basically just delete it. In this case, we want to stop the build's execution without also deleting any record of that resource existing.
2.) Once this is done we should also make the same change for TaskRun
, and then PipelineRun
, which should cancel (but not delete) its created TaskRun
s. We might even just want to go straight to TaskRun and drop this PR. I'll leave that decision to up to you.
I really am not sure… I'm trying to look around for that 😅
Yes, I did this here as it was simpler/quicker but I intend to follow-up on |
641b3c4
to
d8a3947
Compare
@vdemeester Great PR If build is completed then what is the point updating status to |
So, if you want to cancel the build (currently using the spec), then you want to mark it as
Indeed, there is no need. |
At Kubecon, @bobcatfish told me (in the context of tektoncd/pipeline#355) that I shouldn't be making changes to the status of one CRD while reconciling another one, so I jumped through some hoops to handle having the |
I agree with this completely. I do not think you understood my question so let me rephrase I am thinking of this like a cycle. If user marks build as "Cancelled" after completion, I do not see how that has any effect on underlying resource(pod). I would expect in this case for status to be not changed at all. If build has finished then status should not be changed(IMO).
Controller checks if build is still running and then cancels the build if timeout has passed. It doesn't cancel if build has already completed. |
Indeed there is no point, and if a user marks the build as
Yes, this is what happens. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vdemeester my apologies for not understanding the flow correctly :) Thanks for pinging and explaining it. I appreciate it
I have reviewed again. 👍 Please take a look at my comments
test/e2e/simple_test.go
Outdated
|
||
// Wait for a little while | ||
// FIXME(vdemeester) I would prefer something less flaky-prone | ||
time.Sleep(20 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably consider adding polling function to check build status is "Running" for 10sec(? or more). That is better than wait solution IMO. WDYT @vdemeester ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shashwathi yeah, that's why the FIXME 👼 not sure why I didn't use the polling functions here 😅 will update 😉
test/e2e/simple_test.go
Outdated
} | ||
|
||
if _, err := clients.buildClient.watchBuild(buildName); err == nil { | ||
t.Fatalf("watchBuild did not return expected `cancelled` error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
watchBuild
function returns build
object so you could consider verifying that build status is updated with expected reason("BuildCancelled") here.
pkg/reconciler/build/build_test.go
Outdated
t.Errorf("error syncing build: %v", err) | ||
} | ||
|
||
// Check that the build has the expected timeout status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: I think you meant cancelled status here
pkg/reconciler/build/build.go
Outdated
logger.Warnf("build %q has no pod running yet", build.Name) | ||
return nil | ||
} | ||
p, err := c.kubeclientset.CoreV1().Pods(build.Namespace).Get(build.Status.Cluster.PodName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I already mentioned this(?) in previous comment about reusing build.Status.Cluster.PodName
for Delete API directly. Sorry to repeat it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, didn't update that 😓
a2c94b6
to
5d8630b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thank you for addressing my comments.
/hold |
I noticed @imjasonh had some comment earlier in thread so I am adding |
/lgtm Oh my goodness I didn't realize this was blocking on me, so so sorry to keep this held up so long. Thanks for this change! ❤️ |
ah.. I may need to rebase and fix 😅 |
5d8630b
to
5627048
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ImJasonH, shashwathi, vdemeester 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 |
5627048
to
c9d4cc7
Compare
c9d4cc7
to
c4a8e85
Compare
… by updating the status of a build. It will mark the build as failed and clean any pods related to it. Signed-off-by: Vincent Demeester <[email protected]>
c4a8e85
to
0e37001
Compare
The following is the coverage report on pkg/.
|
Hum… the coverage build is a bit a pain 😅 as the threshold is higher than the current coverage on that file, any change trigger a failure… 😓 |
@vdemeester: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
/test pull-knative-build-integration-tests |
I agree @vdemeester - feel free to open an issue and we could work on makin this better - tbh I think we should drop the threshold for the reconcilers themselves - and/or add end to end test coverage measurement to the equation /lgtm |
In response to this:
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. |
… by updating the status of a build. It will mark the build as failed and clean any pods related to it. Signed-off-by: Vincent Demeester <[email protected]>
… by updating the status of a build. It will mark the build as failed and clean any pods related to it. Signed-off-by: Vincent Demeester <[email protected]>
… by updating the status of a build.
It will mark the build as failed and clean any pods related to it.
Related to tektoncd/pipeline#272
I tried that on
knative/build
first (as it's simpler 🙃)cc @abayer @imjasonh
Signed-off-by: Vincent Demeester [email protected]