Update `oc idle` to patch dc's and rc's instead of update #11634

juanvallejo · 2016-10-27T21:12:57Z

Fixes #11622 as well as a minor bug when running the command without --dry-run
cc @openshift/cli-review @Kargakis

juanvallejo · 2016-10-27T21:35:59Z

[test]

0xmichalis · 2016-10-28T12:08:03Z

pkg/unidling/util/scale.go

@@ -90,16 +92,50 @@ func (c *ScaleAnnotater) UpdateObjectScale(namespace string, ref unidlingapi.Cro
 		if typedObj.Annotations == nil {
 			typedObj.Annotations = make(map[string]string)
 		}
+
+		originalObj, err := json.Marshal(typedObj)


@mfojtik @soltysh since you have been into patching lately, should this be versioned or not?

why not use an encoder instead of raw json package?

0xmichalis · 2016-10-28T12:08:23Z

@mfojtik i would like to have this for 1.4

juanvallejo · 2016-10-28T17:26:46Z

pkg/unidling/controller/controller.go

@@ -325,7 +326,8 @@ func (c *UnidlingController) handleRequest(info types.NamespacedName, lastFired

 		scale.Spec.Replicas = scalableRef.Replicas

-		if err = scaleAnnotater.UpdateObjectScale(info.Namespace, scalableRef.CrossGroupObjectReference, obj, scale); err != nil {
+		codec := kapi.Codecs.LegacyCodec(registered.EnabledVersions()...)


@liggitt Switched to use runtime.Encode(), however I am not sure if this is the right way of obtaining a codec here

juanvallejo · 2016-10-28T21:34:42Z

@liggitt
When I use a runtime encoder cmdutil.Factory.JSONEncoder() instead of just marshaling the object with the json package, I receive an error when running oc idle on an endpoint:

$ oc idle docker-registry
Marked service default/docker-registry to unidle resource DeploymentConfig default/docker-registry
(unidle to 1 replicas)
error: unable to scale DeploymentConfig default/docker-registry to 0, but still listed as target for unidling: unable to find api field in struct DeploymentConfig for the json field "metadata"

Am I using the wrong encoder?

deploymentconfig object when using json.Marshal:

{"name":"docker-registry","namespace":"default","selfLink":"/oapi/v1/namespaces/default/deploymentconfigs/docker-registry","uid":"19fdb36f-9d51-11e6-9e27-507b9dac96e1","resourceVersion":"588","generation":1,"creationTimestamp":"2016-10-28T20:57:13Z","labels":{"docker-registry":"default"},"annotations":{"idling.alpha.openshift.io/idled-at":"2016-10-28T21:26:16Z","idling.alpha.openshift.io/previous-scale":"1"},"Spec":{"Strategy":{"Type":"Rolling","CustomParams":null,"RecreateParams":null,"RollingParams":{"UpdatePeriodSeconds":1,"IntervalSeconds":1,"TimeoutSeconds":600,"MaxUnavailable":"25%","MaxSurge":"25%","Pre":null,"Post":null},"Resources":{},"Labels":null,"Annotations":null},"MinReadySeconds":0,"Triggers":[{"Type":"ConfigChange","ImageChangeParams":null}],"Replicas":0,"RevisionHistoryLimit":null,"Test":false,"Paused":false,"Selector":{"docker-registry":"default"},"Template":{"metadata":{"creationTimestamp":null,"labels":{"docker-registry":"default"}},"spec":{"volumes":[{"name":"registry-storage","emptyDir":{}}],"containers":[{"name":"registry","image":"openshift/origin-docker-registry:v1.3.0-rc1","ports":[{"containerPort":5000,"protocol":"TCP"}],"env":[{"name":"REGISTRY_HTTP_ADDR","value":":5000"},{"name":"REGISTRY_HTTP_NET","value":"tcp"},{"name":"REGISTRY_HTTP_SECRET","value":"3hlMyRNRzcGziBH4tFIBH8RVvnyX5q6BLmG0rFpPZZ0="},{"name":"REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_ENFORCEQUOTA","value":"false"}],"resources":{"requests":{"cpu":"100m","memory":"256Mi"}},"volumeMounts":[{"name":"registry-storage","mountPath":"/registry"}],"livenessProbe":{"httpGet":{"path":"/healthz","port":5000,"scheme":"HTTP"},"initialDelaySeconds":10,"timeoutSeconds":5,"periodSeconds":10,"successThreshold":1,"failureThreshold":3},"readinessProbe":{"httpGet":{"path":"/healthz","port":5000,"scheme":"HTTP"},"timeoutSeconds":5,"periodSeconds":10,"successThreshold":1,"failureThreshold":3},"terminationMessagePath":"/dev/termination-log","imagePullPolicy":"IfNotPresent","securityContext":{"privileged":false}}],"restartPolicy":"Always","terminationGracePeriodSeconds":30,"dnsPolicy":"ClusterFirst","serviceAccountName":"registry","securityContext":{}}}},"Status":{"LatestVersion":1,"ObservedGeneration":1,"Replicas":1,"UpdatedReplicas":1,"AvailableReplicas":1,"UnavailableReplicas":0,"Details":{"Message":"caused by a config change","Causes":[{"Type":"ConfigChange","ImageTrigger":null}]},"Conditions":null}}

same object when using f.JSONEncoder():

{"kind":"DeploymentConfig","apiVersion":"v1","metadata":{"name":"docker-registry","namespace":"default","selfLink":"/oapi/v1/namespaces/default/deploymentconfigs/docker-registry","uid":"19fdb36f-9d51-11e6-9e27-507b9dac96e1","resourceVersion":"588","generation":1,"creationTimestamp":"2016-10-28T20:57:13Z","labels":{"docker-registry":"default"}},"spec":{"strategy":{"type":"Rolling","rollingParams":{"updatePeriodSeconds":1,"intervalSeconds":1,"timeoutSeconds":600,"maxUnavailable":"25%","maxSurge":"25%"},"resources":{}},"triggers":[{"type":"ConfigChange"}],"replicas":1,"test":false,"selector":{"docker-registry":"default"},"template":{"metadata":{"creationTimestamp":null,"labels":{"docker-registry":"default"}},"spec":{"volumes":[{"name":"registry-storage","emptyDir":{}}],"containers":[{"name":"registry","image":"openshift/origin-docker-registry:v1.3.0-rc1","ports":[{"containerPort":5000,"protocol":"TCP"}],"env":[{"name":"REGISTRY_HTTP_ADDR","value":":5000"},{"name":"REGISTRY_HTTP_NET","value":"tcp"},{"name":"REGISTRY_HTTP_SECRET","value":"3hlMyRNRzcGziBH4tFIBH8RVvnyX5q6BLmG0rFpPZZ0="},{"name":"REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_ENFORCEQUOTA","value":"false"}],"resources":{"requests":{"cpu":"100m","memory":"256Mi"}},"volumeMounts":[{"name":"registry-storage","mountPath":"/registry"}],"livenessProbe":{"httpGet":{"path":"/healthz","port":5000,"scheme":"HTTP"},"initialDelaySeconds":10,"timeoutSeconds":5,"periodSeconds":10,"successThreshold":1,"failureThreshold":3},"readinessProbe":{"httpGet":{"path":"/healthz","port":5000,"scheme":"HTTP"},"timeoutSeconds":5,"periodSeconds":10,"successThreshold":1,"failureThreshold":3},"terminationMessagePath":"/dev/termination-log","imagePullPolicy":"IfNotPresent","securityContext":{"privileged":false}}],"restartPolicy":"Always","terminationGracePeriodSeconds":30,"dnsPolicy":"ClusterFirst","serviceAccountName":"registry","serviceAccount":"registry","securityContext":{}}}},"status":{"latestVersion":1,"observedGeneration":1,"replicas":1,"updatedReplicas":1,"availableReplicas":1,"details":{"message":"caused by a config change","causes":[{"type":"ConfigChange"}]}}}

cc @fabianofranz

EDIT:

I receicve an empty array of bytes when generating a StrategicMergePatchType with f.JSONEncoder().
The patch that is generated when using json.Marshal is:

{"Spec":{"Replicas":0},"annotations":{"idling.alpha.openshift.io/idled-at":"2016-10-31T19:11:43Z","idling.alpha.openshift.io/previous-scale":"1"}}

0xmichalis · 2016-10-29T00:34:43Z

Also fixes #10988

juanvallejo · 2016-10-31T21:54:19Z

cc @DirectXMan12 @fabianofranz @smarterclayton

Per Solly's suggestion, I have added a ScaleUpdater interface with an Update() method that allows to update or patch a resource depending on what is needed. It was not seen as a good idea to patch resources [when calling UpdateObjectScale() fromunidling/controller/controller.go](https://github.com/openshift/origin/blob/master/pkg/unidling/controller/controller.go#L328), so the implementation with Patching is only seen in cmd/idle.go`

fabianofranz · 2016-11-01T13:45:56Z

Win, now this is looking like properly architected. I imagine this is already covered by tests so we don't need to write new ones, correct?
I'll defer to @DirectXMan12 a final review.

smarterclayton · 2016-11-01T13:53:49Z

Can I get a quick summary of why we can't use patch in scale? Do we need
precondition scale?

On Nov 1, 2016, at 9:46 AM, Fabiano Franz notifications@github.com wrote:

Win, now this is looking like properly architected. I imagine this is
already covered by tests so we don't need to write new ones, correct?
I'll defer to @DirectXMan12 https://github.com/DirectXMan12 a final
review.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11634 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_pzZneCgEspPFhlpt8bb1jcazvfs2ks5q50KYgaJpZM4Ki1Nq
.

liggitt · 2016-11-01T13:56:42Z

Patch on scale with name/namespace/uid set should work correctly.

smarterclayton · 2016-11-01T14:03:08Z

I think this is patch under scale - an unrelated change should not block
scaling. Probably we should be doing scale with no resource version
precondition

On Nov 1, 2016, at 9:56 AM, Jordan Liggitt notifications@github.com wrote:

Patch on scale with name/namespace/uid/resourceVersion set should work
correctly.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11634 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p0Jd3dC3UlS8UrVqMGcrYlzH1hnYks5q50UdgaJpZM4Ki1Nq
.

liggitt · 2016-11-01T14:05:44Z

Yeah, edited that one second after making that comment

juanvallejo · 2016-11-01T17:49:25Z

@liggitt @smarterclayton Should I update unidling/controller/controller.go to use Patch() as well?

smarterclayton · 2016-11-01T18:06:12Z

This is a problem with scale, not with the clients of Scale. If this is a
blind scale, then we should be ignoring the existing value of scale, and we
should not be sending a resource version when we POST the scale resource,
and the scale resource on the server should use patch under the covers to
ignore what else the user provides.

On Tue, Nov 1, 2016 at 1:49 PM, Juan Vallejo notifications@github.com
wrote:

@liggitt https://github.com/liggitt @smarterclayton
https://github.com/smarterclayton Should I update unidling/controller/
controller.go
https://github.com/openshift/origin/blob/master/pkg/unidling/controller/controller.go#L328
to patch as well?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11634 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p6OVGwgZ7zTRCabxH1kf2wSwyE7qks5q53uogaJpZM4Ki1Nq
.

juanvallejo · 2016-11-02T18:24:46Z

@smarterclayton @DirectXMan12 updated unidling/controller/controller.go to use Patch() as well

The only times Patch() is not used is when the resource kind is unknown

PTAL

DirectXMan12

I don't necessarily want to block this PR, but I'd prefer that the ScaleUpdater not be touching the innards (fields) of the ScaleAnnotater. That seems ugly to me. Otherwise, this is probably fine.

juanvallejo · 2016-11-02T19:41:04Z

@DirectXMan12 Thanks for the feedback! Updated ScaleUpdater to use its own dcGetter / rcGetter and reverted ScaleAnnotater's fields back to "private"

0xmichalis · 2016-11-02T20:08:26Z

pkg/cmd/cli/cmd/idle.go

@@ -12,6 +12,7 @@ import (

 	cmdutil "github.com/openshift/origin/pkg/cmd/util"
 	utilerrors "github.com/openshift/origin/pkg/util/errors"


Move these imports with the rest of the Openshift imports

juanvallejo · 2016-11-02T20:35:45Z

@Kargakis

Move these imports with the rest of the Openshift imports

Done!

juanvallejo · 2016-11-03T20:19:15Z

pkg/unidling/controller/controller_test.go

@@ -131,6 +131,40 @@ func prepFakeClient(t *testing.T, nowTime time.Time, scales ...kextapi.Scale) (*
 		return true, nil, errors.NewNotFound(action.GetResource().GroupResource(), obj.Name)
 	})

+	fakeDeployClient.PrependReactor("patch", "deploymentconfigs", func(action ktestingcore.Action) (bool, runtime.Object, error) {


@DirectXMan12 or @Kargakis PTAL

soltysh · 2016-11-04T08:58:08Z

pkg/unidling/controller/controller.go

+		annotator.ChangeAnnotations(typedObj.Spec.Replicas, typedObj.Annotations)
+		typedObj.Spec.Replicas = scale.Spec.Replicas
+
+		newObj, err := json.Marshal(typedObj)


That wasn't still addressed, we should not be using raw json package for any kind of encoding. Sooner or later this will bite us hard. I'd rather fix it before that happens. Here and the next call in the following case, as well.

@soltysh

That wasn't still addressed, we should not be using raw json package for any kind of encoding. Sooner or later this will bite us hard. I'd rather fix it before that happens. Here and the next call in the following case, as well.

Thanks for the feedback, done!

juanvallejo · 2016-11-07T14:39:56Z

conformance flaked on #10988 re[test]

fabianofranz · 2016-11-09T16:19:52Z

flaked on #9076 re[test]

juanvallejo · 2016-11-10T15:07:26Z

conformance flaked on #11775 re[test]

fabianofranz · 2016-11-16T21:01:03Z

@DirectXMan12 @smarterclayton any other comments here?

DirectXMan12

I'd prefer for the scaleUpdater to be moved to utils if it's being used by both the controller and oc -- oc shouldn't have to pull in the entire controller code. Once that's done, this is probably fine.

juanvallejo · 2016-11-17T21:34:00Z

@DirectXMan12

I'd prefer for the scaleUpdater to be moved to utils if it's being used by both the controller and oc

Makes sense, done

juanvallejo · 2016-11-18T15:40:59Z

@DirectXMan12 @fabianofranz I will go ahead and squash if there are no more comments, thanks for the review

openshift-bot · 2016-11-18T15:53:22Z

Evaluated for origin test up to b701818

openshift-bot · 2016-11-18T17:18:17Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11552/) (Base Commit: 32c2948)

fabianofranz · 2016-11-18T18:26:13Z

LGTM [merge]

openshift-bot · 2016-11-18T18:29:19Z

Evaluated for origin merge up to b701818

openshift-bot · 2016-11-18T18:37:24Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11562/) (Base Commit: cdf9282) (Image: devenv-rhel7_5384)

0xmichalis reviewed Oct 28, 2016

View reviewed changes

0xmichalis added this to the 1.4.0 milestone Oct 28, 2016

juanvallejo commented Oct 28, 2016

View reviewed changes

0xmichalis mentioned this pull request Oct 29, 2016

Extended.idling and unidling idling with a single service and DeploymentConfig flake #10988

Closed

DirectXMan12 reviewed Nov 2, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/update-dc-idle-patch-dcs-rcs branch from b88b477 to 05aad3e Compare November 2, 2016 19:39

0xmichalis reviewed Nov 2, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/update-dc-idle-patch-dcs-rcs branch from f797138 to f7aede6 Compare November 2, 2016 20:35

juanvallejo force-pushed the jvallejo/update-dc-idle-patch-dcs-rcs branch from f7aede6 to a3ab2c9 Compare November 3, 2016 14:25

juanvallejo commented Nov 3, 2016

View reviewed changes

soltysh reviewed Nov 4, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/update-dc-idle-patch-dcs-rcs branch from 268eee8 to f8ee34f Compare November 4, 2016 19:56

juanvallejo force-pushed the jvallejo/update-dc-idle-patch-dcs-rcs branch from f8ee34f to df345fa Compare November 8, 2016 20:33

juanvallejo force-pushed the jvallejo/update-dc-idle-patch-dcs-rcs branch 3 times, most recently from 9f9173f to d0b68d6 Compare November 15, 2016 21:52

DirectXMan12 suggested changes Nov 17, 2016

View reviewed changes

patch dcs and rcs instead of update

b701818

juanvallejo force-pushed the jvallejo/update-dc-idle-patch-dcs-rcs branch from aaa2ceb to b701818 Compare November 18, 2016 15:45

openshift-bot merged commit dcaab83 into openshift:master Nov 18, 2016

juanvallejo deleted the jvallejo/update-dc-idle-patch-dcs-rcs branch November 18, 2016 20:05

0xmichalis mentioned this pull request Nov 22, 2016

Update build status to indicate build failure reason #10817

Merged

		@@ -12,6 +12,7 @@ import (

		cmdutil "github.com/openshift/origin/pkg/cmd/util"
		utilerrors "github.com/openshift/origin/pkg/util/errors"

Update oc idle to patch dc's and rc's instead of update #11634

Update oc idle to patch dc's and rc's instead of update #11634

Conversation

juanvallejo commented Oct 27, 2016

juanvallejo commented Oct 27, 2016

0xmichalis Oct 28, 2016

Choose a reason for hiding this comment

liggitt Oct 28, 2016

Choose a reason for hiding this comment

0xmichalis commented Oct 28, 2016

juanvallejo Oct 28, 2016

Choose a reason for hiding this comment

juanvallejo commented Oct 28, 2016 • edited Loading

0xmichalis commented Oct 29, 2016

juanvallejo commented Oct 31, 2016 • edited Loading

fabianofranz commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

liggitt commented Nov 1, 2016 • edited Loading

smarterclayton commented Nov 1, 2016

liggitt commented Nov 1, 2016

juanvallejo commented Nov 1, 2016 • edited Loading

smarterclayton commented Nov 1, 2016

juanvallejo commented Nov 2, 2016

DirectXMan12 left a comment

Choose a reason for hiding this comment

juanvallejo commented Nov 2, 2016

0xmichalis Nov 2, 2016

Choose a reason for hiding this comment

juanvallejo commented Nov 2, 2016

juanvallejo Nov 3, 2016

Choose a reason for hiding this comment

soltysh Nov 4, 2016

Choose a reason for hiding this comment

juanvallejo Nov 4, 2016

Choose a reason for hiding this comment

juanvallejo commented Nov 7, 2016

fabianofranz commented Nov 9, 2016

juanvallejo commented Nov 10, 2016

fabianofranz commented Nov 16, 2016

DirectXMan12 left a comment

Choose a reason for hiding this comment

juanvallejo commented Nov 17, 2016

juanvallejo commented Nov 18, 2016

openshift-bot commented Nov 18, 2016

openshift-bot commented Nov 18, 2016

fabianofranz commented Nov 18, 2016

openshift-bot commented Nov 18, 2016

openshift-bot commented Nov 18, 2016 • edited Loading

Update `oc idle` to patch dc's and rc's instead of update #11634

Update `oc idle` to patch dc's and rc's instead of update #11634

juanvallejo commented Oct 28, 2016 •

edited

Loading

juanvallejo commented Oct 31, 2016 •

edited

Loading

liggitt commented Nov 1, 2016 •

edited

Loading

juanvallejo commented Nov 1, 2016 •

edited

Loading

openshift-bot commented Nov 18, 2016 •

edited

Loading