Implement deployments without Deployment #584

ironcladlou · 2014-12-17T19:42:44Z

What?

This PR implements deployments without using the Deployment API object. ReplicationControllers are used as a replacement for Deployment.

Why?

Since the metadata represented by Deployment can also be represented by ReplicationController annotations, the Deployment resource is a potentially unnecessary and complicating layer of indirection. Removing the resource would reduce the API surface area and bring tighter upstream integration. The original design discussion took place in #507.

Note that this PR does not actually remove Deployment from the public or internal API- that structure can be deprecated and removed separately.

TODO

ironcladlou · 2014-12-17T19:48:49Z

@smarterclayton @pmorie Still tracking down some issue in the e2e test preventing the image change trigger from causing a new deployment, but I think the bulk of the work is ready for general review.

ironcladlou · 2014-12-17T19:51:14Z

@jwforres Can you evaluate this for UI impact?

ironcladlou · 2014-12-17T20:14:25Z

The e2e test failure was fixed by pulling the latest registry image.

pmorie · 2014-12-17T22:27:49Z

@ironcladlou Does the deployment resource still have any meaning? It seems like part of this refactor should be to introduce the 'virtual resource' that we discussed for deployments.

jwforres · 2014-12-17T22:46:32Z

@ironcladlou UI can work against this with some minimal changes. All the key bits from deployment are there (what triggered me and what deploymentConfig I came from). I've got it working locally, can tweak as needed based on any updates to this PR.

ironcladlou · 2014-12-17T23:09:39Z

@pmorie

Does the deployment resource still have any meaning? It seems like part of this refactor should be to introduce the 'virtual resource' that we discussed for deployments.

The Deployment resource becomes vestigial with this PR. We could potentially adapt ReplicationControllers/Deployments, but that would be more design and work. Is it worth it?

ironcladlou · 2014-12-17T23:13:31Z

@jwforres

UI can work against this with some minimal changes. All the key bits from deployment are there (what triggered me and what deploymentConfig I came from). I've got it working locally, can tweak as needed based on any updates to this PR.

There's something I forgot to point out which is subtle but probably relevant: Previously, the ReplicationControllers were related to a point in time snapshot of a DeploymentConfig via Deployment. To replace that, I have to serialize the DeploymentConfig on which the RC is based into an annotation (DeploymentEncodedConfigAnnotation). A better way would be to store the DeploymentConfig.ResourceVersion on the RC, but we need upstream support for retrieving a resource by version.

Now that I've said it, I wonder how exactly you're going to access old versions of DeploymentConfigs and ReplicationControllers to display history... @smarterclayton ?

pmorie · 2014-12-17T23:15:11Z

@ironcladlou So, does 'list deployments' do anything?

smarterclayton · 2014-12-17T23:33:55Z

If we make the name of the rc deterministic (deployment config name + version) we can live with the convention for now

ironcladlou · 2014-12-17T23:34:00Z

@pmorie

So, does 'list deployments' do anything?

It will continue to function as usual: The API object still exists and is plumbed throughout the REST API and storage, but no deployment controllers make use of the object.

ironcladlou · 2014-12-17T23:36:52Z

@smarterclayton

If we make the name of the rc deterministic (deployment config name + version) we can live with the convention for now

That was the convention used for Deployment.Name, and I carried that over to ReplicationControllers. The main concern I have from my previous comment is how the UI is going to present history of any kind without the capability to List or Get old ReplicationControllers or DeploymentConfigs via the REST API pending upstream work for obtaining resources by ResourceVersion.

Is there anything we have aside from implementing certain List/Get interfaces with direct etcd access where necessary?

smarterclayton · 2014-12-17T23:57:18Z

Why do we need to see old versions of replication controllers? Deployment configs we can just identify as a gap that we want to fill

On Dec 17, 2014, at 6:36 PM, Dan Mace notifications@github.com wrote:

@smarterclayton

If we make the name of the rc deterministic (deployment config name + version) we can live with the convention for now

That was the convention used for Deployment.Name, and I carried that over to ReplicationControllers. The main concern I have from my previous comment is how the UI is going to present history of any kind without the capability to List or Get old ReplicationControllers or DeploymentConfigs via the REST API pending upstream work for obtaining resources by ResourceVersion.

Is there anything we have aside from implementing certain List/Get interfaces with direct etcd access where necessary?

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

ironcladlou · 2014-12-18T00:09:27Z

Why do we need to see old versions of replication controllers? Deployment configs we can just identify as a gap that we want to fill

