Emit an event if overwriting PodTemplate affinity #2859
Conversation
tekton-robot
added
the
needs-ok-to-test
|
Hi @adshmh. Thanks for your PR. I'm waiting for a tektoncd 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. |
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/cc @jlpettersson |
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/ok-to-test |
tekton-robot
added
ok-to-test
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
1 similar comment
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
/retest |
1 similar comment
|
/retest |
pkg/pod/pod.go
Outdated
|
|
||
| // WillOverwritePodSetAffinity returns a bool indicating whether the | ||
| // affinity for pods will be overwritten with affinity assistant. | ||
| func WillOverwritePodSetAffinity(taskRun *v1beta1.TaskRun) bool { |
There was a problem hiding this comment.
Is it possible to declare this in a package so that it can be private?
There was a problem hiding this comment.
Thank you for the review. I think this is related to the comment below, i.e. if the WillOverwritePodSetAffinity is moved to the reconciler package, then it can be made private.
pkg/pod/status.go
Outdated
|
|
||
| // WarningOverwritingPodAffinity indicates that the affinity specified by | ||
| // the pod template will be overwritten with affinity assistant | ||
| WarningOverwritingPodAffinity = "PodAffinityOverwrite" |
There was a problem hiding this comment.
Also this, is it possible to declare this in a context so that it can be private? I think it is only used in a single place - so I feel it should be possible.
There was a problem hiding this comment.
Thank you for the review. Fixed.
pkg/reconciler/taskrun/taskrun.go
Outdated
| @@ -360,11 +360,15 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, | |||
| tr.Spec.Workspaces = taskRunWorkspaces | |||
| } | |||
|
|
|||
| willOverwriteAffinity := podconvert.WillOverwritePodSetAffinity(tr) | |||
| pod, err = c.createPod(ctx, tr, rtr) | |||
There was a problem hiding this comment.
is it possible to move in the willOverwritePodSetAffinity to inside c.createPod() - not sure it is possible, but it would be closer to the place where the affinity is set.
There was a problem hiding this comment.
Thank you for the review. Yes, it can be moved inside createPod, but then we need to pass the recorder to createPod as well, and it will need to start emitting events. I implemented it this way to avoid changing createPod signature, and to keep all the event emitting in the reconciler package.
If moving this to the pod package is preferred, please let me know and I'll update the PR.
There was a problem hiding this comment.
It looks like you wouldn't have to change the signature of createPod, you can look up the recorder from the context:
// in c.createPod
recorder := controller.GetEventRecorder(ctx)
if recorder != nil && willOverwritePodAffinity(tr) {
recorder.Eventf(...)
}There was a problem hiding this comment.
Thank you for the reviews. Moved willOverwritePodSetAffinity to createPod, no changes were made to the signature of createPod.
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
adshmh
force-pushed
the
2678-emit-event-when-set-affinity-property-is-overwritten
branch
from
8a765be to
7ee173b
Compare
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
1 similar comment
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/kind feature |
tekton-robot
added
the
kind/feature
pkg/pod/pod.go
Outdated
| if taskRun.Spec.PodTemplate != nil { | ||
| podTemplate = *taskRun.Spec.PodTemplate | ||
| } | ||
| return taskRun.Annotations[workspace.AnnotationAffinityAssistantName] != "" && podTemplate.Affinity != nil |
There was a problem hiding this comment.
Is it possible that taskRun.Annotations is ever a nil map? I think this might panic if so.
There was a problem hiding this comment.
Thank you for the review. I did not introduce a nil check here as it is only reading the map so there won't be a panic if the map is nil. Please let me know if I am missing something here regarding the nil check.
pkg/reconciler/taskrun/taskrun.go
Outdated
| @@ -360,11 +360,15 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, | |||
| tr.Spec.Workspaces = taskRunWorkspaces | |||
| } | |||
|
|
|||
| willOverwriteAffinity := podconvert.WillOverwritePodSetAffinity(tr) | |||
| pod, err = c.createPod(ctx, tr, rtr) | |||
There was a problem hiding this comment.
It looks like you wouldn't have to change the signature of createPod, you can look up the recorder from the context:
// in c.createPod
recorder := controller.GetEventRecorder(ctx)
if recorder != nil && willOverwritePodAffinity(tr) {
recorder.Eventf(...)
}
tekton-robot
added
the
needs-rebase
adshmh
force-pushed
the
2678-emit-event-when-set-affinity-property-is-overwritten
branch
from
7ee173b to
bc7e8b2
Compare
The taskrun controller now emits a warning event if the affinity specified by pod template will be overwritten with affinity assistant. Closes tektoncd#2678
adshmh
force-pushed
the
2678-emit-event-when-set-affinity-property-is-overwritten
branch
from
bc7e8b2 to
380c8c4
Compare
tekton-robot
removed
the
needs-rebase
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlpettersson, sbwsg 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 |
tekton-robot
added
the
approved
|
@tektoncd/core-maintainers does anyone have bandwidth to review and lgtm if all looks ok? |
| pod, err = c.KubeClientSet.CoreV1().Pods(tr.Namespace).Create(pod) | ||
| if err == nil && willOverwritePodSetAffinity(tr) { | ||
| if recorder := controller.GetEventRecorder(ctx); recorder != nil { | ||
| recorder.Eventf(tr, corev1.EventTypeWarning, "PodAffinityOverwrite", "Pod template affinity is overwritten by affinity assistant for pod %q", pod.Name) |
There was a problem hiding this comment.
I would have introduced const here for PodAffinityOverwrite but going through the reviews from @jlpettersson on the same 🙃 , looks good for now, can be converted to const if needed in future
|
/lgtm thanks @adshmh @jlpettersson @sbwsg 🙏 |
Changes
The taskrun controller now emits a warning event if the affinity
specified by pod template will be overwritten with affinity assistant.
Closes #2678
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes