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

cluster-autoscaler: standardize context usage #7664

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cluster-autoscaler: standardize context usage #7664

wants to merge 1 commit into from
+868 −867

Conversation

jackfrancis
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR standardizes CA-specific context usage of the "k8s.io/autoscaler/cluster-autoscaler/context" library.

Specifically:

  • Standardize the import reference to ca_context to disambiguate from the standard golang "context" package.
    • e.g., ca_context "k8s.io/autoscaler/cluster-autoscaler/context"
  • Standardize variable references to type AutoscalingContext in functions, etc, as autoscalingContext (again, to disambiguate from the standard context foo)
  • Undo any non-idiomatic usage of the standard golang "context" package
    • e.g., we always import as "context", and never with an alias
    • e.g., we always use ctx as the variable reference to a context.Context type

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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. area/cluster-autoscaler labels Jan 6, 2025
@jackfrancis
Copy link
Contributor Author

/assign @MaciekPytel @towca

While reviewing some other PRs this stuff was driving me nuts. I think this PR, while pretty huge, improves the way we use context(s) significantly and isn't too horrible to review.

@jackfrancis
Copy link
Contributor Author

/test ls

@k8s-ci-robot
Copy link
Contributor

@jackfrancis: The specified target(s) for /test were not found.
The following commands are available to trigger optional jobs:

/test pull-cluster-autoscaler-e2e-azure-master

/test pull-kubernetes-e2e-autoscaling-vpa-full

In response to this:

/test ls

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-sigs/prow repository.

@jackfrancis
Copy link
Contributor Author

/test pull-cluster-autoscaler-e2e-azure-master

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jackfrancis
Once this PR has been reviewed and has the lgtm label, please ask for approval from maciekpytel. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2025
@jackfrancis
Copy link
Contributor Author

/test pull-cluster-autoscaler-e2e-azure-master

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2025
@jackfrancis jackfrancis force-pushed the context-standardize branch from 45876c7 to a802d00 Compare March 21, 2025 21:38
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2025
@jackfrancis
Copy link
Contributor Author

/retest

@jackfrancis jackfrancis force-pushed the context-standardize branch from a802d00 to e7dea53 Compare March 25, 2025 14:43
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2025
@jackfrancis jackfrancis force-pushed the context-standardize branch from e7dea53 to de117ea Compare March 27, 2025 16:47
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2025
@jackfrancis
Copy link
Contributor Author

/retest

@elmiko
Copy link
Contributor

elmiko commented Mar 27, 2025

/cc

@k8s-ci-robot k8s-ci-robot requested a review from elmiko March 27, 2025 18:50
elmiko
elmiko reviewed Mar 27, 2025
View reviewed changes
Copy link
Contributor

@elmiko elmiko 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 cleanup @jackfrancis , it looks good to me but i did not go searching for missed references. i do have a couple questions about adding some comments.

cluster-autoscaler/core/scaledown/actuation/actuator.go
@@ -219,7 +219,7 @@ func (a *Actuator) taintNodesSync(NodeGroupViews []*budgets.NodeGroupView) (time
err error
}, len(nodesToTaint))
taintedNodes := make(chan *apiv1.Node, len(nodesToTaint))
workqueue.ParallelizeUntil(default_context.Background(), maxConcurrentNodesTainting, len(nodesToTaint), func(piece int) {
workqueue.ParallelizeUntil(context.Background(), maxConcurrentNodesTainting, len(nodesToTaint), func(piece int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if we should add a comment here to explain why the default context is being used instead of autoscalingcontext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

autoscalingContext doesn't fulfill the context.Context interface (maybe it's unfortunate that we named it AutoscalingContext, but that ship has sailed. Ref:

cluster-autoscaler/main.go
@@ -270,10 +270,10 @@ func run(healthCheck *metrics.HealthCheck, debuggingSnapshotter debuggingsnapsho
autoscalingOpts := flags.AutoscalingOptions()

metrics.RegisterAll(autoscalingOpts.EmitPerNodeGroupMetrics)
context, cancel := ctx.WithCancel(ctx.Background())
ctx, cancel := context.WithCancel(context.Background())
Copy link
Contributor

Choose a reason for hiding this comment

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

should we comment about why we are using the default context here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto my earlier response

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elmiko elmiko elmiko left review comments

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

@vadasambar vadasambar Awaiting requested review from vadasambar

Assignees

@MaciekPytel MaciekPytel

@towca towca

Labels
area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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.
5 participants
@jackfrancis @k8s-ci-robot @elmiko @MaciekPytel @towca