Support absence of source RC in RollingUpdater #7183

ironcladlou · 2015-04-22T18:49:09Z

This is some POC work to show what's involved in changing RollingUpdater to support the absence of an "old" ReplicationController. These changes would also allow OpenShift to make use of RollingUpdater by pre-processing our RCs to include the annotations RollingUpdater expects.

I'm not crazy about having to set annotations, but I'd rather do that downstream than change the fundamental operating principals of this code. I'm also not entirely pleased with how this refactored code reads (feels modal), but at least there's something to talk about.

cc @smarterclayton @bgrant0607

googlebot · 2015-04-22T18:49:10Z

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 · 2015-04-23T20:39:05Z

@bgrant0607 @smarterclayton thoughts?

bgrant0607 · 2015-04-23T22:28:37Z

If you haven't already seen it, take a look at:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/design/simple-rolling-update.md

cc @brendandburns, who's also working on RCs

bgrant0607 · 2015-04-23T22:32:10Z

pkg/kubectl/rolling_updater.go

+			oldRc.Spec.Replicas -= 1
+		}
+
+		fmt.Printf("At beginning of loop: %s\n", rollingLabelFor(newRc, oldRc))


FWIW, there should be a way to turn off this output, for this and all other kubectl operations.

On Apr 23, 2015, at 6:32 PM, Brian Grant notifications@github.com wrote:

In pkg/kubectl/rolling_updater.go:

newRc.Spec.Replicas += 1

oldRc.Spec.Replicas -= 1

fmt.Printf("At beginning of loop: %s replicas: %d, %s replicas: %d\n",

oldName, oldRc.Spec.Replicas,

newName, newRc.Spec.Replicas)

fmt.Fprintf(out, "Updating %s replicas: %d, %s replicas: %d\n",

oldName, oldRc.Spec.Replicas,

newName, newRc.Spec.Replicas)

if oldRc != nil {

oldRc.Spec.Replicas -= 1

}

fmt.Printf("At beginning of loop: %s\n", rollingLabelFor(newRc, oldRc))
FWIW, there should be a way to turn off this output, for this and all other kubectl operations.

Make RollingUpdater have an io.Writer member and write with Fprint
—
Reply to this email directly or view it on GitHub.

bgrant0607 · 2015-04-23T22:37:25Z

Is there any reason to add replicas gradually when there was no original RC? I'm aware of a few such cases internally, but it's not very common. It would decrease the modality of the code if that case just short-circuited by calling create.

That said, this is not unreasonable, IMO.

smarterclayton · 2015-04-23T22:43:16Z

That's not unreasonable. I can think of a few cases as well (thundering herd style rollouts hammering a db) but it just seems unlikely.

On Apr 23, 2015, at 6:37 PM, Brian Grant notifications@github.com wrote:

Is there any reason to add replicas gradually when there was no original RC? I'm aware of a few such cases internally, but it's not very common. It would decrease the modality of the code if that case just short-circuited by calling create.

That said, this is not unreasonable, IMO.

—
Reply to this email directly or view it on GitHub.

ironcladlou · 2015-04-24T13:31:06Z

@bgrant0607 @smarterclayton

Is there any reason to add replicas gradually when there was no original RC?

Clayton and I had the same discussion the other day. Sounds like we all agree that we can just immediately scale up when there's no old RC for a transition. The code would definitely be less modal, which is a bonus.

Thanks for the feedback!

ironcladlou · 2015-04-24T15:01:16Z

@bgrant0607 @smarterclayton

Refactored based on yesterday's discussion. I've been taking a look at the simple rolling update doc and what I have here now doesn't quite fit the contract outlined by @brendanburns. For example, the document describes a "Rename" behavior for when there's no source RC, which is not represented in master or my branch.

I take it the document is a plan for future work?

smarterclayton · 2015-04-24T16:14:43Z

I cc'd you on the pull that does that

On Apr 24, 2015, at 11:01 AM, Dan Mace notifications@github.com wrote:

@bgrant0607 @smarterclayton

