Add cleanup policy to RollingUpdater

[ironcladlou]

• Support configurable cleanup policies in RollingUpdater. Downstream
  library consumers don't necessarily have the same rules for post
  deployment cleanup; making the behavior policy driven is more flexible.
• Refactor RollingUpdater to accept a config object during Update instead
  of a long argument list.
• Add test coverage for cleanup policy.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[ironcladlou]

Release the bees!

@googlebot 🐝🐝🐝🐝🐝

[bgrant0607]

@ironcladlou Is this is step towards unification with Openshift's DeploymentConfig? Have you followed the discussion on #1743 at all?

I'll definitely take a look. I totally agree the parameters shouldn't come directly from flags. Ideally, I'd like the parameter block be suitable for inclusion in an API object (Deployment).

[ironcladlou]

@bgrant0607

@ironcladlou Is this is step towards unification with Openshift's DeploymentConfig? Have you followed the discussion on #1743 at all?

Perhaps indirectly. We need to support a rolling deployment strategy in the short term, and it would be nice if OpenShift could reuse the upstream RollingUpdater; however, the upstream code makes some assumptions which don't hold downstream:

1. The old RC should always be deleted
2. There is always an old RC (not the case during an initial deployment in OpenShift)

This PR addresses 1. To really reuse the code, we'd also need to address 2. If there's no interest in the modifications required to address 2, accepting this PR may not make sense in the context of the broader goal.

[ironcladlou]

@bgrant0607

I'll definitely take a look. I totally agree the parameters shouldn't come directly from flags. Ideally, I'd like the parameter block be suitable for inclusion in an API object (Deployment).

There will inevitably be such an API in OpenShift to represent many of the fields on the RollingUpdaterConfig described in this PR. It would definitely make sense to keep that as close to a proposed future upstream API as possible.

[smarterclayton]

I think the work in the short term leads naturally into prequel work for Deployment - this will force our two models closer together and expose any gaps we can highlight in the working code.

[ironcladlou]

@bgrant0607 Given the above comments, how do you feel about this change (and the followup I'd make to support deploying when there's no "old" RC)?

[bgrant0607]

Sorry. Will take a look in about 30 minutes.

[bgrant0607]

Should just be "Delete".

[ironcladlou]

There are a lot of files in this package. Any concerns about having sort of ambiguous constants like "Delete" and "Preserve" given the scope?

[bgrant0607]

I was just referring to the string values, not the variable names.

[bgrant0607]

This is the only thing I was waiting for, but I'm fine with this going in as is, so long as you don't mind this getting changed later.

[ironcladlou]

Ah! Changed.

[bgrant0607]

This change seems ok.

FYI, future changes coming down the road:

• a generator for the new RC (see Suggest a simple rolling update. #6713)
• support specifying the new RC by name rather than (in addition to) by file
• change the annotation to keep track of the number of instances added/removed rather than the desired number of replicas
• support rollback
• support multiple "old" RCs
• declaratively specify update parameters such as update period

cc @rjnagal @brendandburns

[bgrant0607]

In what scenario do you imagine there would be no old RC? Why wouldn't create rc be used?

[ironcladlou]

@bgrant0607

In what scenario do you imagine there would be no old RC? Why wouldn't create rc be used?

This is the event sequence in OpenShift:

1. User makes some change to a deployment configuration
2. OpenShift creates a deployment (a ReplicationController) to reflect the new change of configuration; the replica count of the deployment is 0
3. OpenShift launches a pod which executes a deployment "strategy" (e.g. rolling update); the strategy is given as input the current live deployment name and the new (inactive) deployment name
4. The strategy works to activate the new deployment and deactivate the prior deployment

The separation of concerns we've established means the rolling updater's create functionality is irrelevant in this context; our strategies don't create deployments. During the very first deployment process, there is no old deployment, only the new one created by OpenShift and handed to the strategy to activate.

I'm not saying one way or the other is preferable at this point, just trying to establish context for the use case to illustrate the conceptual gaps which are limiting RollingUpdater's reuse potential. If changing RollingUpdater to accommodate our needs in this way would be special-casing for a mechanism that would never make sense upstream given the plans for Kube deployments, maybe it makes more sense for us to DIY a rolling update strategy in the very short term.

cc @smarterclayton

[smarterclayton]

If someone deleted the old RC during a rolling update, if the user had to quit out and try to come back in, they may have no previous context, but would want to continue the scale up. I guess with the RC gone you'd probably lose the old label selector, so you couldn't scale anything old down, but you could finish the scale up.

When we have deployment as a first class kube object, the first deployment would have no previous version, but because of races you can't be sure of that. So the code that does the roll-out (based on whatever strategy inputs are provided) would at least need to be aware of the fact that the old RC might not exist anymore. I'd prefer to call into one code path (roll from X to Y) and have X not exist either be a short circuit or at least handled.

----- Original Message -----

@bgrant0607

In what scenario do you imagine there would be no old RC? Why wouldn't
create rc be used?

This is the event sequence in OpenShift:

1. User makes some change to a deployment configuration
2. OpenShift creates a deployment (a ReplicationController) to reflect the
   new change of configuration; the replica count of the deployment is 0
3. OpenShift launches a pod which executes a deployment "strategy" (e.g.
   rolling update); the strategy is given as input the current live deployment
   name and the new (inactive) deployment name
4. The strategy works to activate the new deployment and deactivate the prior
   deployment

The separation of concerns we've established means the rolling updater's
create functionality is irrelevant in this context; our strategies don't
create deployments. During the very first deployment process, there is no
old deployment, only the new one created by OpenShift and handed to the
strategy to activate.

I'm not saying one way or the other is preferable at this point, just trying
to establish context for the use case to illustrate the conceptual gaps
which are limiting RollingUpdater's reuse potential. If changing
RollingUpdater to accommodate our needs in this way would be special-casing
for a mechanism that would never make sense upstream given the plans for
Kube deployments, maybe it makes more sense for us to DIY a rolling update
strategy in the very short term.

cc @smarterclayton

---

Reply to this email directly or view it on GitHub:
#6996 (comment)

[bgrant0607]

Makes sense. I'd like to see support for 0 or more "old" RCs.

[bgrant0607]

LGTM

[bgrant0607]

Restarted Travis.