Add PriorityClass setting to registry pods for CatalogSource via annotations #2304
Conversation
|
@dinhxuanvu: This pull request references Bugzilla bug 1954869, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
openshift-ci
bot
added
bugzilla/severity-high
|
@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: jianzhangbjz. Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
timflannagan
changed the title
openshift-ci
bot
removed
bugzilla/severity-high
|
@dinhxuanvu: No Bugzilla bug is referenced in the title of this pull request. In response to this:
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. |
|
(I removed the bug from the PR title so automation doesn't pick this up once it merges) |
|
I think this implementation looks reasonable to me - I'd like to remove any downstream-specific handling here and just treat propagating priorityclass specifications into the catalogsource pods like Joe had eluded to in another comment. |
dinhxuanvu
force-pushed
the
catsrc-prioclass
branch
from
69d6e20 to
139edb6
Compare
|
Resolved and updated. |
The registry pods may need to have necessary priorityclass settings. OLM will set the priorityclass setting for registry pods by using the priorityclass annotations in the default catalogsources. Signed-off-by: Vu Dinh <[email protected]>
dinhxuanvu
force-pushed
the
catsrc-prioclass
branch
from
139edb6 to
c9ccfd9
Compare
dinhxuanvu
changed the title
| priorityclass: "system-cluster-critical", | ||
| }, | ||
| { | ||
| testName: "Grpc/WithInvalidPriorityClassAnnotation", |
There was a problem hiding this comment.
(not blocking) Technically not invalid but we won't propagate any empty key value to the priorityclass default.
| require.Len(t, outPods.Items, 1) | ||
| outPod := outPods.Items[0] | ||
| require.Equal(t, tt.priorityclass, outPod.Spec.PriorityClassName) | ||
| require.Equal(t, pod.GetLabels()[PodHashLabelKey], outPod.GetLabels()[PodHashLabelKey]) |
There was a problem hiding this comment.
Not super concerned with blindingly indexing like this, but it might produce test flakes down the line.
|
/lgtm |
| @@ -171,15 +171,16 @@ func (c *GrpcRegistryReconciler) currentUpdatePods(source grpcCatalogSourceDecor | |||
| return pods | |||
| } | |||
|
|
|||
| func (c *GrpcRegistryReconciler) currentPodsWithCorrectImage(source grpcCatalogSourceDecorator) []*corev1.Pod { | |||
| func (c *GrpcRegistryReconciler) currentPodsWithCorrectImageAndSpec(source grpcCatalogSourceDecorator, saName string) []*corev1.Pod { | |||
There was a problem hiding this comment.
Not technically what I had in mind but works for now 😆
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dinhxuanvu, kevinrizza 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 |
openshift-ci
bot
added
the
approved
The registry pods may need to have necessary priorityclass settings.
OLM will set the priorityclass setting for registry pods by using
the priorityclass annotations in the catalogsources.
Signed-off-by: Vu Dinh [email protected]
Description of the change:
Motivation for the change:
Reviewer Checklist
/doc