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: reuse SchedulingPolicy struct for the scheduling policy definition of InstanceTemplate #7290

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

refactor: reuse SchedulingPolicy struct for the scheduling policy definition of InstanceTemplate #7290

wants to merge 1 commit into from

Conversation

starnop
Copy link
Contributor

@starnop starnop commented May 9, 2024 •
edited

Reuse SchedulingPolicy struct for the scheduling policy definition of InstanceTemplate

Fixed #7291

@CLAassistant
Copy link

CLAassistant commented May 9, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the size/XXL Denotes a PR that changes 1000+ lines. label May 9, 2024
@apecloud-bot apecloud-bot added the approved PR Approved Test label May 9, 2024
@JashBook JashBook removed the approved PR Approved Test label May 9, 2024
@apecloud-bot apecloud-bot requested a review from realzyy May 9, 2024 10:07
@free6om free6om requested a review from leon-inf May 9, 2024 10:11
free6om
free6om reviewed May 9, 2024
View reviewed changes
apis/apps/v1alpha1/cluster_types.go Outdated
Comment on lines 445 to 469
// Deprecated since v0.10, replaced by the `schedulingPolicy` field.
//
// +kubebuilder:deprecatedversion:warning="This field has been deprecated since 0.10.0"
// +kubebuilder:pruning:PreserveUnknownFields
// +optional
NodeName *string `json:"nodeName,omitempty"`

// Defines NodeSelector to override.
//
// Deprecated since v0.10, replaced by the `schedulingPolicy` field.
//
// +kubebuilder:deprecatedversion:warning="This field has been deprecated since 0.10.0"
// +kubebuilder:pruning:PreserveUnknownFields
// +optional
NodeSelector map[string]string `json:"nodeSelector,omitempty"`

// Tolerations specifies a list of tolerations to be applied to the Pod, allowing it to tolerate node taints.
// This field can be used to add new tolerations or override existing ones.
//
// Deprecated since v0.10, replaced by the `schedulingPolicy` field.
//
// +kubebuilder:deprecatedversion:warning="This field has been deprecated since 0.10.0"
// +kubebuilder:pruning:PreserveUnknownFields
// +optional
Tolerations []corev1.Toleration `json:"tolerations,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

should remove them directly as they are introduced in the 0.9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

Makefile Outdated
@@ -22,6 +22,8 @@ TEST_TYPE ?= wesql
GIT_COMMIT = $(shell git rev-list -1 HEAD)
GIT_VERSION = $(shell git describe --always --abbrev=0 --tag)
GENERATED_CLIENT_PKG = "pkg/client"
GENERATED_DEEP_COPY_FILE = "zz_generated.deepcopy.go"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest not to modify the unrelated files of the issue that this PR tries to resolve.

@free6om
Copy link
Contributor

free6om commented May 9, 2024

missing the controller part.

@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from 2eef999 to c156c53 Compare May 10, 2024 02:30
@apecloud-bot apecloud-bot added the approved PR Approved Test label May 10, 2024
@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from c156c53 to fc7860f Compare May 10, 2024 03:10
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 10, 2024
@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from fc7860f to 5f15feb Compare May 10, 2024 06:24
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 10, 2024
@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from 5f15feb to acc038a Compare May 10, 2024 06:33
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 10, 2024
@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from acc038a to bfd640e Compare May 10, 2024 08:38
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 10, 2024
@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from bfd640e to 462f2e1 Compare May 10, 2024 08:53
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 10, 2024
free6om
free6om reviewed May 10, 2024
View reviewed changes
pkg/controller/instanceset/instance_util.go Outdated
@@ -586,12 +586,12 @@ func buildInstanceTemplateExt(template workloads.InstanceTemplate, templateExt *
replicas = *template.Replicas
}
templateExt.Replicas = replicas
if template.NodeName != nil {
templateExt.Spec.NodeName = *template.NodeName
if template.SchedulingPolicy.NodeName != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

do nil check of SchedulingPolicy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

The SchedulingPolicy is not a pointer 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.

Indeed SchedulingPolicy should be optional. The pointer type is more appropriate. I will modify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@free6om PTAL again, THX

@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from 462f2e1 to b5f3908 Compare May 11, 2024 02:40
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 11, 2024
@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from b5f3908 to 3c4fe93 Compare May 11, 2024 02:43
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 11, 2024
@starnop starnop force-pushed the reuse-SchedulingPolicy-for-InstanceTemplate branch from 3c4fe93 to 7307c4e Compare May 11, 2024 02:56
@apecloud-bot apecloud-bot added approved PR Approved Test and removed approved PR Approved Test labels May 11, 2024
@starnop
refactor: reuse SchedulingPolicy struct for the scheduling policy def…
7307c4e 
…inition of InstanceTemplate

Signed-off-by: starnop <starnop@163.com>
weicao
weicao reviewed May 13, 2024
View reviewed changes
pkg/controller/instanceset/instance_util.go
@@ -651,6 +664,105 @@ func mergeCPUNMemory(s, d *corev1.ResourceList) {
}
}

func mergeAffinity(affinity1Ptr, affinity2Ptr **corev1.Affinity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we have two mergeAffinity now.

Should be addressed in the future pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weicao 👍 I didn't find a similar method before, I'll see if I can reuse it directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@weicao 👍 I didn't find a similar method before, I'll see if I can reuse it directly.

in pkg/controller/scheduling/scheduling_utils.go

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

@weicao weicao weicao left review comments

@free6om free6om free6om approved these changes

@nayutah nayutah nayutah approved these changes

@ldming ldming Awaiting requested review from ldming ldming is a code owner

@heng4fun heng4fun Awaiting requested review from heng4fun heng4fun is a code owner

@wangyelei wangyelei Awaiting requested review from wangyelei wangyelei is a code owner

@Y-Rookie Y-Rookie Awaiting requested review from Y-Rookie Y-Rookie is a code owner

@realzyy realzyy Awaiting requested review from realzyy

@leon-inf leon-inf Awaiting requested review from leon-inf

Assignees
No one assigned
Labels
approved PR Approved Test area/user-interaction size/XXL Denotes a PR that changes 1000+ lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Features] reuse SchedulingPolicy struct for the scheduling policy defiinition of InstanceTemplate
7 participants
@starnop @CLAassistant @free6om @weicao @nayutah @JashBook @apecloud-bot