Skip to content

don't do pod deletion management for pipeline builds #10370

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

Merged
merged 1 commit into from
Aug 13, 2016

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Aug 12, 2016

No description provided.

@bparees
Copy link
Contributor Author

bparees commented Aug 12, 2016

@csrwng ptal

@bparees
Copy link
Contributor Author

bparees commented Aug 12, 2016

@csrwng bump

@csrwng
Copy link
Contributor

csrwng commented Aug 12, 2016

@bparees sorry I missed this one earlier. What does it mean when your pipeline build is still running but a pod associated with the build is deleted?

@@ -373,9 +373,13 @@ func (bc *BuildPodDeleteController) HandleBuildPodDeletion(pod *kapi.Pod) error
}
build := obj.(*buildapi.Build)

if build.Spec.Strategy.JenkinsPipelineStrategy != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ut?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ut?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit test :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you and your high standards. yeah let me do that.

@bparees
Copy link
Contributor Author

bparees commented Aug 12, 2016

sorry I missed this one earlier. What does it mean when your pipeline build is still running but a pod associated with the build is deleted?

it means craziness happened because there should never be a pod associated w/ a pipeline build.

but because of some of the weird things we do to handle resync scenarios, it's possible a "pod deletion" event could get generated for a pod that never existed, which could cause us to mark the pipeline build as "ERROR/FAIL" (which is what we normally do when a pod disappears for a running build).

so i'm just trying to make very sure we don't do anything with pipeline builds as part of the pod deletion handling logic. (and conversely, if the pipeline build is deleted, that we don't accidentally terminate an innocent pod)

@csrwng
Copy link
Contributor

csrwng commented Aug 12, 2016

got it

@bparees bparees force-pushed the pipeline_build_deletion branch from 30fa932 to 1ad5d0f Compare August 12, 2016 21:14
@bparees bparees force-pushed the pipeline_build_deletion branch from 1ad5d0f to fb6397a Compare August 12, 2016 21:20
@bparees
Copy link
Contributor Author

bparees commented Aug 12, 2016

@csrwng UTed.

@csrwng
Copy link
Contributor

csrwng commented Aug 12, 2016

LGTM

@bparees
Copy link
Contributor Author

bparees commented Aug 13, 2016

[merge]

Ben Parees | OpenShift

On Aug 12, 2016 5:25 PM, "Cesar Wong" [email protected] wrote:

LGTM


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10370 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEvl3pyLzO5Xz3POq42SXFeLJSMrMC1Gks5qfOTQgaJpZM4Jiwxm
.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 13, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7897/) (Image: devenv-rhel7_4821)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to fb6397a

@openshift-bot openshift-bot merged commit 69c7837 into openshift:master Aug 13, 2016
@bparees bparees deleted the pipeline_build_deletion branch August 22, 2016 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants