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

Sync with upstream v1.29.4 #327

Merged
merged 1,063 commits into from
Oct 23, 2024
Merged

Sync with upstream v1.29.4 #327

merged 1,063 commits into from
Oct 23, 2024

Conversation

rishabh-11
Copy link

@rishabh-11 rishabh-11 commented Sep 27, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #326

Special notes for your reviewer:
IT pass for AWS

Release note:

Synced with upstream v1.29.4

ROunofF and others added 30 commits October 10, 2023 19:29
…lity

Convert scale-down checks to drainability rules
Update ec2_instance_types with make generate
- HasNodeGroupStartedScaleUp checks wheter a scale up request exists
  without checking any upcoming nodes.
Add HasNodeGroupStartedScaleUp to cluster state registry.
jbartosik et al are transitioning off of workload autoscalers (incl vpa
and addon-resizer). kwiesmueller is on the new team and has agreed to
take on reviewer/approver responsibilities.
…-fix

Add information about provisioning-class-name annotation.
Fix multiple run of informers created in fetcher.go
Signed-off-by: Jonathan Raymond <[email protected]>
Add mechanism to override drainability status
…scale from zero method via environment variable

The architecture label in the build generic labels method of the cluster API (CAPI) provider is now populated using the GetDefaultScaleFromZeroArchitecture().Name() method.

The method allows CAPI users deploying the cluster-autoscaler to define the default architecture to be used by the cluster-autoscaler for scale up from zero via the env var CAPI_SCALE_ZERO_DEFAULT_ARCH. Amd64 is kept as a fallback for historical reasons.

The introduced changes will not take into account the case of nodes heterogeneous in architecture. The labels generation to infer properties like the cpu architecture from the node groups' features should be considered as a CAPI provider specific implementation.
@rishabh-11 rishabh-11 requested review from unmarshall and a team as code owners September 27, 2024 08:52
@gardener-robot gardener-robot added the needs/review Needs review label Sep 27, 2024
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 15 committers have signed the CLA.

✅ rishabh-11
❌ k8s-ci-robot
❌ MaciekPytel
❌ kdw174
❌ Silvest89
❌ gjtempleton
❌ vadasambar
❌ Shubham82
❌ BigDarkClown
❌ damikag
❌ maksim-paskal
❌ apricote
❌ wenxuan0923
❌ x13n
❌ kmsarabu
You have signed the CLA already but the status is still pending? Let us recheck it.

@gardener-robot gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Sep 27, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 27, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 27, 2024
Copy link

@aaronfern aaronfern left a comment

Choose a reason for hiding this comment

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

Looks good in general

In addition to the two changes asked, I have one question, and one more change

  1. Do we not update SYNC-CHANGES with patch versions?
  2. Can you please update the docstring of machine_cloud_provider.GetOptions to the new docstring here

Comment on lines 611 to 618
// FORK-CHANGE: logging version of g/autoscaler as well
gardenerversion, err := os.ReadFile("VERSION")
if err != nil {
klog.Warningf("Error reading gardener autoscaler version, err: %s", err)
} else {
klog.V(1).Infof("Gardener Cluster Autoscaler %s", gardenerversion)
}

Choose a reason for hiding this comment

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

This is removed by error ig. Can you please re add this?

Copy link
Author

Choose a reason for hiding this comment

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

Did not remove it by mistake. There is no Version file anymore. So we can either add a version file and then reintroduce this or we can get rid of this.

Copy link
Author

Choose a reason for hiding this comment

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

I checked and it is working fine. It uses the VERSION file at the root of the repo, not the version dir. I have added this piece of code back

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Oct 2, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 7, 2024
@rishabh-11
Copy link
Author

Do we not update SYNC-CHANGES with patch versions?

It is not needed IMO as the sync changes are for minor and not patch versions.

Can you please update the docstring of machine_cloud_provider.GetOptions to the new docstring here

The docstring is part of 1.30 and is also not present upstream. I think it is fine for us to keep it like this.

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 21, 2024
@aaronfern
Copy link

I think it is fine for us to keep it like this

Okay, fair enough for me then

Copy link

@aaronfern aaronfern left a comment

Choose a reason for hiding this comment

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

Thanks for the changes
/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Oct 22, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 22, 2024
@rishabh-11 rishabh-11 merged commit e16e855 into gardener:rel-v1.29 Oct 23, 2024
9 of 10 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Oct 23, 2024
@rishabh-11 rishabh-11 deleted the rel-v1.29 branch October 23, 2024 06:44
@rishabh-11 rishabh-11 restored the rel-v1.29 branch October 23, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.