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

[CA-1.18] #2934 cherry-pick: Fixes 2932: let the capi version to be discovered #3105

Conversation

xmudrii
Copy link
Member

@xmudrii xmudrii commented Apr 30, 2020

This PR proposes to cherry-pick #2934 to the cluster-autoscaler-release-1.18 branch.

With Cluster Autoscaler v1.18.0, it's possible to use the Cluster-API provider. However, there is a bug that makes the provider unusable for some users.

The provider supports all Cluster-API groups and versions, but in the original PR, the group and version are hardcoded. Users that are not using v1alpha2 have two options, which are not optimal:

  • Wait for Cluster Autoscaler v1.19.0, which will come up with K8s 1.19.0. The problem is that it's somewhere in August, so that could be a bit long to wait
  • Build the image which includes the fix manually, which is not the greatest approach.

The PR (#2934) that fixes this could be looked at as a bugfix and it doesn't introduce any behavior changes (the default group is still cluster.x-k8s.io and version will be auto-discovered). I believe that it would make sense to cherry-pick it to the release-1.18 branch, so the bugfix is included in the next patch release. That would make the provider useful for all users, regardless of the group and version they are using.

Original PR description:

…API version.
This change adds detection for an environment variable to specify the group for the clusterapi resources. If the environment
variable `CAPI_GROUP` is specified, then it will
be used instead of the default.
This also decouples the API group from the version and let the latter to be discovered dynamically.

cc @elmiko

…API version.

This change adds detection for an environment variable to specify the group for the clusterapi resources. If the environment
variable `CAPI_GROUP` is specified, then it will
be used instead of the default.
This also decouples the API group from the version and let the latter to be discovered dynamically.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 30, 2020
@k8s-ci-robot k8s-ci-robot requested review from detiber and ncdc April 30, 2020 17:17
@elmiko
Copy link
Contributor

elmiko commented Apr 30, 2020

i think this is a good commit to cherry-pick, i'd like to get @enxebre 's thoughts also.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2020
@detiber
Copy link
Member

detiber commented Apr 30, 2020

+1 from me

@enxebre
Copy link
Member

enxebre commented May 4, 2020

I'm all good with this. But It should be validated by some autoscaler core owner for the release branching management and back ports policy.

@MaciekPytel
Copy link
Contributor

MaciekPytel commented May 4, 2020

/approve
Based on PR description this seems like a reasonable fix to cherry-pick - CA not working seems like a good justification. I'll leave lgtm-ing to someone who is actually familiar with CAPI code.

Regarding a broader backport policy - we are fine with cherry-picking provider specific fixes and there is plenty of precedent for it. We're also more relaxed than kubernetes/kubernetes when it comes to both cherry-picking features (if they're critical enough and changes are reasonably small) and how many versions we support (it's usually ~2 more than k/k).
In case of smaller changes that are limited to specific provider we defer to judgement of owners of that provider - so far no owner has abused this power, so we haven't had the need for a more formal process. Example: #3055 is a small cherry-pick limited to a single provider.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko, MaciekPytel

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:

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

@enxebre
Copy link
Member

enxebre commented May 4, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit 9cd7d97 into kubernetes:cluster-autoscaler-release-1.18 May 4, 2020
@xmudrii xmudrii deleted the cherry-pick-2934 branch May 4, 2020 14:48
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants