Enhance status for deploymentconfigs #6233

0xmichalis · 2015-12-08T14:30:19Z

Add various status fields for enhancing user experience. The fields are
also required for round-tripping upstream Deployments.

@ironcladlou @openshift/api-review PTAL

Fixes #5918
Part of https://trello.com/c/Mljldox7/643-5-deployments-downstream-support-for-upstream-features

@mfojtik @pweil- fyi

deads2k · 2015-12-08T14:42:02Z

pkg/deploy/controller/deploymentconfig/controller.go

@@ -137,7 +138,10 @@ func (c *DeploymentConfigController) Handle(config *deployapi.DeploymentConfig)
 		return fmt.Errorf("couldn't create deployment for deployment config %s: %v", deployutil.LabelForDeploymentConfig(config), err)
 	}
 	c.recorder.Eventf(config, "DeploymentCreated", "Created new deployment %q for version %d", created.Name, config.LatestVersion)
-	return nil
+	// Update deploymentConfig status; we probably need a separate call for status


If we're introducing the idea of a status, let's introduce a status at least internally. Since status should be read-only, we can be compatible by duplicating existing fields in two locations.

Does the deployer pod update logical status? If so, then we'll need to need to respect changes on it if the status stanza is missing. If not and its only the baked in controller, then we should be good to ignore status updates on old fields.

The deployer pod updates the status on the replication controller, it never touches the config. I think that status would be internal only, @ironcladlou thoughts?

The deployer pod updates the status on the replication controller, it never touches the config. I think that status would be internal only, @ironcladlou thoughts?

I'm fine with a deployer pod updating status. I just wanted to make sure we handled it properly for backwards compatibility. Sounds like we don't have to worry about it.

The strategies just scale RCs up and down and don't touch the config. They also have no reason to read the new field via public API or anything. So I don't think introducing status to the config should have an effect outside the controllers.

deads2k · 2015-12-09T14:28:50Z

pkg/deploy/api/types.go

+	Replicas int
+
+	// Selector is a label query over pods that should match the Replicas count.
+	Selector map[string]string


Is this new? What was the existing field?

can a single selector select all the replicas we care about? What if you are rolling between two deployments which resulted from two versions of the config, and have different selectors?

can a single selector select all the replicas we care about? What if you are rolling between two deployments which resulted from two versions of the config, and have different selectors?

I asked @Kargakis to split this into one pull for making spec/status with no other changes and then a separate pull to add the changes. I think this is new, so that should help us find and see what this is doing.

All of these fields are copied from our versioned objects.

@liggitt how would we end up with an old deployment having a different selector?

The new fields were behind DeploymentTemplate previously.

The new fields were behind DeploymentTemplate previously.

Ah, definitely interested in that separate pull then. Checking congruence in one and then reasoning about changes in another..

Couldn't we have updated the Selector of the DC and orphaned existing pods for an old RC created by this DC?

0xmichalis · 2015-12-22T16:01:38Z

pkg/deploy/controller/deploymentconfig/controller.go

@@ -137,7 +138,10 @@ func (c *DeploymentConfigController) Handle(config *deployapi.DeploymentConfig)
 		return fmt.Errorf("couldn't create deployment for deployment config %s: %v", deployutil.LabelForDeploymentConfig(config), err)
 	}
 	c.recorder.Eventf(config, "DeploymentCreated", "Created new deployment %q for version %d", created.Name, config.Status.LatestVersion)
-	return nil
+	// Update deploymentConfig status; we probably need a separate call for status


Opened #6465 for a separate status update call.

DirectXMan12 · 2015-12-22T16:14:46Z

pkg/deploy/registry/deployconfig/etcd/etcd.go

 	oldReplicas := deploymentConfig.Spec.Replicas
 	deploymentConfig.Spec.Replicas = scale.Spec.Replicas
 	if err := r.registry.UpdateDeploymentConfig(ctx, deploymentConfig); err != nil {
 		return nil, false, err
 	}
+
+	scaleRet.Spec.Replicas = scale.Spec.Replicas
 	scaleRet.Status.Replicas = totalReplicas + (scale.Spec.Replicas - oldReplicas)


With @ironcladlou's change to how DeploymentConfig scaling works, I don't think this calculation is accurate any more. We should just keep TotalReplicas == oldReplicas, since the replica count on RCs controlled by the DC won't change until the DC controller updates them.

0xmichalis · 2016-01-04T10:07:38Z

#6040 flake

0xmichalis · 2016-01-06T22:10:07Z

Error from server: 501: All the given peers are not reachable (failed to propose on members [https://172.18.4.185:4001] twice [last error: Unexpected HTTP status code]) [0]
!!! Error in hack/../test/end-to-end/core.sh:162
    'oc delete pod cli-with-token' exited with status 1
Call stack:
    1: hack/../test/end-to-end/core.sh:162 main(...)
Exiting with status 1

0xmichalis · 2016-01-12T14:56:50Z

pkg/deploy/controller/deploymentconfig/controller.go

+	// TODO: Use a separate call for updating the status of a DC
+	// https://github.com/openshift/origin/issues/6465
+	config.Status.Replicas = kdeputil.GetReplicaCountForRCs(deployments)
+	_, err := c.osClient.DeploymentConfigs(config.Namespace).Update(config)


@ironcladlou won't this result in a conflict if an older client had scaled the DC (L220)?

I think you're right- shouldn't we be reassigning config local after the update on L220?

I think you're right- shouldn't we be reassigning config local after the update on L220?

Not finding the conflict in a test, suggests we have a test missing.

It seems like this update is more properly a patch until we have separate status updating.

I think you're right- shouldn't we be reassigning config local after the update on L220?

Would using a copy of the dc in the last update call not conflict?

Not finding the conflict in a test, suggests we have a test missing.

We need to use older clients in our tests

Or in this specific case we could emulate an older client by manually updating the latest rc for a dc

smarterclayton · 2016-01-26T03:52:39Z

This API change is approved.

0xmichalis · 2016-01-28T02:22:13Z

I would say that this is blocked on #6465 (so we can avoid resource conflicts in case of old client updates) which in turn is blocked on #6337 (the trickiest one; planning to start working on it soon)

0xmichalis · 2016-06-22T16:33:40Z

Rebased on top of the latest controller changes, shared a pod cache, added an extended test. This will need another look @mfojtik @ironcladlou @smarterclayton @deads2k

0xmichalis · 2016-06-23T09:10:34Z

[test]

openshift-bot · 2016-06-23T11:05:35Z

Evaluated for origin test up to 0e85054

openshift-bot · 2016-06-23T12:22:54Z

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

mfojtik · 2016-06-23T14:49:00Z

LGTM

0xmichalis · 2016-06-23T14:52:32Z

[merge]

0xmichalis · 2016-06-23T14:53:59Z

This will also enable oc rollout status.

openshift-bot · 2016-06-23T14:55:36Z

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

openshift-bot · 2016-06-23T14:55:37Z

Evaluated for origin merge up to 0e85054

deads2k reviewed Dec 8, 2015
View reviewed changes

0xmichalis changed the title ~~api: New deploymentConfig field~~ [WIP] api: New deploymentConfig field Dec 8, 2015

deads2k reviewed Dec 9, 2015
View reviewed changes

0xmichalis mentioned this pull request Dec 9, 2015

deployapi: Refactor internal objects to match versioned #6246

Merged

4 tasks

0xmichalis changed the title ~~[WIP] api: New deploymentConfig field~~ api: New deploymentConfig field Dec 22, 2015

0xmichalis reviewed Dec 22, 2015
View reviewed changes

0xmichalis mentioned this pull request Dec 22, 2015

DeploymentConfig Status Update #6465

Closed

DirectXMan12 reviewed Dec 22, 2015
View reviewed changes

0xmichalis mentioned this pull request Dec 22, 2015

k8s rebase - 4a65fa1f35e98ae96785836d99bf4ec7712ab682 #6320

Merged

46 tasks

0xmichalis reviewed Jan 12, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2016

0xmichalis changed the title ~~api: New deploymentConfig field~~ [WIP] api: New deploymentConfig field Jan 28, 2016

0xmichalis mentioned this pull request Feb 4, 2016

Allow new-app to create test deployments #7010

Merged

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 4, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 19, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 9, 2016

0xmichalis added the blocked label May 31, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 31, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2016

0xmichalis mentioned this pull request Jun 9, 2016

Deployment Hook Status Reporting #9260

Closed

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 13, 2016

0xmichalis referenced this pull request Jun 20, 2016

Update status only when needed

3d83c4f

0xmichalis mentioned this pull request Jun 20, 2016

Deployment issues for 1.3 #9450

Closed

10 tasks

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2016

0xmichalis removed the blocked label Jun 22, 2016

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2016

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2016

0xmichalis added 2 commits June 23, 2016 12:47

deploy: generated code for deploymentconfig status enhancements

a0ac96f

deploy: enhance status for deploymentconfigs

0e85054

openshift-bot merged commit cc9cf0e into openshift:master Jun 23, 2016

0xmichalis deleted the dc-new-api-field branch June 23, 2016 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance status for deploymentconfigs #6233

Enhance status for deploymentconfigs #6233

0xmichalis commented Dec 8, 2015 •

edited

Loading

deads2k Dec 8, 2015

0xmichalis Dec 8, 2015

deads2k Dec 8, 2015

ironcladlou Dec 8, 2015

deads2k Dec 9, 2015

liggitt Dec 9, 2015

deads2k Dec 9, 2015

0xmichalis Dec 9, 2015

0xmichalis Dec 9, 2015

deads2k Dec 9, 2015

0xmichalis Dec 22, 2015

DirectXMan12 Dec 22, 2015

0xmichalis commented Jan 4, 2016

0xmichalis commented Jan 6, 2016

0xmichalis Jan 12, 2016

ironcladlou Jan 12, 2016

deads2k Jan 12, 2016

0xmichalis Jan 12, 2016

0xmichalis Jan 12, 2016

0xmichalis Jan 12, 2016

smarterclayton commented Jan 26, 2016

0xmichalis commented Jan 28, 2016

0xmichalis commented Jun 22, 2016

0xmichalis commented Jun 23, 2016

openshift-bot commented Jun 23, 2016

openshift-bot commented Jun 23, 2016

mfojtik commented Jun 23, 2016

0xmichalis commented Jun 23, 2016

0xmichalis commented Jun 23, 2016

openshift-bot commented Jun 23, 2016 •

edited

Loading

openshift-bot commented Jun 23, 2016

Enhance status for deploymentconfigs #6233

Enhance status for deploymentconfigs #6233

Conversation

0xmichalis commented Dec 8, 2015 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jan 4, 2016

0xmichalis commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jan 26, 2016

0xmichalis commented Jan 28, 2016

0xmichalis commented Jun 22, 2016

0xmichalis commented Jun 23, 2016

openshift-bot commented Jun 23, 2016

openshift-bot commented Jun 23, 2016

mfojtik commented Jun 23, 2016

0xmichalis commented Jun 23, 2016

0xmichalis commented Jun 23, 2016

openshift-bot commented Jun 23, 2016 • edited Loading

openshift-bot commented Jun 23, 2016

0xmichalis commented Dec 8, 2015 •

edited

Loading

openshift-bot commented Jun 23, 2016 •

edited

Loading