-
Notifications
You must be signed in to change notification settings - Fork 516
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.
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
feat: update PodGroup label key #521
feat: update PodGroup label key #521
Conversation
Welcome @cmssczy! |
Hi @cmssczy. 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 Once the patch is verified, the new status will be reflected by the 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. |
@denkensk PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denkensk I have 2 more questions,
- Should we handle the case that a Pod has both the new and deprecated key?
- Should we add docs in README noticing the key changes?
pkg/util/podgroup.go
Outdated
@@ -47,9 +47,12 @@ func CreateMergePatch(original, new interface{}) ([]byte, error) { | |||
return patch, nil | |||
} | |||
|
|||
// GetPodGroupLabel get pod group from pod annotations | |||
// GetPodGroupLabel get pod group name from pod annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// GetPodGroupLabel get pod group name from pod annotations | |
// GetPodGroupLabel get pod group name from pod labels |
@@ -99,7 +99,20 @@ func (r *PodGroupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c | |||
log.Error(err, "List pods for group failed") | |||
return ctrl.Result{}, err | |||
} | |||
pods := podList.Items | |||
pods := podList.DeepCopy().Items | |||
// use deprecated label key if cannot list pod by new label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// use deprecated label key if cannot list pod by new label. | |
// also list pods by the deprecated key for compatibility |
pkg/controllers/podgroup_test.go
Outdated
@@ -310,6 +326,20 @@ func makePods(podNames []string, pgName string, phase v1.PodPhase, reference []m | |||
return pds | |||
} | |||
|
|||
func makePodsWithDeprecatedLabel(podNames []string, pgName string, phase v1.PodPhase, reference []metav1.OwnerReference) []*v1.Pod { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about adding an args for makePods
instead of a new function?
/ok-to-test |
If so, the new key first
Yes. It's essential and necessary. |
e239aa1
to
b530534
Compare
Having implement this in function
Added a notice in |
@@ -99,7 +99,20 @@ func (r *PodGroupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c | |||
log.Error(err, "List pods for group failed") | |||
return ctrl.Result{}, err | |||
} | |||
pods := podList.Items | |||
pods := podList.DeepCopy().Items | |||
// also list pods by the deprecated key for compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to actually, given the version is alpha.
Also I'm planning to standardize the group name (to x-k8s.io #525 ) in this release. So it's a breaking change anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denkensk WDYT?
pkg/coscheduling/README.md
Outdated
@@ -15,8 +15,9 @@ This folder holds the coscheduling plugin implementations based on [Coscheduling | |||
## Tutorial | |||
|
|||
### PodGroup | |||
>❗We have changed this label from `pod-group.scheduling.sigs.k8s.io` to `scheduling.sigs.k8s.io/pod-group` in new version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once #526 gets merged, update it to scheduling.x-k8s.io/pod-group
b530534
to
068ba00
Compare
068ba00
to
23e980c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a nit.
@@ -281,7 +281,6 @@ func setUp(ctx context.Context, | |||
objs := []runtime.Object{pg} | |||
if len(podNames) != 0 { | |||
ps := makePods(podNames, pgName, podPhase, podOwnerReference) | |||
// s.AddKnownTypes(clientgoscheme.SchemeGroupVersion, ps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment had been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still there :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I misunderstanding you? This line had been remove in this pr already.
Did you mean remove this line or uncomment this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah my bad... somehow I always perceived this line was newly introduced...
/lgtm Thanks @czybjtu ! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: czybjtu, Huang-Wei 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 |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes ##440
Special notes for your reviewer:
Does this PR introduce a user-facing change?