Refactored based on yesterday's discussion. I've been taking a look at the simple rolling update doc and what I have here now doesn't quite fit the contract outlined by @brendanburns. For example, the document describes a "Rename" behavior for when there's no source RC, which is not represented in master or my branch.

I take it the document is a plan for future work?

—
Reply to this email directly or view it on GitHub.

ironcladlou · 2015-04-24T18:37:09Z

I'll rebase this when #7279 lands.

bgrant0607 · 2015-04-24T20:08:45Z

Just pointing out that I don't think there will be a way to express this scenario via the kubectl command line. I'm fine with that.

In general, I'd like kubectl to be better factored for use as a library.

ironcladlou · 2015-04-27T13:11:58Z

Rebased.

bgrant0607 · 2015-04-28T00:35:37Z

Assigning to @brendandburns, since he's active in this part of the code at the moment.

ironcladlou · 2015-04-28T17:11:04Z

@brendanburns Any thoughts on this?

bgrant0607 · 2015-04-28T18:45:37Z

Or @jlowdermilk, since he reviewed the other PRs.

j3ffml · 2015-04-28T18:52:45Z

I don't quite understand the reason for using rolling-update if there's no existing rc... Why not just use create or run-container? Is it to pick up the annotations added by rollingupdate? But those are ephemeral and removed after the update finishes anyway...

bgrant0607 · 2015-04-28T19:16:56Z

@jlowdermilk It's to simplify a client layered on top, which mostly will be doing updates, except the first time, where it needs to create the RC. This may be useful for us, as well, for implementing Deployment (#1743). See discussion in #6996.

ironcladlou · 2015-04-29T19:52:39Z

@jlowdermilk @bgrant0607 Did you both reach consensus on this?

j3ffml · 2015-04-29T20:20:50Z

pkg/kubectl/rolling_updater.go

@@ -138,11 +149,32 @@ func (r *RollingUpdater) Update(config *RollingUpdaterConfig) error {
 		}
 	}

+	if oldRc == nil {
+		// There's no source RC, so just scale up the new immediately and return


This block is identical to the final resize step for the general case (line 208). Should make it a function to avoid code duplication.

Extracted a function.

When performing a rolling update, scale up the new RC immediately in the absence of a source RC. This is to handle initial deployments and any other case where the source RC either never existed or was otherwise removed from the system. With no source RC, there's no reason to do a gradual transition.

j3ffml · 2015-04-29T20:53:59Z

pkg/kubectl/rolling_updater.go

+		return rc, true, fmt.Errorf("Missing %s annotation for controller %s", DesiredReplicasAnnotation, name)
+	}
+
+	// If there's no source associated with the RC, don't validate association.


Hmmm. this logic needs tweaking; if the newRc has a sourceIdAnnotation but we didn't provide a sourceId, we should probably return an error, as that means we detected a partial rollout from an existing oldRc that the user (perhaps accidentally) didn't specify. I think the cases should be

no sourceId provided, but newRc has sourceIdAnnotation: return error - partial rollout from oldName detected, but no starting rc specified.

sourceId provided, but newRc doesn't have sourceIdAnnotation, or annotation doesn't match: return error - missing/unexpected annotation

sourceId provided and matches newRc annotation: success, return rc

sourceId not provided, and newRc has no annotation: success, return rc.

ironcladlou · 2015-04-29T21:07:03Z

@jlowdermilk I'm starting to wonder whether it's really a big deal for consumers to just use a straight up resize for initial deployments and only use the rolling updater when there are two RC's to transition. You alluded to this previously. The modal complexity is starting to add up, especially since the rename stuff landed. What do you think?

j3ffml · 2015-04-29T21:21:56Z

@ironcladlou, I think using resize for initial deployments makes the most sense from an end user perspective. IMO we should be optimizing the client for end users not layered clients.

ironcladlou · 2015-04-29T21:23:38Z

@bgrant0607 @smarterclayton Thoughts? The way this is headed, I'm prepared to retract this request and use a combination of resize and rolling updates to handle initial/subsequent rollouts.

smarterclayton · 2015-04-29T22:25:07Z

My overriding concern is having a simple, easy to use command that can do the "go from X to Y". Because the reality of the world intrudes, X may sometimes get deleted, or renamed, or disappear. If we want to make it easy to rely on rolling-update for the core of a true deployment object, and also be able to script and customize your deployment logic (using bash + rolling-update or Go plus the library), we should ensure that we don't end up with two completely different implementations of rolling updater - one we maintain, and the cobbled together nightmare in bash an end user who needs "small tweak X" has to maintain.

If it's easy in bash to script "resize if it doesn't exist, tweak replicas slowly over time if it does", I think we're ok. If it requires deep knowledge of bash and the surface areas of our commands, I think we're not.

/soapbox

On Apr 29, 2015, at 5:24 PM, Dan Mace notifications@github.com wrote:

@bgrant0607 @smarterclayton Thoughts? The way this is headed, I'm prepared to retract this request and use a combination of resize and rolling updates to handle initial/subsequent rollouts.

—
Reply to this email directly or view it on GitHub.

bgrant0607 · 2015-05-02T19:48:01Z

Sorry, I've been OOO. I'll take a look by/on Monday.

I agree that the TOOL should be targeted at end users, including use in scripts. However, the LIBRARY should support layered/embedded uses, only one of which is the tool. That said, we want the library to be flexible, but not overly complicated.

One could imagine layering rolling update on top of resize, but resize is simple enough that it's probably not worth it.

bgrant0607 · 2015-05-02T19:52:04Z

We also need to keep an eye on #7575. It's probably worth sketching out the matrix of scenarios we expect to support.

bgrant0607 · 2015-05-06T21:35:32Z

#7575 was merged.

bgrant0607 · 2015-05-06T21:43:19Z

The other rolling update PRs in flight have landed.

This looks reasonably clean and self-contained. Please rebase, resolve comments, and get it merged.

ironcladlou · 2015-05-08T20:28:45Z

I'm in the middle of integrating the current RollingUpdater with OpenShift (here). During the course of integrating, I uncovered one more mismatch of assumptions related to the existing RC replica count validation (see here).

When I'm finished integrating, I'll revisit this PR.

bgrant0607 · 2015-07-27T06:57:25Z

Is this PR still relevant?

cc @nikhiljindal

ironcladlou · 2015-07-27T13:47:50Z

I think this particular implementation of the concept has fallen too far behind other refactors to remain useful without a lot of rework, and I'm presently focused on other aspects of rolling updates. If we agree the concept is still relevant/desired, does it make sense to "downgrade" the PR to an issue so that a later PR could address it?

In any case, this PR is going to languish and should probably be closed for now.

j3ffml · 2015-07-27T16:14:58Z

Closing as out of date. @ironcladlou, please open an issue for this feature so we can triage and get to it later.

googlebot added the cla: no label Apr 22, 2015

bgrant0607 reviewed Apr 23, 2015
View reviewed changes

davidopp assigned bgrant0607 Apr 24, 2015

ironcladlou force-pushed the rolling-updater-handle-missing-old branch 4 times, most recently from 764e573 to 37f5e9e Compare April 24, 2015 14:56

ironcladlou force-pushed the rolling-updater-handle-missing-old branch from 37f5e9e to b5bbecd Compare April 24, 2015 16:54

ironcladlou changed the title ~~WIP: Support a lack of old RC in RollingUpdater~~ Support absence of source RC in RollingUpdater Apr 24, 2015

ironcladlou force-pushed the rolling-updater-handle-missing-old branch from b5bbecd to d4fc660 Compare April 27, 2015 13:11

bgrant0607 assigned brendandburns and unassigned bgrant0607 Apr 28, 2015

bgrant0607 assigned j3ffml and unassigned brendandburns Apr 28, 2015

j3ffml reviewed Apr 29, 2015
View reviewed changes

ironcladlou force-pushed the rolling-updater-handle-missing-old branch from d4fc660 to 70e1f81 Compare April 29, 2015 20:36

