Add migrate command for legacy HPAs #18517

DirectXMan12 · 2018-02-07T22:05:34Z

There are current broken HPAs floating around that either use the legacy
oapi DeploymentConfig defintion (v1.DeploymentConfig) or the incorrect
API group, due to the webconsole (all scalables, but with the group as
extensions/v1beta1). This introduces a migrate command that corrects
the ScaleTargetRef of those HPAs to have correct API groups that are
usable by generic scale clients.

Related to #18377, #18380, openshift/origin-web-console#2776

cc @deads2k @liggitt @spadgett

DirectXMan12 · 2018-02-07T22:06:34Z

I'd like to also add the capability to automatically migrate arbitrary ScaleTargetRefs to the equivalent preferred version to make deprecation of scalable APIs easier, but I figured this is a good start.

deads2k · 2018-02-08T13:08:00Z

/cc @enj our resident migrate expert

openshift-ci-robot · 2018-02-08T13:08:03Z

@deads2k: GitHub didn't allow me to request PR reviews from the following users: our, resident, migrate, expert.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @enj our resident migrate expert

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aveshagarwal · 2018-02-09T13:58:48Z

@enj any feedback?

aveshagarwal · 2018-02-09T13:59:12Z

@derekwaynecarr any feedback?

sjenning · 2018-02-12T02:19:01Z

In verify-imports

2018/02/07 22:19:29 Inspecting imports under github.com/openshift/origin/pkg/oc...
2018/02/07 22:19:32 -- validating imports for 111 packages in the tree
2018/02/07 22:19:32 -- found forbidden imports for github.com/openshift/origin/pkg/oc/admin/migrate/legacyhpa:
2018/02/07 22:19:32 	vendor/github.com/stretchr/testify/assert

Need to update generated completions and docs as well.

