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

Update deployment logs and surge on recreate #8787

Merged
merged 9 commits into from
May 16, 2016

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented May 7, 2016

Don't use glog in deployment output, also surge on recreate. Make deployment truly reentrant, with the --until=[start|pre|mid|0%|50%|100%] condition to gate how far to go:

$ openshift-deploy --until=pre
--> pre: Running hook pod ...
my hook pod ran
--> pre: Success
--> pre hook completed
$ openshift-deploy --until=50%
--> pre: Hook pod already succeeded
--> Scaling deployment-2 from 0 to 5
    Scaling deployment-2 up to 3
    Scaling deployment-1 down to 2
--> Reached 50% (currently 60%)
$ sleep 5
$ curl http://myservice:8080/metrics
user metrics ok
$ openshift-deploy
--> pre: Hook pod already succeeded
--> Scaling deployment-2 from 3 to 5
    Scaling deployment-2 up to 5
    Scaling deployment-1 down to 0
--> Success

Will allow custom deployments to be scriptable.

@smarterclayton smarterclayton force-pushed the manage_deploy_logs branch 4 times, most recently from c713dfe to f452554 Compare May 8, 2016 01:29
@smarterclayton
Copy link
Contributor Author

@ironcladlou @Kargakis this will allow someone to create a custom deployment that is just the openshift-deploy image and script a deployment with bash.

@smarterclayton smarterclayton force-pushed the manage_deploy_logs branch 2 times, most recently from 08aeda9 to 0c079db Compare May 8, 2016 16:19
@smarterclayton
Copy link
Contributor Author

[test]

}
}

