volcano icon indicating copy to clipboard operation
volcano copied to clipboard

Published 20 hours ago • verified publisher icon volcano-sh

add ut and refactor for pkg/scheduler/plugins/util/k8s package

Open googs1025 opened this issue 1 year ago • 13 comments

What type of PR is this?

unit test

What this PR does / why we need it:

  • refactor nodelock
  • add some unit test for pkg/scheduler/plugins/util/k8s/snapshot.go
  • fix IsPVCUsedByPods method

Which issue(s) this PR fixes:

See more detail https://github.com/volcano-sh/volcano/issues/3053#issuecomment-2042837878

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.:

None

googs1025 avatar Apr 14 '24 13:04 googs1025

@lowang-bh /PTAL thank a lot!

googs1025 avatar Apr 17 '24 01:04 googs1025

@lowang-bh image image Additionally, I noticed some code duplication in this section. I intend to submit a refactoring pull request to improve it here. like those two https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L79 https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L112

WDYT

googs1025 avatar Apr 17 '24 14:04 googs1025

Additionally, I noticed some code duplication in this section. I intend to submit a refactoring pull request to improve it here. like those two https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L79 https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L112

WDYT

Welcom！

lowang-bh avatar Apr 18 '24 03:04 lowang-bh

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign lowang-bh You can assign the PR to them by writing /assign @lowang-bh in a comment when ready.

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

volcano-sh-bot avatar Apr 18 '24 14:04 volcano-sh-bot

In addition to adding unit tests, I also tried to refactor nodelock. Because it involves changes to the original logic, please reviewers to help me check it carefully. /PTAL @lowang-bh @william-wang @shinytang6

googs1025 avatar Apr 18 '24 14:04 googs1025

/assign @william-wang @shinytang6 Thanks for the review

googs1025 avatar Apr 18 '24 14:04 googs1025

done

googs1025 avatar Apr 21 '24 13:04 googs1025

/PTAL @william-wang @shinytang6 thanks a lot

googs1025 avatar Apr 28 '24 02:04 googs1025

New changes are detected. LGTM label has been removed.

volcano-sh-bot avatar May 07 '24 04:05 volcano-sh-bot

@Monokaix @lowang-bh Please help review this pr thanks!

googs1025 avatar May 21 '24 14:05 googs1025

/lgtm

lowang-bh avatar May 21 '24 14:05 lowang-bh

@william-wang /PTAL thanks!

googs1025 avatar Jun 11 '24 13:06 googs1025

Are there any suggested changes or can we move on to the next step? @william-wang @Monokaix

googs1025 avatar Jun 24 '24 02:06 googs1025

/lgtm

Monokaix avatar Jun 24 '24 06:06 Monokaix

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

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

volcano-sh-bot avatar Jul 25 '24 08:07 volcano-sh-bot