-
-
Notifications
You must be signed in to change notification settings - Fork 511
WIP: Rework step status signaling #6011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
also when Wait() does not return err the step does not get cancled in the background either till it finished ?!? |
|
related work #3850 |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
That's nothing new… There's a summary issue for the whole pipeline canceling: #2875 |
|
@qwerty287 I'll work on that now ... will take some time i guess to get through all of it :) |
we have not documented pipeline even if it is our core engine that does the havey lifting ... also the agent-server0rpc stuff is in there but shound be a separate thing
…hed and refactored by us a lot and now is mosty ours
This comment was marked as resolved.
This comment was marked as resolved.
| return err | ||
| switch { | ||
| case strings.Contains(err.Error(), "desc = queue: task canceled"): | ||
| return pipeline.ErrCancel |
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.
we should make that signal explicit and not relay on an queue error msg
| int64 finished = 2; | ||
| string error = 3; | ||
| bool canceled = 4; | ||
| } |
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.
needs more thought: should we have enum so we can signal who has canceled ... if the server canceled it or the agent e.g. because of an agent shouldown
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.
similar to #6024 ... or handled by it ...
| ) | ||
|
|
||
| // Peer defines a peer-to-peer connection. | ||
| // TODO: suffix Next, Wait, Init, Done and Extend with "Workflow" for docu purpose |
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 needs more docu!
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.
// Peer defines the RPC interface between an agent and the Woodpecker server.
//
// It is responsible for coordinating workflow execution, including:
// - Fetching workflows from the queue
// - Reporting workflow and step state transitions
// - Handling workflow completion and cancellation
//
// Method naming intentionally omits explicit "Workflow" suffixes to keep the API concise.
// The workflow or step context is defined by the parameters of each method.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.
// - Handling workflow completion and cancellation
duplicate to aboth
we should instead mention how Wait blocks and how ... but tjat is subject to change
also we should prefux the interface
| var workflowsToCancel []string | ||
| for _, w := range workflows { | ||
| if w.State == model.StatusRunning || w.State == model.StatusPending { | ||
| workflowsToCancel = append(workflowsToCancel, fmt.Sprint(w.ID)) |
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.
in case the agent just crashed hard... and you cancle it the steps always stay at state "running" ... we should do a cleanup here too...
This reverts commit b1cbd96.
TODOs:services dont report back when they are finished: similar issue for detached steps: because both are just droped into the background: woodpecker/pipeline/pipeline.go Lines 276 to 279 in 8b6c814
|


currently if we cancel we get the grpc client on Wait() returns an err witch marks the pipeline failed:



with this patch we propper ignore that ... but now it shows success:
wat i expect is:
this state is initially as souch but after some time it gets updated to success ... i think it's related to the issue we sometimes see pipelines marked as success but they failed etc...
read more at #2875
close #833
close
wip: #3848, #2062, #6024