Add filtering logic for a `PriorityClass` that the name is prefixed with `system-` #108

196Ikuchil · 2022-02-13T09:47:06Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The priorityclass, system-cluster-critical and system-node-critical, are created automatically, but it can't delete and is unable to modify some fields.
The name of PriorityClass that is prefixed with system-, is reserved by the system.
So, we add filtering logic to import/export functions.

Which issue(s) this PR fixes:

Fixes #98

Special notes for your reviewer:

https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#how-to-use-priority-and-preemption

/label tide/merge-method-squash
/assign @sanposhiho

k8s-ci-robot · 2022-02-13T09:47:14Z

Hi @196Ikuchil. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

lianghao208 · 2022-02-14T02:41:52Z

export/export.go

+		// The name of a PriorityClass object cannot be prefixed with `system-`.
+		// It is reserved by the system and we cannot recreate it. No need to export.
+		for _, i := range pcs.Items {
+			if !strings.HasPrefix(i.GetObjectMeta().GetName(), "system-") {


Is it better to filter out the exact match of system-cluster-critical and system-node-critical? In case users create system-xxx PriorityClass by themselves.

Users cannot create system-xxx PriorityClasses. So, it looks good.

The name of a PriorityClass object must be a valid DNS subdomain name, and it cannot be prefixed with system-.
https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass

sanposhiho

@196Ikuchil
Could you create a function (named like filterPriorityClass) to filter PriorityClasses and change the logic to use that?

sanposhiho · 2022-02-14T03:31:10Z

/ok-to-test

196Ikuchil · 2022-02-14T10:51:47Z

Fixed it👌

sanposhiho · 2022-02-14T12:28:56Z

export/export.go

@@ -501,3 +515,8 @@ func (s *Service) applyPods(ctx context.Context, r *ResourcesForImport, eg *util
 	}
 	return nil
 }
+
+// The name of PriorityClass that is prefixed with `system-`, is reserved by the system.
+func filterPriorityClass(name string) bool {


Okay. Rename it to isSystemPriorityClass. (IMO, I feel a little strange iffilterXX returns bool.)

export/export.go

export/export_test.go

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

196Ikuchil · 2022-02-14T13:59:06Z

@sanposhiho
Thanks for reviewing. I fixed some wording as per your suggestions.

sanposhiho

Only wording of comments. otherwise looks good.

sanposhiho · 2022-02-14T13:59:39Z

export/export.go

+		// The name of a PriorityClass object cannot be prefixed with `system-`.
+		// It is reserved by the system and we cannot recreate it. No need to export.


Remove these lines (the doc of isSystemPriorityClass will describe it.)

sanposhiho · 2022-02-14T14:00:04Z

export/export.go

+		// The name of PriorityClass that is prefixed with `system-`, is reserved by the system and we cannot recreate it.
+		// Therefore, filter it.


Remove these lines as well

sanposhiho · 2022-02-14T14:02:06Z

export/export.go

@@ -501,3 +515,10 @@ func (s *Service) applyPods(ctx context.Context, r *ResourcesForImport, eg *util
 	}
 	return nil
 }
+
+// isSystemPriorityClass returns whether the given name of PriorityClass is prefixed with `system-` or not.
+// The prefix `system-` is reserved by Kubernetes and cannot be used in the name of PriorityClass.


Sorry, my wording is not good 😓

Suggested change

// The prefix `system-` is reserved by Kubernetes and cannot be used in the name of PriorityClass.

// The prefix `system-` is reserved by Kubernetes and users cannot create PriorityClass that name is prefixed with `system-`.

@sanposhiho How is this?

The `system-` prefix is reserved by Kubernetes, and users cannot create a PriorityClass with such a name.

looks good 👍

sanposhiho · 2022-02-14T14:03:26Z

export/export.go

+
+// isSystemPriorityClass returns whether the given name of PriorityClass is prefixed with `system-` or not.
+// The prefix `system-` is reserved by Kubernetes and cannot be used in the name of PriorityClass.
+// See: https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass


Suggested change

// See: https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass

// See: https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass

//

// So, we need to exclude these PriorityClasses when import/export PriorityClasses.

196Ikuchil · 2022-02-14T14:21:41Z

@sanposhiho
Thanks! Fixed it, again✋

sanposhiho

Thanks!

/lgtm
/approve

k8s-ci-robot · 2022-02-14T14:53:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 196Ikuchil, sanposhiho

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:

~~OWNERS~~ [sanposhiho]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

196Ikuchil added 3 commits February 13, 2022 18:43

fix:filter the high privileged priorityclass

7fbb5d6

test:add test for listPcs()

d03065f

test:add test for applyPcs()

103f675

k8s-ci-robot assigned sanposhiho Feb 13, 2022

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 13, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 13, 2022

k8s-ci-robot requested review from adtac and alculquicondor February 13, 2022 09:47

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 13, 2022

lianghao208 reviewed Feb 14, 2022

View reviewed changes

sanposhiho requested changes Feb 14, 2022

View reviewed changes

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 Feb 14, 2022

feat:filterPriorityClass

87a68b9

sanposhiho reviewed Feb 14, 2022

View reviewed changes

196Ikuchil and others added 4 commits February 14, 2022 22:47

Update export/export_test.go

0f0882f

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

Update export/export.go

7a1b342

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

Update export/export_test.go

686b61c

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

fix:change function name to isSystemPriorityClass

7aee3b7

sanposhiho reviewed Feb 14, 2022

View reviewed changes

docs:fix some comments

fc38406

sanposhiho approved these changes Feb 14, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2022

k8s-ci-robot merged commit 7e37705 into kubernetes-sigs:master Feb 14, 2022

196Ikuchil mentioned this pull request Mar 31, 2022

REQUEST: New membership for 196Ikuchil kubernetes/org#3346

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filtering logic for a `PriorityClass` that the name is prefixed with `system-` #108

Add filtering logic for a `PriorityClass` that the name is prefixed with `system-` #108

196Ikuchil commented Feb 13, 2022 •

edited

Loading

k8s-ci-robot commented Feb 13, 2022

lianghao208 Feb 14, 2022

sanposhiho Feb 14, 2022

sanposhiho left a comment •

edited

Loading

sanposhiho commented Feb 14, 2022

196Ikuchil commented Feb 14, 2022

sanposhiho Feb 14, 2022

196Ikuchil commented Feb 14, 2022

sanposhiho left a comment

sanposhiho Feb 14, 2022

sanposhiho Feb 14, 2022 •

edited

Loading

sanposhiho Feb 14, 2022

196Ikuchil Feb 14, 2022

sanposhiho Feb 14, 2022

sanposhiho Feb 14, 2022

196Ikuchil commented Feb 14, 2022

sanposhiho left a comment

k8s-ci-robot commented Feb 14, 2022

		// The name of a PriorityClass object cannot be prefixed with `system-`.
		// It is reserved by the system and we cannot recreate it. No need to export.

		// The name of PriorityClass that is prefixed with `system-`, is reserved by the system and we cannot recreate it.
		// Therefore, filter it.

	// The prefix `system-` is reserved by Kubernetes and cannot be used in the name of PriorityClass.
	// The prefix `system-` is reserved by Kubernetes and users cannot create PriorityClass that name is prefixed with `system-`.

Add filtering logic for a PriorityClass that the name is prefixed with system- #108

Add filtering logic for a PriorityClass that the name is prefixed with system- #108

Conversation

196Ikuchil commented Feb 13, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

k8s-ci-robot commented Feb 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho left a comment • edited Loading

Choose a reason for hiding this comment

sanposhiho commented Feb 14, 2022

196Ikuchil commented Feb 14, 2022

Choose a reason for hiding this comment

196Ikuchil commented Feb 14, 2022

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

196Ikuchil commented Feb 14, 2022

sanposhiho left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 14, 2022

Add filtering logic for a `PriorityClass` that the name is prefixed with `system-` #108

Add filtering logic for a `PriorityClass` that the name is prefixed with `system-` #108

196Ikuchil commented Feb 13, 2022 •

edited

Loading

sanposhiho left a comment •

edited

Loading

sanposhiho Feb 14, 2022 •

edited

Loading