Add extraLabels to both subcontroller and vbsc pods#735
Add extraLabels to both subcontroller and vbsc pods#735jdoupe wants to merge 4 commits intoplanetscale:mainfrom
Conversation
|
@jdoupe can you please create a quick issue that describes what problem you're trying to solve? And link it to the PR here. Thanks! |
jdoupe
force-pushed
the
add-extra-labels
branch
2 times, most recently
from
c7ba815 to
ce2285c
Compare
|
Administrative note: (and you're probably already aware) the |
|
Bump @frouioui |
frouioui
left a comment
frouioui
left a comment
There was a problem hiding this comment.
Hi! The operator manifest and the vitess repository will need to be modified given this PR updates the CRDs. A guide on how to do this is available here:
jdoupe
force-pushed
the
add-extra-labels
branch
2 times, most recently
from
55a0f83 to
a20ed6a
Compare
|
@frouioui - I wanted to mention that in addition to the steps outlined in #629 (comment) that it appears that the WATCH_NAMESPACE variable has also been altered (by you ;P ) in the time since, and likely needs to be "watched" on any change, as well as the securityContext that @mattlord added in the recent past. Don't know if either or both of those are appropriate to add to the source (deploy/deployment.yaml). I'll also note that kustomize(?) looks like it may have changed a bunch of whitespace things, as well as some other mundane bits. All in all, this looks relatively straightforward, so do let me know if something else needs a tweak. Thanks! |
Signed-off-by: Jeremy Doupe <jeremy@doupe.com>
Signed-off-by: Jeremy Doupe <jeremy@doupe.com>
Signed-off-by: Jeremy Doupe <jeremy@doupe.com>
Signed-off-by: Jeremy Doupe <jeremy@doupe.com>
jdoupe
force-pushed
the
add-extra-labels
branch
from
a20ed6a to
757fb6e
Compare
mhamza15
left a comment
mhamza15
left a comment
There was a problem hiding this comment.
Left a few comments, LGTM otherwise. I also wrote up some regression tests that should reproduce the cases the two comments are referring to: mhamza15@f685780
| spec.ExtraLabels = make(map[string]string, len(extraLabels)) | ||
| for k, v := range extraLabels { | ||
| spec.ExtraLabels[k] = v | ||
| } |
| spec.ExtraLabels = make(map[string]string, len(vt.Spec.Backup.ExtraLabels)) | ||
| for k, v := range vt.Spec.Backup.ExtraLabels { | ||
| spec.ExtraLabels[k] = v | ||
| } |
| } | ||
| // Merge user-provided extra labels from ClusterBackupSpec. | ||
| if vbs.Spec.ExtraLabels != nil { | ||
| update.Labels(&labels, vbs.Spec.ExtraLabels) |
There was a problem hiding this comment.
This adds the custom labels to the label set that ReconcileObject uses to decide whether the existing pod belongs to this controller. If a subcontroller pod already exists and the user adds extraLabels later, the pod will not have those new labels yet, so reconciliation fails with NameCollision before it gets a chance to update the pod. I believe we should keep labels to only the stable operator labels here, and add vbs.Spec.ExtraLabels directly to the pod in New and UpdateInPlace instead.
| } | ||
|
|
||
| // Apply ExtraLabels to the pod, similar to regular vttablet pods. | ||
| update.Labels(&pod.Labels, tabletSpec.ExtraLabels) |
There was a problem hiding this comment.
Scheduled backup pods still will not get the custom tablet pool labels because the job only copies the generated pod spec, not the generated pod labels: https://github.com/jdoupe/vitess-operator/blob/757fb6e82d4eb3bbfbd5db7eeeda54faebe57154/pkg/controller/vitessbackupschedule/vitessbackupschedule_controller.go#L684-L686.
Those labels are lost before Kubernetes creates the real backup pod. I think we need to also add the custom tablet pool labels to the job template.
4 participants
Fixes #736
By adding the following:
vitessbackupschedules.spec.extraLabels
vitessbackupstorages.spec.extraLabels
vitessclusters.spec.backup.extraLabels
For vtbackup(-init), the code has just been modified to pass along the existing extraLabels on the tabletPool.
@mattlord / @frouioui