if d.until == "start" {
return strategy.NewConditionReachedErr("Ready to start deployment")
Copy link
Contributor

Choose a reason for hiding this comment

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

Umm, these conditions are not errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're exceptions to the normal flow - it's handled at the top level.
Effectively we have defined a structured termination condition.

On Tue, May 10, 2016 at 11:55 AM, Michail Kargakis <[email protected]

wrote:

In pkg/cmd/infra/deployer/deployer.go
#8787 (comment):

    }
}
  • if d.until == "start" {
  •   return strategy.NewConditionReachedErr("Ready to start deployment")
    

Umm, these conditions are not errors.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8787/files/a96fb629083f11e94f9496eb3fd5d5fa15ab9245#r62698862

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2016
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2016
@0xmichalis
Copy link
Contributor

@ironcladlou I think that the patch in the rolling updater will help us move DeploymentConfigs to use ProgressDeadlineSeconds (obsoletes TimeoutSeconds in rolling settings and ActiveDeadlineSeconds in the deployer?). In the upstream controller we will use Conditions on the Deployment to track progress, here we can use annotations on the replication controllers.

@0xmichalis
Copy link
Contributor

In the upstream controller we will use Conditions on the Deployment to track progress, here we can use annotations on the replication controllers.

Or we could use Conditions on the deploymentconfig by decoding it once and then getting/updating it accordingly. I think I still prefer annotations on the rcs.

@0xmichalis
Copy link
Contributor

obsoletes TimeoutSeconds in rolling settings and ActiveDeadlineSeconds in the deployer?

Nevermind about ADS. I had a look at the rolling updater - TimeoutSeconds is almost exactly what ProgressDeadlineSeconds will be upstream. I may even change it to be exactly the same. So this patch doesn't help us the way I thought it would. But we already have PDS!:)

@smarterclayton
Copy link
Contributor Author

Only other comments on this?

if !strings.HasSuffix(until, "%") {
return 0, false
}
until = until[:len(until)-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you reuse PercentageBetween here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to get the percentage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, you can do the other way around.

@0xmichalis
Copy link
Contributor

Is there an upstream PR for the rolling updater?

@0xmichalis
Copy link
Contributor

Looks good in general

@smarterclayton
Copy link
Contributor Author

No I'll open once once you're comfortable with the changes.

On Fri, May 13, 2016 at 12:40 PM, Michail Kargakis <[email protected]

wrote:

Is there an upstream PR for the rolling updater?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8787 (comment)

@0xmichalis
Copy link
Contributor

I was thinking of exposing --until (or something similar to it that would direct the percentage of my rollout) via the deployment api. Then in combination with pausing/resuming and proportional scaling someone could do A/B with a single deploymentconfig. The deployer needs to not take into account the time it is paused.

@smarterclayton
Copy link
Contributor Author

I think that would be useful. Would be worth discussing upstream.

I'm going to extract the upstream PR now and squash.

On Sat, May 14, 2016 at 11:21 AM, Michail Kargakis <[email protected]

wrote:

I was thinking of exposing --until (or something similar to it that would
direct the percentage of my rollout) via the deployment api. Then in
combination with pausing/resuming and proportional scaling someone could do
A/B with a single deploymentconfig. The deployer needs to not take into
account the time it is paused.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8787 (comment)

@smarterclayton
Copy link
Contributor Author

Actually, not going to squash since the commits are mostly independent.
Ready for final review.

On Sat, May 14, 2016 at 1:33 PM, Clayton Coleman [email protected]
wrote:

I think that would be useful. Would be worth discussing upstream.

I'm going to extract the upstream PR now and squash.

On Sat, May 14, 2016 at 11:21 AM, Michail Kargakis <
[email protected]> wrote:

I was thinking of exposing --until (or something similar to it that would
direct the percentage of my rollout) via the deployment api. Then in
combination with pausing/resuming and proportional scaling someone could do
A/B with a single deploymentconfig. The deployer needs to not take into
account the time it is paused.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8787 (comment)

@0xmichalis
Copy link
Contributor

Still havent played with this PR but I have a question. I am running a 50% deployment. After it finishes and the deployer exits successfully, the deployment will be marked Complete and then the deploymentconfig controller on the next resync will scale it up to 100% and scale down the old 50%, right?

@0xmichalis
Copy link
Contributor

Actually if this is correct then the controller will not scale up the deployment but scale down the deploymentconfig because of the scaling support it provides to old clients

@smarterclayton
Copy link
Contributor Author

Yeah, you'd more than likely get the old behavior.

The expectation is that you'd always finish this script with "run to
completion".

On Sat, May 14, 2016 at 1:46 PM, Michail Kargakis [email protected]
wrote:

Actually if this is correct then the controller will not scale up the
deployment but scale down the deploymentconfig because of the scaling
support it provides to old clients


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8787 (comment)

Useful for iterative testing of deployment logic, or running an external
deployer.
When re-entering deployments, we may not need to even check the scale.
Allows a user to script a deployment to reach certain conditions, then
take action:

    $ openshift-deploy --until=pre
    --> pre: Running hook pod ...
    my hook pod ran
    --> pre: Success
    --> pre hook completed
    $ openshift-deploy --until=50%
    --> pre: Hook pod already succeeded
    --> Scaling deployment-2 from 0 to 5
        Scaling deployment-2 up to 3
        Scaling deployment-1 down to 2
    --> Reached 50% (currently 60%)
    $ sleep 5
    $ curl http://myservice:8080/metrics
    user metrics ok
    $ openshift-deploy
    --> pre: Hook pod already succeeded
    --> Scaling deployment-2 from 3 to 5
        Scaling deployment-2 up to 5
        Scaling deployment-1 down to 0
    --> Success

Will allow custom deployments
Allow customParams to be specified when type is Recreate or Rolling.
Allow image to be empty, add validation for resources and environment.
Add a custom deployment extended test.
Shows info about custom deployments that also having rolling update.
Makes multiline command printing better. Adds printing tag images hooks.
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f92fe67

@0xmichalis
Copy link
Contributor

LGTM

@smarterclayton
Copy link
Contributor Author

Will merge on green. Thanks

@openshift-bot
Copy link
Contributor

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

@smarterclayton
Copy link
Contributor Author

smarterclayton commented May 16, 2016 via email

@openshift-bot
Copy link
Contributor

openshift-bot commented May 16, 2016

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

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f92fe67

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.

4 participants