Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UPSTREAM: <carry>: Short-circuit HPA oapi/v1.DC #18380

Conversation

DirectXMan12
Copy link
Contributor

The legacy oapi v1 group-version very much confuses anything not
designed to explicitly work with it. Since we now don't do any custom
HPA setup, we need to teach the scale client and the HPA what to do
with the oapi version of DC, since it won't even show up in its
discovery process.

Related to #18377

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 31, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Jan 31, 2018
@DirectXMan12
Copy link
Contributor Author

A.K.A. HPA Carries II: Electric Boogaloo

cc @deads2k as we talked about earlier. I'm not quite sure I like a couple of details, but this is more-or-less what the HPA patches would look like.

cc @sjenning FYI

@DirectXMan12
Copy link
Contributor Author

@deads2k will get you an oadm migrate command a bit later/tomorrow.

@DirectXMan12 DirectXMan12 force-pushed the bug/hpa-dc-oapi-sadness-electric-boogaloo branch from 8bcd888 to 0c79992 Compare January 31, 2018 22:35
@@ -360,6 +360,20 @@ func (a *HorizontalController) reconcileAutoscaler(hpav1Shared *autoscalingv1.Ho
}

mappings, err := a.mapper.RESTMappings(targetGK)
// TODO(directxman12): this is a dirty, dirty hack because legacy oapi isn't part of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a weird niggle. To make rebases easier, please move the logic to a file called patch_dc.go and then after the error simply do mappings = hackOutOapiDeploymentConfig(mappings)

That way when it conflicts (and there are always conflicts), we have a single line to fix up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ack, will do.

// TODO(directxman12): this is a dirty, dirty hack because oapi just appears in discovery as "/v1", like
// the kube core API. We can remove it if/when we get rid of the legacy oapi group entirely. It makes me
// cry a bit inside, but such is life.
glog.Infof("DEBUG: %#v == %#v: %v", gvr, dcGVR, gvr == dcGVR)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hack is ok, but not the info level debug message. Similar comment about making this a one liner with a patch_dc.go file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, that must've slipped in, sorry.

The legacy oapi v1 group-version very much confuses anything not
designed to explicitly work with it.  Since we now don't do any custom
HPA setup, we need to teach the scale client and the HPA what to do
with the oapi version of DC, since it won't even show up in its
discovery process.
@DirectXMan12 DirectXMan12 force-pushed the bug/hpa-dc-oapi-sadness-electric-boogaloo branch from 0c79992 to eea8aa3 Compare February 1, 2018 16:39
@aveshagarwal
Copy link
Contributor

@deads2k @liggitt could you provide lgtm/approval?

@deads2k
Copy link
Contributor

deads2k commented Feb 7, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, DirectXMan12

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2018
@sjenning
Copy link
Contributor

sjenning commented Feb 9, 2018

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 3c18cd3 into openshift:master Feb 9, 2018
@DirectXMan12 DirectXMan12 deleted the bug/hpa-dc-oapi-sadness-electric-boogaloo branch February 9, 2018 19:35
openshift-merge-robot added a commit that referenced this pull request Mar 6, 2018
Automatic merge from submit-queue (batch tested with PRs 18666, 18810, 18430, 18517, 18653).

Add migrate command for legacy HPAs

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants