[ZD-19670] CloudBees customer reported that the quite period checkout be... #32
Conversation
… behaviour has anomolous logic that can result in failed builds - This is likely where there is a delay between the build leaving the quiet period and the build starting - If there is a commit during that period of time then the build may checkout an inconsistent state - The solution is to use the time when the quiet period is completed as the timestamp for the checkout - Obviously people may prefer the original behaviour in some cases
|
plugins » cvs-plugin #228 SUCCESS |
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
[ZD-19670] CloudBees customer reported that the quite period checkout be...
| } | ||
| } | ||
| if (scmCause != null && wi.getAction(QuietPeriodCompleted.class) == null) { | ||
| wi.addAction(new QuietPeriodCompleted()); |
There was a problem hiding this comment.
@mc1arke what do you think of this? It feels wrong to me; the behavior of a build should not vary according to its Causes, which are normally considered to be informational only.
There was a problem hiding this comment.
Behaviour of the build should change according to causes (e.g. a build triggered by a git push may perform different initial operations from the same job triggered by a manual execution [not a CVS example, I know]). However this code here does feel wrong in how it's trying to achieve the end result, but I've not really looked at an alternative.
There was a problem hiding this comment.
@jglick well if User A triggers the build it might break because User A does not have permission to access the deployment credentials... or a build triggered by a GitHub Pull Request might run in a sandbox and skip deployment while a regular build from the direct GitHub trusted code repo might run outside the sandbox and follow through with deployment.
I think the principle of causes affecting build behaviour is well established... no matter how dirty it might feel ;-)
The real issue here is that Jenkins has never really had the actual implementation of quiet period that cruise control had... and the "quiet period" that Jenkins has does something different, and thus people end up trying to work-around and nobody actually asks what the real requirement is in the first place...
of course the real requirement is atomic commits, but I don't see CVS supporting those any time soon.
CruiseControl's quiet period would basically wait for a lull in commits of at least X seconds before starting a trigger... the perfect storm of commits would result in no builds being triggered until the commits stop.
Jenkins instead says, I am going to start the build at now+X.... defanging the perfect storm of commits problem... but if you checkout based on when the build starts there could be another set of commits from the next user that are partially included.
So the fix for SCM triggered actions is to watch for the quiet period being complete, so that we can catch the appropriate end of the commit storm window. Perhaps a better solution would be to capture the SCMTrigger timestamp itself and then just add the quiet period on to that and use that as the time-stamp for SCM triggered builds... same could be applied to other remote triggers if the time of triggering is recorded... but that would require changes in core
There was a problem hiding this comment.
a build triggered by a git push may perform different initial operations from the same job triggered by a manual execution
I am not aware of any such difference.
There was a problem hiding this comment.
Mind you, I agree that the behavior of the quiet period you describe seems poor. (And the CVS plugin should feel free to reinterpret the behavior of the quiet period, since this feature is pointless for any modern SCM—it was introduced for the benefit of CVS users.) I am just questioning what the purpose is of making this behavior conditional on SCMTriggerCause. IIUC, if you specify delay=0sec the quiet period is ignored anyway.
There was a problem hiding this comment.
Yes but the checkout time is the time of start building, not the time of exit quiet period.
The SCM trigger is special in this case because it was started by a commit and there could be another commit by the time the actual commit starts executing.
The issue here is when you have a system nearing resource limits. The build is triggered, the quiet period runs down, and then the build is waiting for an executor... while waiting there are some commits, then the build starts and sucks in those commits... it would be ok if the commits that rolled in caused the quiet period to be reset as then you would at least get commits + quiet period + execution delay + commits + quiet period + execution delay, but instead you get commits + quite period + execution delay + partial commits
...haviour has anomolous logic that can result in failed builds