feat(results): expose Tekton Results Watcher config via TektonConfig#3709
feat(results): expose Tekton Results Watcher config via TektonConfig#3709adchauha wants to merge 1 commit into
Conversation
Add spec.result.watcher so watcher flags can be configured through TektonConfig without manually patching tekton-results-watcher Deployment args.
tekton-robot
added
the
release-note
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
tekton-robot
added
the
size/XL
|
/kind feature |
tekton-robot
added
the
kind/feature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3709 +/- ##
==========================================
- Coverage 25.51% 25.47% -0.05%
==========================================
Files 448 449 +1
Lines 23309 23372 +63
==========================================
+ Hits 5948 5954 +6
- Misses 16675 16726 +51
- Partials 686 692 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
enarha
left a comment
enarha
left a comment
There was a problem hiding this comment.
The idea of storing the watcher configuration under .result.watcher is good.
The watcher has 21 configuration options (https://github.com/tektoncd/results/blob/main/cmd/watcher/main.go#L64-L85) , but here I see exposed only 15. Was that done on purpose and what's the criteria to decide which flags are exposed through the TektonConfig and which not?
I believe that as implemented, the new flags will be set on new deployment, but if user changes the TektonConfig later, the configuration on the cluster won't be updated. Can you please test that scenario and ensure it works? Updating the TektonConfig should always trigger reconciliation and new configuration should take an effect immediately.
| labelKeys := getSortedKeys(flags) | ||
| for _, key := range labelKeys { | ||
| labelKey := fmt.Sprintf("%s.data.%s", deploymentName, key) | ||
| podLabels[labelKey] = fmt.Sprintf("%v", flags[key]) |
There was a problem hiding this comment.
This writes every watcher setting into a Kubernetes label, but a few of these settings (summary_labels, summary_annotations, label_selector) are free-text strings that can contain characters Kubernetes doesn't allow in label values — like /, ,, or =.
For example, your own docs example sets summary_labels: tekton.dev/pipeline. If that value gets written as a label, Kubernetes will reject it with a validation error (label values can only contain letters, numbers, -, _, .), and the whole deployment will fail to apply.
Suggestion: don't put the raw value into the label. Instead, use something like a hash of the value (e.g. fmt.Sprintf("%x", md5.Sum([]byte(value)))) as the label value. The label here is only used to force the pod to restart when a setting changes — it doesn't need to be human-readable, so a hash works and is always a valid label value.
| assert.Equal(t, deployment.Spec.Template.Spec.PriorityClassName, "system-cluster-critical") | ||
| } | ||
|
|
||
| func TestUpdateWatcherFlagsInDeployment(t *testing.T) { |
There was a problem hiding this comment.
This only tests the happy path, but more importantly tests should be testing the edge cases, invalid values, repeated values, errors handling, etc.
|
|
||
| argUpdated := false | ||
| for argIndex, existingArg := range container.Args { | ||
| if strings.HasPrefix(existingArg, expectedArg) { |
There was a problem hiding this comment.
This code assumes each existing flag in the container's args list looks like a single string, e.g. "-check_owner=true". But if you look at the base watcher deployment manifest, some flags are written as two separate list entries instead, e.g.:
args:
- -api_addr
- $(TEKTON_RESULTS_API_SERVICE)
If one of the flags you're now configuring is ever defined that way, this code would overwrite just the -api_addr entry with -api_addr=value, but leave the old value ($(TEKTON_RESULTS_API_SERVICE)) sitting in the list as a stray, unrelated argument. That stray value would break how the watcher parses its command-line flags (it would think that's the next positional argument and stop reading further flags).
It happens not to bite you today because none of the 15 flags you're adding currently exist in the base manifest's args list. But it's worth handling now so it doesn't silently break later (e.g. if this helperis reused for api_addr/auth_mode, or if the watcher's manifest changes format upstream).
Suggestion: when you find a matching flag, check if the next array element is a plain value (not startingwith -) and remove it too before inserting the combined -flag=value form. Or simpler: always remove the old flag (and its value, if separate) first, then append the new -flag=value — that avoids the two different “shapes” colliding.
| errs = errs.Also(trs.Performance.Validate(fmt.Sprintf("%s.performance", path))) | ||
|
|
||
| // validate watcher properties | ||
| errs = errs.Also(trs.Watcher.Validate(fmt.Sprintf("%s.watcher", path))) |
There was a problem hiding this comment.
This validation only runs when someone creates/edits a TektonResult object directly. But the PR description says the main way users will set this is through TektonConfig (spec.result.watcher) — and TektonConfig's own validation (pkg/apis/operator/v1alpha1/tektonconfig_validation.go, around line 143) never calls this Watcher.Validate(). It only validates tc.Spec.Result.Options.
That means if a user sets an invalid label_selector or a negative duration in TektonConfig, Kubernetes will accept it right away (no error shown to the user). The mistake would only be caught later, indirectly, when the operator tries to create the underlying TektonResult object — and even then it might not be obvious to the user why it failed.
Suggestion: add a line in tektonconfig_validation.go next to the existing tc.Spec.Result.Options.validate(...) call, e.g.:
errs = errs.Also(tc.Spec.Result.Watcher.Validate("spec.result.watcher")) so users get immediate, clear feedback when they make a mistake in TektonConfig — which is the entry point most people will actually use.
5 participants
Add spec.result.watcher so watcher flags can be configured through TektonConfig without manually patching tekton-results-watcher Deployment args.
Changes
ResultsWatcherPropertiesandspec.result.watcherto TektonConfig and TektonResult CRDstekton-results-watcherDeployment container args viaUpdateWatcherFlagsInDeploymentdocs/TektonConfig.mdanddocs/TektonResult.mdWatcher flags exposed include:
completed_run_grace_periodcheck_ownerstore_deadlinedisable_storing_incomplete_runslogs_api,logs_timestamps,store_eventsummary_labels,summary_annotations,label_selectorrequeue_interval,forward_buffer,update_log_timeout,dynamic_reconcile_timeout,disable_crd_updateSubmitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make test lintbefore submitting a PRSee the contribution guide for more details.
Release Notes