Skip to content
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

deployment controller: cancel deployers on new cancelled deployments #8418

Merged
merged 1 commit into from
Apr 11, 2016
Merged

deployment controller: cancel deployers on new cancelled deployments #8418

merged 1 commit into from
Apr 11, 2016

Conversation

0xmichalis
Copy link
Contributor

Deployer pods may stay around due to an unavailable node and get scheduled
after a cancelled deployment has been transitioned from New to Failed.

Found out in #8403

@pweil- @smarterclayton @ironcladlou

@0xmichalis
Copy link
Contributor Author

[test]

@0xmichalis 0xmichalis closed this Apr 8, 2016
@0xmichalis 0xmichalis reopened this Apr 8, 2016
Deployer pods may stay around due to an unavailable node and get scheduled
after a cancelled deployment has been transitioned from New to Failed.
@@ -62,7 +62,7 @@ func LabelForDeployment(deployment *api.ReplicationController) string {

// LabelForDeploymentConfig builds a string identifier for a DeploymentConfig.
func LabelForDeploymentConfig(config *deployapi.DeploymentConfig) string {
return fmt.Sprintf("%s/%s:%d", config.Namespace, config.Name, config.Status.LatestVersion)
return fmt.Sprintf("%s/%s", config.Namespace, config.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is confusing since we already name deployments after their version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this going to cause backwards compatibility issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? AFAIK we use it only for logging

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, my grep-fuu failed me.

@smarterclayton
Copy link
Contributor

[test]

On Fri, Apr 8, 2016 at 8:10 AM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2821/)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8418 (comment)

@smarterclayton
Copy link
Contributor

[test]

On Fri, Apr 8, 2016 at 10:55 AM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test NOTFOUND (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2829/)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8418 (comment)

@ironcladlou
Copy link
Contributor

LGTM once #8418 (comment) is answered

@smarterclayton
Copy link
Contributor

Can we spawn a follow up to track the bigger implications of this (in the
context of the even larger changes to deployments going on)?

On Fri, Apr 8, 2016 at 11:39 AM, Dan Mace [email protected] wrote:

LGTM once #8418 (comment)
#8418 (comment) is
answered


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8418 (comment)

@smarterclayton
Copy link
Contributor

[test]

On Fri, Apr 8, 2016 at 1:12 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test NOTFOUND (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2838/)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8418 (comment)

@smarterclayton
Copy link
Contributor

@soltysh has the failure for pullthrough

On Apr 8, 2016, at 3:50 PM, OpenShift Bot [email protected] wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2856/)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8418 (comment)

@0xmichalis
Copy link
Contributor Author

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f444ceb

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2870/)

@smarterclayton
Copy link
Contributor

Approved [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5555/) (Image: devenv-rhel7_3946)

@0xmichalis
Copy link
Contributor Author

#7429 flake

re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f444ceb

@openshift-bot openshift-bot merged commit 3860072 into openshift:master Apr 11, 2016
@0xmichalis 0xmichalis deleted the fail-deployer-on-failed-deployment branch April 11, 2016 10:24
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.

5 participants