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.

Sign up for GitHub

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

Jump to bottom

Update Vendor v1.25.0-alpha.2 , bump Go to 1.18.1 and additional support for StorageInfoListers #5013

Merged
merged 4 commits into from
Aug 24, 2022
Merged

Update Vendor v1.25.0-alpha.2 , bump Go to 1.18.1 and additional support for StorageInfoListers #5013

merged 4 commits into from
Aug 24, 2022

Conversation

jayantjain93
Copy link
Contributor

Which component this PR applies to?

cluster-autoscaler

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4904

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 7, 2022
@jayantjain93
Copy link
Contributor Author

/assign @x13n

@jayantjain93 jayantjain93 force-pushed the vendor-1.25.0 branch 3 times, most recently from aab1c73 to eb31809 Compare July 7, 2022 09:33
@jayantjain93 jayantjain93 changed the title Update Vendor v1.25.0-alpha.0 and additional support for StorageInfoListers Update Vendor v1.25.0-alpha.0 , bump Go to 1.18.1 and additional support for StorageInfoListers Jul 7, 2022
@jayantjain93 jayantjain93 changed the title Update Vendor v1.25.0-alpha.0 , bump Go to 1.18.1 and additional support for StorageInfoListers Update Vendor v1.25.0-alpha.2 , bump Go to 1.18.1 and additional support for StorageInfoListers Jul 7, 2022
@jayantjain93 jayantjain93 force-pushed the vendor-1.25.0 branch 2 times, most recently from 84dcd74 to 0d5db30 Compare July 7, 2022 11:16
@jayantjain93 jayantjain93 force-pushed the vendor-1.25.0 branch 2 times, most recently from fc0e519 to 31b7836 Compare July 7, 2022 11:38
x13n
x13n reviewed Jul 7, 2022
View reviewed changes
Copy link
Member

@x13n x13n left a comment

Choose a reason for hiding this comment

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

Please add some tests for the new cluster snapshot logic.

cluster-autoscaler/simulator/basic_cluster_snapshot.go Outdated Show resolved Hide resolved
cluster-autoscaler/simulator/basic_cluster_snapshot.go Outdated Show resolved Hide resolved
cluster-autoscaler/simulator/delta_cluster_snapshot.go Outdated Show resolved Hide resolved
cluster-autoscaler/simulator/delta_cluster_snapshot.go Outdated Show resolved Hide resolved
@jayantjain93
Copy link
Contributor Author

Also added tests for the change

@jayantjain93 jayantjain93 force-pushed the vendor-1.25.0 branch 3 times, most recently from 316af4c to 2e46552 Compare July 14, 2022 09:33
@jayantjain93 jayantjain93 force-pushed the vendor-1.25.0 branch 2 times, most recently from 7c7f107 to bf91b29 Compare July 14, 2022 10:59
@x13n
Copy link
Member

x13n commented Jul 14, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2022
@mwielgus
Copy link
Contributor

@jayantjain93 Where are we with this PR? It is LGTM'ed and the only missing thing seems to be the rebase.

@MaciekPytel
Copy link
Contributor

@mwielgus That's on me, I wanted to review this one and haven't manage to do it for a long time.

@MaciekPytel
Copy link
Contributor

/hold
/approve

@jayantjain93 Sorry it took me so long to get to this.
Overall looks good, but I left a few minor comments. Leaving a hold to avoid accidentally merging before you have a chance to look at those. Please feel free to "/hold cancel" without blocking on my review.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jayantjain93, 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2022
@jayantjain93
Cluster Autoscaler Update-vendor to v1.25.0-alpha.2
29f2a6a
@jayantjain93
Fix CA breaking after vendor update to v1.25.0-alpha.0 by supporting …
5c31c33 
…StorageInfoLister
@jayantjain93
add support for isPVC check in basic_cluster_snapshot.go and delta_cl…
4fdf24a 
…uster_snapshot.go

- added a comment in delegating_shared_lister.go
@jayantjain93
update golang version to 1.18.1
dde246d 
- changing now since apimachinary is using type which is available in 1.18
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2022
@jayantjain93
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2022
@x13n
Copy link
Member

x13n commented Aug 24, 2022 •
edited
Loading

I think delta snapshot would be more efficient if it stored actual deltas (possibly negative) of PVC usage counters. Now we'll iterate over all nodeinfos when trying to schedule a pod using PVC on a forked snapshot. This PR is a huge and 1.25-blocking change though, so let's merge this, we can always optimize further if there's a scalability regression.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2022
@k8s-ci-robot k8s-ci-robot merged commit a503c84 into kubernetes:master Aug 24, 2022
navinjoy pushed a commit to navinjoy/autoscaler that referenced this pull request Oct 26, 2022
@k8s-ci-robot
Merge pull request kubernetes#5013 from jayantjain93/vendor-1.25.0
5b47c24 
Update Vendor v1.25.0-alpha.2 , bump Go to 1.18.1 and additional support for StorageInfoListers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaciekPytel MaciekPytel MaciekPytel left review comments

@x13n x13n x13n left review comments

@aleksandra-malinowska aleksandra-malinowska Awaiting requested review from aleksandra-malinowska

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@x13n x13n

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend ClusterSnapshot implementations to include StorageInfos lister
6 participants
@jayantjain93 @x13n @mwielgus @MaciekPytel @k8s-ci-robot @jbartosik