ironcladlou force-pushed the rolling-updater-handle-missing-old branch from 70e1f81 to 2d68c1c Compare April 29, 2015 20:39

j3ffml reviewed Apr 29, 2015
View reviewed changes

j3ffml added the status/icebox label May 11, 2015

thockin removed the status/icebox label May 29, 2015

erictune added this to the v1.0-post milestone Jun 1, 2015

bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015

j3ffml closed this Jul 27, 2015

ironcladlou deleted the rolling-updater-handle-missing-old branch July 28, 2015 17:17

ironcladlou unassigned j3ffml Aug 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support absence of source RC in RollingUpdater #7183

Support absence of source RC in RollingUpdater #7183

ironcladlou commented Apr 22, 2015

googlebot commented Apr 22, 2015

ironcladlou commented Apr 23, 2015

bgrant0607 commented Apr 23, 2015

bgrant0607 Apr 23, 2015

smarterclayton Apr 23, 2015

bgrant0607 commented Apr 23, 2015

smarterclayton commented Apr 23, 2015

ironcladlou commented Apr 24, 2015

ironcladlou commented Apr 24, 2015

smarterclayton commented Apr 24, 2015

ironcladlou commented Apr 24, 2015

bgrant0607 commented Apr 24, 2015

ironcladlou commented Apr 27, 2015

bgrant0607 commented Apr 28, 2015

ironcladlou commented Apr 28, 2015

bgrant0607 commented Apr 28, 2015

j3ffml commented Apr 28, 2015

bgrant0607 commented Apr 28, 2015

ironcladlou commented Apr 29, 2015

j3ffml Apr 29, 2015

ironcladlou Apr 29, 2015

j3ffml Apr 29, 2015

ironcladlou commented Apr 29, 2015

j3ffml commented Apr 29, 2015

ironcladlou commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

bgrant0607 commented May 2, 2015

bgrant0607 commented May 2, 2015

bgrant0607 commented May 6, 2015

bgrant0607 commented May 6, 2015

ironcladlou commented May 8, 2015

bgrant0607 commented Jul 27, 2015

ironcladlou commented Jul 27, 2015

j3ffml commented Jul 27, 2015

Support absence of source RC in RollingUpdater #7183

Support absence of source RC in RollingUpdater #7183

Conversation

ironcladlou commented Apr 22, 2015

googlebot commented Apr 22, 2015

ironcladlou commented Apr 23, 2015

bgrant0607 commented Apr 23, 2015

bgrant0607 Apr 23, 2015

Choose a reason for hiding this comment

smarterclayton Apr 23, 2015

Choose a reason for hiding this comment

bgrant0607 commented Apr 23, 2015

smarterclayton commented Apr 23, 2015

ironcladlou commented Apr 24, 2015

ironcladlou commented Apr 24, 2015

smarterclayton commented Apr 24, 2015

ironcladlou commented Apr 24, 2015

bgrant0607 commented Apr 24, 2015

ironcladlou commented Apr 27, 2015

bgrant0607 commented Apr 28, 2015

ironcladlou commented Apr 28, 2015

bgrant0607 commented Apr 28, 2015

j3ffml commented Apr 28, 2015

bgrant0607 commented Apr 28, 2015

ironcladlou commented Apr 29, 2015

j3ffml Apr 29, 2015

Choose a reason for hiding this comment

ironcladlou Apr 29, 2015

Choose a reason for hiding this comment

j3ffml Apr 29, 2015

Choose a reason for hiding this comment

ironcladlou commented Apr 29, 2015

j3ffml commented Apr 29, 2015

ironcladlou commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

bgrant0607 commented May 2, 2015

bgrant0607 commented May 2, 2015

bgrant0607 commented May 6, 2015

bgrant0607 commented May 6, 2015

ironcladlou commented May 8, 2015

bgrant0607 commented Jul 27, 2015

ironcladlou commented Jul 27, 2015

j3ffml commented Jul 27, 2015