You're right, there's no reason to see the old ReplicationControllers. I just want to confirm that this will be a regression (as we're effectively losing the ability to see old deployments). Having access to the history is also required for rollbacks, so this could introduce speed bumps for that feature.

smarterclayton · 2014-12-18T15:41:02Z

Rollbacks are almost always correlated in time with a deployment, so I don't think we'll lose much, except that the client needs to figure out what the proper RC is based on deployment config and name, and as we discussed that's more complex today.

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

Why do we need to see old versions of replication controllers? Deployment
configs we can just identify as a gap that we want to fill

You're right, there's no reason to see the old ReplicationControllers. I just
want to confirm that this will be a regression (as we're effectively losing
the ability to see old deployments). Having access to the history is also
required for rollbacks, so this could introduce speed bumps for that
feature.

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

ironcladlou · 2015-01-05T15:49:58Z

[test]

openshift-bot · 2015-01-05T15:56:42Z

Origin Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/699/)

ironcladlou · 2015-01-05T18:27:16Z

@pmorie @smarterclayton

Before I go revamp the godocs and such, this is ready for a review.

ironcladlou · 2015-01-05T20:28:54Z

[test] again

pmorie · 2015-01-05T22:16:53Z

@ironcladlou as we discussed earlier I think it would be best to re-implement the read-only operations for the Deployment resource during this PR, to avoid having a span of time where the deployments functionality and resource were still there, but didn't work at all.

Otherwise, the implementation you have here LGTM.

ironcladlou · 2015-01-06T20:16:16Z

@pmorie

as we discussed earlier I think it would be best to re-implement the read-only operations for the
Deployment resource during this PR, to avoid having a span of time where the deployments
functionality and resource were still there, but didn't work at all.

To recap offline talks: the Deployment resource will be deprecated, and no attempts will be made with this PR to virtualize access to RCs via Deployment. We'll decide at a later time whether to remove it outright or use it as an API abstraction.

ironcladlou · 2015-01-07T18:54:14Z

@jwforres, please review the UI commit.

jwforres · 2015-01-07T19:19:46Z

ui commit LGTM

ironcladlou · 2015-01-08T15:31:56Z

@smarterclayton I now use the runtime.Codec interface under the hood for encoding/decoding, and inject latest.Codec all the way from master through the factories/controllers. Also cleaned up the switch you pointed out.

ironcladlou · 2015-01-08T19:12:19Z

@smarterclayton Improved the annotation docs, and added a DeploymentVersionAnnotation to all ReplicationControllers for deployments which tracks the DeploymentConfig.LatestVersion on which the deployment was based.

ironcladlou · 2015-01-08T20:59:39Z

@pmorie @smarterclayton AFAIK all feedback is addressed, please give it another look.

ironcladlou · 2015-01-08T22:44:33Z

Also: I'm revamping the documentation for a separate PR, since it's a lot of work and will require its own discussion.

smarterclayton · 2015-01-08T22:47:59Z

Hope to finish review by tonight. It's in pretty good shape - what's your confidence on tests and e2e and general use?

ironcladlou · 2015-01-08T23:03:09Z

@smarterclayton

Hope to finish review by tonight. It's in pretty good shape - what's your confidence on tests and e2e and general use?

Pizazz levels are way up. e2e is fine, and I did manual testing with the console including builds and deployments. I can give it one last round of manual testing in the console tomorrow morning.

ironcladlou · 2015-01-08T23:08:56Z

General use is fine except for the lack of documentation, which I'm trying to address. We also have followup cards coming up immediately for nicer CLI and console stuff around deployments and rollbacks. There will be a period of discomfort dealing with RC's from the CLI, as annotations aren't rendered which are what you need to correlate stuff, but I consider that minor at the moment.

pmorie · 2015-01-09T16:55:45Z

@ironcladlou @smarterclayton pizzazz levels check out on this PR. going to use the PKE meter next 👻

ironcladlou · 2015-01-09T16:56:43Z

Follow-on for updated documentation: #631

pmorie · 2015-01-09T17:19:53Z

pkg/deploy/controller/deployment_controller_test.go

@@ -100,9 +104,9 @@ func TestHandleNewDeploymentCreatePodFail(t *testing.T) {
 				return nil, fmt.Errorf("Failed to create pod %s", pod.Name)
 			},
 		},
-		NextDeployment: func() *deployapi.Deployment {
+		NextDeployment: func() *kapi.ReplicationController {
 			deployment := basicDeployment()


Can you not use the OkDeployment method here?

Deprecate the Deployment resource and remplement deployment mechanics using ReplicationControllers instead of Deployment. The result is a deployment subsystem which adds fewer new concepts to Kubernetes while providing the same functionality as the former subsystem.

smarterclayton · 2015-01-09T18:22:11Z

LGTM [merge]

smarterclayton · 2015-01-09T18:22:18Z

What's the worst that can happen?

ironcladlou · 2015-01-09T18:27:03Z

@pmorie didn't finish his PKE readings... the PR could be haunted

pmorie · 2015-01-09T18:28:10Z

LGTM

openshift-bot · 2015-01-09T18:28:30Z

Origin Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/583/) (Image: devenv-fedora_524)

openshift-bot · 2015-01-09T18:28:31Z

Evaluated for origin up to 1964dfe

Merged by openshift-bot

smarterclayton · 2015-01-09T18:56:53Z

Dan, he's been dead the whole movie.

On Jan 9, 2015, at 1:28 PM, Paul Morie notifications@github.com wrote:

LGTM

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

ironcladlou changed the title ~~WIP implement deployments without Deployment~~ (Do not merge) WIP: Implement deployments without Deployment Dec 17, 2014

ironcladlou mentioned this pull request Dec 17, 2014

Replace Deployment with ReplicationController #507

Closed

ironcladlou force-pushed the rc-deployments-redux branch 2 times, most recently from 22e96cd to 02123e9 Compare January 5, 2015 15:38

ironcladlou force-pushed the rc-deployments-redux branch 3 times, most recently from 8dab37b to 02123e9 Compare January 5, 2015 17:50

ironcladlou force-pushed the rc-deployments-redux branch 3 times, most recently from 6bd61c4 to 39efec2 Compare January 7, 2015 18:53

ironcladlou changed the title ~~(Do not merge) WIP: Implement deployments without Deployment~~ Implement deployments without Deployment Jan 7, 2015

ironcladlou force-pushed the rc-deployments-redux branch from 39efec2 to 0eb0234 Compare January 8, 2015 15:19

ironcladlou force-pushed the rc-deployments-redux branch from 0eb0234 to 2dc416b Compare January 8, 2015 19:10

ironcladlou mentioned this pull request Jan 9, 2015

Rewrite and re-scope deployment documentation #631

Merged

pmorie reviewed Jan 9, 2015
View reviewed changes

ironcladlou force-pushed the rc-deployments-redux branch from 2dc416b to 1964dfe Compare January 9, 2015 17:28

openshift-bot pushed a commit that referenced this pull request Jan 9, 2015

Merge pull request #584 from ironcladlou/rc-deployments-redux

a0f73e9

Merged by openshift-bot

openshift-bot merged commit a0f73e9 into openshift:master Jan 9, 2015

ironcladlou deleted the rc-deployments-redux branch January 12, 2015 19:51

Implement deployments without Deployment #584

Implement deployments without Deployment #584

Conversation

ironcladlou commented Dec 17, 2014

What?

Why?

TODO

ironcladlou commented Dec 17, 2014

ironcladlou commented Dec 17, 2014

ironcladlou commented Dec 17, 2014

pmorie commented Dec 17, 2014

jwforres commented Dec 17, 2014

ironcladlou commented Dec 17, 2014

ironcladlou commented Dec 17, 2014

pmorie commented Dec 17, 2014

smarterclayton commented Dec 17, 2014

ironcladlou commented Dec 17, 2014

ironcladlou commented Dec 17, 2014

smarterclayton commented Dec 17, 2014

ironcladlou commented Dec 18, 2014

smarterclayton commented Dec 18, 2014

ironcladlou commented Jan 5, 2015

openshift-bot commented Jan 5, 2015

ironcladlou commented Jan 5, 2015

ironcladlou commented Jan 5, 2015

pmorie commented Jan 5, 2015

ironcladlou commented Jan 6, 2015

ironcladlou commented Jan 7, 2015

jwforres commented Jan 7, 2015

ironcladlou commented Jan 8, 2015

ironcladlou commented Jan 8, 2015

ironcladlou commented Jan 8, 2015

ironcladlou commented Jan 8, 2015

smarterclayton commented Jan 8, 2015

ironcladlou commented Jan 8, 2015

ironcladlou commented Jan 8, 2015

pmorie commented Jan 9, 2015

ironcladlou commented Jan 9, 2015

pmorie Jan 9, 2015

Choose a reason for hiding this comment

smarterclayton commented Jan 9, 2015

smarterclayton commented Jan 9, 2015

ironcladlou commented Jan 9, 2015

pmorie commented Jan 9, 2015

openshift-bot commented Jan 9, 2015

openshift-bot commented Jan 9, 2015

smarterclayton commented Jan 9, 2015