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

[Magnum][ClusterAutoscaler] Backport control-plane label #5888

Closed
modzilla99 opened this issue Jun 26, 2023 · 19 comments
Closed

[Magnum][ClusterAutoscaler] Backport control-plane label #5888

modzilla99 opened this issue Jun 26, 2023 · 19 comments
Assignees
Labels
area/cluster-autoscaler kind/feature Categorizes issue or PR as related to a new feature.

Comments

@modzilla99
Copy link
Contributor

I fixed #5751 with #5776, but since we already deploy v1.26 and v1.27 with that node role, those versions are also effected. Can this change be cherry-picked for those releases as well, or do we have to build custom images for our customers?

@modzilla99 modzilla99 added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 26, 2023
@Shubham82
Copy link
Contributor

IMO, we can do the cherry-picked #5776 to the previous releases.

@tghartland, what are your thoughts on this?

@Shubham82
Copy link
Contributor

We can do it in the following release of CA: 1.26, 1.25, 1.24
if it seems good then I'm up for it.

@tghartland
Copy link
Contributor

tghartland commented Jun 26, 2023

No reason it can't be backported.
The openstack code still deploys the "master" label which is still caught by the existing condition, and the new check will just be for the "control-plane" label if manually added. Otherwise the new check does nothing.
So please go ahead.

@Shubham82
Copy link
Contributor

Thanks @tghartland for the above info.

@modzilla99
Copy link
Contributor Author

We can do it in the following release of CA: 1.26, 1.25, 1.24 if it seems good then I'm up for it.

That would be amazing! ^^

@Shubham82
Copy link
Contributor

@modzilla99 as per @tghartland above comment, it can't backport.
Am I understanding right?

@tghartland
Copy link
Contributor

It can be backported. Sorry if I worded it confusingly!

@Shubham82
Copy link
Contributor

@tghartland Please don't be sorry and thanks for confirmation.
I will open a PR for cherry-picked it to the previous releases.

@Shubham82
Copy link
Contributor

/assign

@Shubham82
Copy link
Contributor

I checked it, we have to do it for CA v1.27 also because it is not there: https://github.com/kubernetes/autoscaler/blob/cluster-autoscaler-release-1.27/cluster-autoscaler/cloudprovider/magnum/magnum_cloud_provider.go

These changes will reflect in the CA v1.28, so i will backport it in the v1.27 also.

@Shubham82
Copy link
Contributor

Raised the following PR to fix this issue:
For CA 1.27 #5910
For CA 1.26 #5909
For CA 1.25 #5908
For CA 1.24 #5907

@Shubham82
Copy link
Contributor

Shubham82 commented Jun 29, 2023

@tghartland and @modzilla99
Please have a look at these PRs

@modzilla99
Copy link
Contributor Author

lgtm

@tghartland
Copy link
Contributor

All good.

@Shubham82
Copy link
Contributor

Thanks @tghartland and @modzilla99 for taking a look at PRs.

@Shubham82
Copy link
Contributor

Shubham82 commented Jul 3, 2023

Closing this issue as all the related PRs are merged now.
#5910
#5909
#5908
#5907

@Shubham82
Copy link
Contributor

/close

@k8s-ci-robot
Copy link
Contributor

@Shubham82: Closing this issue.

In response to this:

/close

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.

@modzilla99
Copy link
Contributor Author

Thank you @Shubham82 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cluster-autoscaler kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

5 participants