soltysh · 2018-02-13T13:42:32Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+	Kind       string
+}
+
+func (vk versionKind) GroupVersionKind() (schema.GroupVersionKind, error) {


You can replace that with just schema.FromAPIVersionAndKind

don't need this any more, but the reason that I didn't is because I wanted the error, not to have it fail silently

soltysh · 2018-02-13T13:46:33Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+		{"v1", "DeploymentConfig"}: {"apps.openshift.io/v1", "DeploymentConfig"},
+
+		// webconsole shenaniganry
+		{"extensions/v1beta1", "DeploymentConfig"}:      {"apps.openshift.io/v1", "DeploymentConfig"},


Say what? That should never happen.

The web console was trying to target the scale subresource in the extensions/v1beta1 group (I think based on an earlier HPA API version), but the subresource was getting dropped. So it unfortunately got set as extensions/v1beta1.DeploymentConfig.

soltysh · 2018-02-13T13:46:51Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+		{"extensions/v1beta1", "DeploymentConfig"}:      {"apps.openshift.io/v1", "DeploymentConfig"},
+		{"extensions/v1beta1", "Deployment"}:            {"apps/v1", "Deployment"},
+		{"extensions/v1beta1", "ReplicaSet"}:            {"apps/v1", "ReplicaSet"},
+		{"extensions/v1beta1", "ReplicationController"}: {"v1", "ReplicationController"},


soltysh · 2018-02-13T13:49:01Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+func (o *MigrateLegacyHPAOptions) checkAndTransform(hpaRaw runtime.Object) (migrate.Reporter, error) {
+	var hpa *autoscaling.HorizontalPodAutoscaler
+	var wasHPA bool
+	if hpa, wasHPA = hpaRaw.(*autoscaling.HorizontalPodAutoscaler); !wasHPA {


This version seems more readable and saves precious keystrokes:

hpa, wasHPA := hpaRaw.(*autoscaling.HorizontalPodAutoscaler) if !wasHPA { ...

soltysh · 2018-02-13T13:49:49Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+func (o *MigrateLegacyHPAOptions) save(info *resource.Info, reporter migrate.Reporter) error {
+	var hpa *autoscaling.HorizontalPodAutoscaler
+	var wasHPA bool
+	if hpa, wasHPA = info.Object.(*autoscaling.HorizontalPodAutoscaler); !wasHPA {


Same remark wrt precious keystrokes 😉

enj · 2018-02-13T15:14:45Z

@enj any feedback?

Been on PTO, still catching up. Will look shortly.

enj

Lots of little things need fixing. Agree with all the changes @soltysh suggested.

enj · 2018-02-14T22:13:00Z

pkg/oc/admin/migrate/legacyhpa/hpa_test.go

+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"


Use == or deep equal.

hmm? What's wrong with testify?

enj · 2018-02-14T22:13:15Z

pkg/oc/admin/migrate/legacyhpa/hpa_test.go

+
+	"github.com/stretchr/testify/assert"
+
+	autoscaling "k8s.io/kubernetes/pkg/apis/autoscaling"


Drop the unused name.

enj · 2018-02-14T22:19:57Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+	  %[1]s --initial=v1.DeploymentConfig --final=apps.openshift.io/v1.DeploymentConfig --confirm`)
+)
+
+func prettyPrintMigrations(versionKinds map[versionKind]versionKind) string {


This needs to do sorting so the output is stable.

enj · 2018-02-14T22:20:30Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+	return strings.Join(lines, "\n")
+}
+
+type versionKind struct {


Uh this is just the TypeMeta struct, so just use that instead.

enj · 2018-02-14T22:22:03Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+}
+
+func (o *MigrateLegacyHPAOptions) Bind(c *cobra.Command) {
+	o.ResourceOptions.Bind(c)


Just call this in NewCmdMigrateLegacyHPA and drop the Bind.

yep. Used to have more options here -- simplified and forgot to strip it off

enj · 2018-02-14T22:24:22Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+			ErrOut: errout,
+
+			AllNamespaces: true,
+			Include:       []string{"horizontalpodautoscalers.autoscaling"},


You need to add validation that prevents this include from being changed via the options that are binded to this command.

enj · 2018-02-14T22:26:56Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+	return migrate.ReporterBool(false), nil
+}
+
+func (o *MigrateLegacyHPAOptions) latestVersionKind(current versionKind) (versionKind, error) {


This should just return a TypeMeta since it cannot error.

enj · 2018-02-14T22:32:18Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+		return fmt.Errorf("unrecognized object %#v", info.Object)
+	}
+
+	updatedHPA, err := o.hpaClient.HorizontalPodAutoscalers(hpa.Namespace).Update(hpa)


This should just be:

_, err := o.hpaClient.HorizontalPodAutoscalers(hpa.Namespace).Update(hpa) return migrate.DefaultRetriable(info, err)

There is no reason to refresh the info or check the error.

DirectXMan12 · 2018-02-27T17:48:22Z

@enj comments addressed, except for the testify one. For future reference, what's wrong with just using testify?

enj · 2018-02-27T18:16:00Z

@enj comments addressed, except for the testify one. For future reference, what's wrong with just using testify?

That is a kube only depedency. Using it here turns in into an OpenShift dependency. We are actively trying to remove dependencies (and not add new ones), and we do not use that anywhere else in OS (hence the angry message from verify imports). Please remove it.

enj

@DirectXMan12 can you add a simple CLI test that migrate one "old" HPA to a "new" HPA. Doesn't need to be fancy, just enough to fail if the command doesn't work at all.

enj · 2018-02-27T18:19:47Z

pkg/oc/admin/migrate/legacyhpa/hpa.go

+	migrate.ResourceOptions
+}
+
+func (o *MigrateLegacyHPAOptions) Bind(c *cobra.Command) {


I think you forgot to delete this.

sjenning · 2018-03-01T21:20:10Z

Test failed with

Error from server (AlreadyExists): error when creating "test/testdata/hpa/legacy-and-normal-hpa.yaml": horizontalpodautoscalers.autoscaling "legacy-hpa" already exists

enj · 2018-03-02T02:09:20Z

test/cmd/migrate.sh

+# create a legacy and a normal HPA
+os::cmd::expect_success 'oc create -f test/testdata/hpa/legacy-and-normal-hpa.yaml'
+# verify dry run
+os::cmd::expect_success_and_text 'oc adm migrate legacy-hpa' 'migrated=1'


I assume you also need to run once with --confirm?

yep, whoops

enj · 2018-03-02T02:09:49Z

test/testdata/hpa/legacy-and-normal-hpa.yaml

+apiVersion: autoscaling/v1
+kind: HorizontalPodAutoscaler
+metadata:
+  name: legacy-hpa


Wrong name?

There are current broken HPAs floating around that either use the legacy oapi DeploymentConfig defintion (`v1.DeploymentConfig`) or the incorrect API group, due to the webconsole (all scalables, but with the group as `extensions/v1beta1`). This introduces a migrate command that corrects the ScaleTargetRef of those HPAs to have correct API groups that are usable by generic scale clients.

DirectXMan12 · 2018-03-02T17:34:48Z

/retest

enj · 2018-03-02T18:12:56Z

/approve

Deferring LGTM to anyone who knows if we are supposed to be merging stuff or waiting until 3.10 branch.

DirectXMan12 · 2018-03-02T20:03:09Z

/retest

enj · 2018-03-04T17:14:56Z

/approve
/retest

enj · 2018-03-04T17:16:10Z

/refresh

soltysh

/lgtm
/approve

openshift-ci-robot · 2018-03-05T12:19:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, enj, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~contrib/completions/OWNERS~~ [enj,soltysh]
~~docs/man/OWNERS~~ [enj,soltysh]
~~pkg/oc/OWNERS~~ [enj,soltysh]
~~test/cmd/OWNERS~~ [enj,soltysh]
~~test/testdata/OWNERS~~ [enj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2018-03-05T12:19:29Z

Deferring LGTM to anyone who knows if we are supposed to be merging stuff or waiting until 3.10 branch.

Master is open for 3.10
/retest

liggitt · 2018-03-05T12:36:06Z

Deferring LGTM to anyone who knows if we are supposed to be merging stuff or waiting until 3.10 branch.

Pretty sure this is required for 3.9 as well. Cherrypick?

deads2k · 2018-03-05T15:16:54Z

/cherrypick release-3.9

openshift-cherrypick-robot · 2018-03-05T15:16:55Z

@deads2k: once the present PR merges, I will cherry-pick it on top of release-3.9 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sjenning · 2018-03-05T20:31:35Z

/retest

openshift-merge-robot · 2018-03-06T03:34:29Z

Automatic merge from submit-queue (batch tested with PRs 18666, 18810, 18430, 18517, 18653).

openshift-cherrypick-robot · 2018-03-06T03:35:36Z

@deads2k: new pull request created: #18854

In response to this:

/cherrypick release-3.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…-18517-to-release-3.9 Automatic merge from submit-queue. [release-3.9] Add migrate command for legacy HPAs This is an automated cherry-pick of #18517 /assign deads2k

liggitt · 2018-03-27T05:29:31Z

does this let us drop https://github.com/openshift/kubernetes/blob/release-1.9.1/pkg/controller/podautoscaler/patch_dc.go in 3.10?

deads2k · 2018-03-27T12:31:59Z

does this let us drop https://github.com/openshift/kubernetes/blob/release-1.9.1/pkg/controller/podautoscaler/patch_dc.go in 3.10?

No. You could still create them from an old manifest unless we decide to break backward compatibility or carry a different patch on the kube-apiserver to rewrite the data instead.

openshift-ci-robot requested review from juanvallejo and soltysh February 7, 2018 22:05

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 7, 2018

openshift-ci-robot requested a review from enj February 8, 2018 13:08

sjenning added the kind/bug Categorizes issue or PR as related to a bug. label Feb 9, 2018

sjenning assigned derekwaynecarr Feb 9, 2018

soltysh suggested changes Feb 13, 2018

View reviewed changes

enj suggested changes Feb 14, 2018

View reviewed changes

DirectXMan12 force-pushed the feature/migrate-oapi-hpa branch from 54bb08a to 4e9b329 Compare February 27, 2018 17:44

enj reviewed Feb 27, 2018

View reviewed changes

DirectXMan12 force-pushed the feature/migrate-oapi-hpa branch 2 times, most recently from 733aa58 to 1df1296 Compare February 28, 2018 22:06

enj reviewed Mar 2, 2018

View reviewed changes

DirectXMan12 force-pushed the feature/migrate-oapi-hpa branch from 1df1296 to e6b35ca Compare March 2, 2018 16:26

DirectXMan12 force-pushed the feature/migrate-oapi-hpa branch from e6b35ca to da442d5 Compare March 2, 2018 16:44

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2018

openshift-ci-robot assigned soltysh Mar 5, 2018

soltysh approved these changes Mar 5, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2018

openshift-merge-robot merged commit f2c2c9c into openshift:master Mar 6, 2018

openshift-cherrypick-robot mentioned this pull request Mar 6, 2018

[release-3.9] Add migrate command for legacy HPAs #18854

Merged

DirectXMan12 deleted the feature/migrate-oapi-hpa branch March 6, 2018 18:53

liggitt mentioned this pull request Jun 8, 2018

unable to determine resource for scale target reference #19940

Closed

jwforres mentioned this pull request Jul 3, 2018

HPA showing unkown cpu utilization #19941

Closed


		"github.com/stretchr/testify/assert"

		autoscaling "k8s.io/kubernetes/pkg/apis/autoscaling"

Add migrate command for legacy HPAs #18517

Add migrate command for legacy HPAs #18517

Conversation

DirectXMan12 commented Feb 7, 2018

DirectXMan12 commented Feb 7, 2018

deads2k commented Feb 8, 2018

openshift-ci-robot commented Feb 8, 2018

aveshagarwal commented Feb 9, 2018

aveshagarwal commented Feb 9, 2018

sjenning commented Feb 12, 2018

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

enj commented Feb 13, 2018

enj left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Feb 27, 2018

enj commented Feb 27, 2018 • edited Loading

enj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Mar 2, 2018

enj commented Mar 2, 2018

DirectXMan12 commented Mar 2, 2018

enj commented Mar 4, 2018

enj commented Mar 4, 2018

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 5, 2018

soltysh commented Mar 5, 2018

liggitt commented Mar 5, 2018

deads2k commented Mar 5, 2018

openshift-cherrypick-robot commented Mar 5, 2018

sjenning commented Mar 5, 2018

openshift-merge-robot commented Mar 6, 2018

openshift-cherrypick-robot commented Mar 6, 2018

liggitt commented Mar 27, 2018

deads2k commented Mar 27, 2018

enj commented Feb 27, 2018 •

edited

Loading