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

Refactor Code to Use PollUntilContextTimeout Replacing Deprecated PolImmediate #122708

Closed
wants to merge 1 commit into from
Closed

Refactor Code to Use PollUntilContextTimeout Replacing Deprecated PolImmediate #122708

wants to merge 1 commit into from

Conversation

daniel-hutao
Copy link
Contributor

@daniel-hutao daniel-hutao commented Jan 11, 2024 •
edited

Looking forward to your feedback to continue with the rest of the updates.

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Refactor code to use PollUntilContextTimeout replacing deprecated PolImmediate.

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

Completely updating all the deprecated PollImmediate to PollUntilContextTimeout is a big task. It took me quite a while, and I have changed about half of them.

However, I have noticed that the number of files modified in the current update is already quite substantial. If I were to continue with this update task, it might require modifying dozens more files, which could pose some difficulties for the review process.
Therefore, I have decided to submit a PR now. The remaining update work can be completed in a separate PR later on.

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. 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. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @daniel-hutao. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kube-proxy area/kubeadm area/kubelet area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 11, 2024
neolit123
neolit123 reviewed Jan 11, 2024
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

please remove the cmd/kubeadm changes from this PR as we have them pending in another PR
#122529
thanks.

@neolit123
Copy link
Member

/remove-sig cluster-lifecycle
/remove-area kubeadm

@k8s-ci-robot k8s-ci-robot removed sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. area/kubeadm labels Jan 11, 2024
@daniel-hutao
Copy link
Contributor Author

please remove the cmd/kubeadm changes from this PR as we have them pending in another PR #122529 thanks.

@neolit123 Ok, I'll remove it in the next commit.

@bart0sh bart0sh added this to Triage in SIG Node PR Triage Jan 12, 2024
@k8s-ci-robot k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Jan 15, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: daniel-hutao
Once this PR has been reviewed and has the lgtm label, please assign dims for approval. For more information see the Kubernetes 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

@kannon92
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 15, 2024
@kannon92
Copy link
Contributor

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 15, 2024
@kannon92
Copy link
Contributor

Thanks for this PR. IMO this is a pretty large PR as is and its going to be difficult to find reviewers/approvers for this.

You could make it easier on yourself by maybe separating these PRs by sigs.

@kannon92 kannon92 moved this from Triage to Needs Reviewer in SIG Node PR Triage Jan 15, 2024
@k8s-ci-robot
Copy link
Contributor

@daniel-hutao: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-linter-hints 37d795e link false /test pull-kubernetes-linter-hints
pull-kubernetes-verify-lint 37d795e link false /test pull-kubernetes-verify-lint
pull-kubernetes-unit 37d795e link true /test pull-kubernetes-unit
pull-kubernetes-verify 37d795e link true /test pull-kubernetes-verify
pull-kubernetes-e2e-capz-windows-master 37d795e link false /test pull-kubernetes-e2e-capz-windows-master

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@kannon92
Copy link
Contributor

@daniel-hutao these failures are related to your PR. Could you look into fixing these?

I brought your PR up in slack in #k8s-code-organization as this has come up before.

https://kubernetes.slack.com/archives/CHGFYJVAN/p1705335454294119

I'd like to maybe have test code separated from actual prod code at a minimum.

/cc @pohly

kannon92
kannon92 reviewed Jan 15, 2024
View reviewed changes
Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

As stated earlier, I think the PR is a valid addition but I think we should aim to break this PR up.

Ideally I'd like the test code separated into its own PR and the production code into a different PR (and ideally that should be small and separated by sigs so we can get more detailed reviews).

pkg/kubelet/util/manager/watch_based_manager.go
@@ -17,6 +17,7 @@ limitations under the License.
package manager
Copy link
Contributor

Choose a reason for hiding this comment

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

For Kubelet, this is a non test related change. It would be nice to be a bit more careful on these.

pkg/controller/nodelifecycle/node_lifecycle_controller.go
}
return false, nil
}); err != nil {
if err = wait.PollUntilContextTimeout(context.Background(), retrySleepTime, retrySleepTime*scheduler.NodeHealthUpdateRetry, true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Another non test related change.

pkg/kubelet/cm/devicemanager/plugin/v1beta1/stub.go
conn.Close()
return true, nil
})
err = wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 10*time.Second, true,
Copy link
Contributor

Choose a reason for hiding this comment

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

non test related change that seems to be part of some api changes. Be careful on these also.

@daniel-hutao
Copy link
Contributor Author

@kannon92 Thank for your review and advice. I agree with your idea and prepare to separate this PR to some small PRs. I'll open an issue to trigger these PRs.

@daniel-hutao daniel-hutao mentioned this pull request Jan 16, 2024
Open
@daniel-hutao
Copy link
Contributor Author

/close

I prefer to close this PR first. The separate PRs are triggering in #122800.

SIG Node CI/Test Board automation moved this from Triage to Done Jan 26, 2024
@k8s-ci-robot
Copy link
Contributor

@daniel-hutao: Closed this PR.

In response to this:

/close

I prefer to close this PR first. The separate PRs are triggering in #122800.

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.

SIG Node PR Triage automation moved this from Needs Reviewer to Done Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kannon92 kannon92 kannon92 left review comments

@atiratree atiratree Awaiting requested review from atiratree

@bowei bowei Awaiting requested review from bowei

@neolit123 neolit123 Awaiting requested review from neolit123

@pohly pohly Awaiting requested review from pohly

Assignees
No one assigned
Labels
area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kube-proxy area/kubelet area/test 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Apps
Archived in project
SIG Auth
Archived in project
SIG CLI
Archived in project
SIG Node CI/Test Board
Done
SIG Node PR Triage
Archived in project
[sig-windows] PR tracking
Status: Done
SIG Node PR Triage
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@daniel-hutao @k8s-ci-robot @neolit123 @kannon92