Change dry-run behavior by Alexander-Kita · Pull Request #16008 · knative/serving

Alexander-Kita · 2025-08-11T21:56:18Z

Proposed Changes

Will always/only run dry-run if specified in user request
Remove podspec-dryrun feature flag

Release Note

* podspec-dryrun feature flag has been removed. Dry run validation will now occur when a user opts into it using `kubectl apply --dry-run=server`

knative-prow · 2025-08-11T21:56:27Z

Welcome @Alexander-Kita! It looks like this is your first PR to knative/serving 🎉

knative-prow · 2025-08-11T21:56:28Z

Hi @Alexander-Kita. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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-sigs/prow repository.

dprotaso · 2025-08-14T14:13:29Z

/ok-to-test

codecov · 2025-08-14T14:18:42Z

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.17%. Comparing base (6bab565) to head (eec0a1a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/webhook/podspec_dryrun.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16008      +/-   ##
==========================================
+ Coverage   80.13%   80.17%   +0.04%     
==========================================
  Files         214      214              
  Lines       16893    16877      -16     
==========================================
- Hits        13537    13531       -6     
+ Misses       2994     2987       -7     
+ Partials      362      359       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dprotaso

I think as part of this change we should drop the feature flag

Alexander-Kita · 2025-08-24T05:47:31Z

I think as part of this change we should drop the feature flag

Removed feature flag, dry run validation now only occurs when dry-run=server is specified. Please let me know if I missed anything.

@dprotaso

dprotaso

some minor feedback

dprotaso · 2025-08-28T13:02:09Z

test/v1/service.go

+func PatchServiceWithDryRun(t testing.TB, clients *test.Clients, service *v1.Service, fopt ...rtesting.ServiceOption) (svc *v1.Service, err error) {
+	newSvc := service.DeepCopy()
+	for _, opt := range fopt {
+		opt(newSvc)
+	}
+	LogResourceObject(t, ResourceObjects{Service: newSvc})
+	patchBytes, err := duck.CreateBytePatch(service, newSvc)
+	if err != nil {
+		return nil, err
+	}
+	return svc, reconciler.RetryTestErrors(func(int) (err error) {
+		svc, err = clients.ServingClient.Services.Patch(context.Background(), service.ObjectMeta.Name, types.JSONPatchType, patchBytes, metav1.PatchOptions{DryRun: []string{metav1.DryRunAll}})
+		return err
+	})
+}
+


Let's move this to test/e2e/service_validation_test.go as it's specific to that test

dprotaso · 2025-08-28T13:03:21Z

test/e2e/service_validation_test.go

 	}

-	_, err = v1test.PatchService(t, clients, service, withInvalidContainer())
+	_, err = v1test.PatchServiceWithDryRun(t, clients, service, withInvalidContainer())


This is technically changing the test to use dry-run.

I'd probably suggest having an alternate 'dry-run' e2e test

This test does not work the same as it used to because we removed the feature flag (it had previously used the annotation). Should the test be renamed or moved instead?

oh then let's rename the test

dprotaso · 2025-08-28T13:09:09Z

pkg/webhook/validate_unstructured_test.go


 func TestSkipUpdate(t *testing.T) {
 	validService := &v1.Service{
-		ObjectMeta: metav1.ObjectMeta{


TestSkipUpdate - would be more apt-ly named TestSkipDryRun no?

I believe it is specifically testing the update portion, so maybe it could be TestSkipUpdateDryrun?

Looking at it more - it's ensuring we skip calling and k8s calls when we perform a dry-run (when the resource hasn't changed).

I say we just remove this behaviour and delete this test. Ideally when a user performs a dry run we ensure we validate everything even if it's the same. The reason is because we wouldn't know if something 'new' is running in the cluster that could influence the podspec dry-run.

I think this will simplify the implementation.

Alexander-Kita · 2025-08-29T03:33:49Z

@dprotaso Implemented requested changes in latest commit

dprotaso

looks great - just two minor things we don't need.

I removed it from my local copy and it didn't have any material effect

dprotaso · 2025-09-03T00:39:49Z

pkg/reconciler/route/table_test.go

 		cfg.Domain = v.(*config.Domain)
 	}

+	ctx = apis.WithDryRun(ctx)


We can drop this

dprotaso · 2025-09-03T00:39:55Z

pkg/reconciler/route/resources/service_test.go

 		t.Run(tc.name, func(t *testing.T) {
 			ingress := &netv1alpha1.Ingress{Status: tc.status}
 			ctx := config.ToContext(context.Background(), testConfig())
+			ctx = apis.WithDryRun(ctx)


we can drop this

dprotaso · 2025-09-03T00:51:23Z

Also two more things

Can you take a look at the linter error in the GitHub Actions?
Can you update the docs here https://github.com/knative/docs (removing the feature flag and maybe adding a dry-run example in under Serving > Developer Topics)

Alexander-Kita · 2025-09-06T05:13:44Z

@dprotaso Looks like the passed except for one, which seems unrelated to the code (seems like some kind of missing file issue)

dprotaso · 2025-09-08T13:52:26Z

/retest

dprotaso · 2025-09-08T13:53:47Z

/lgtm
/approve

thanks @Alexander-Kita 🎉

knative-prow · 2025-09-08T13:53:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Alexander-Kita, dprotaso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow bot requested review from dsimansk and skonto August 11, 2025 21:56

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 11, 2025

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 14, 2025

dprotaso reviewed Aug 21, 2025

View reviewed changes

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 24, 2025

dprotaso reviewed Aug 28, 2025

View reviewed changes

Alexander-Kita requested a review from dprotaso September 2, 2025 14:31

dprotaso reviewed Sep 3, 2025

View reviewed changes

Alexander-Kita mentioned this pull request Sep 6, 2025

Add dry run section and take out old feature flag in serving knative/docs#6366

Merged

Alexander-Kita added 6 commits September 5, 2025 20:26

optimize dryrun

bf580ab

add tests for skip create case

cfcdc9b

cleanup code

f2f2ce6

remove dryrun feature flag

0d25c1f

remove unnecessary line

c426ed1

fix test

f5c9521

Alexander-Kita added 2 commits September 5, 2025 20:26

cleanup and remove update check

ab1eac0

linting and cleanup

eec0a1a

Alexander-Kita force-pushed the optimize-dryrun branch from 727161b to eec0a1a Compare September 6, 2025 01:27

knative-prow bot assigned dprotaso Sep 8, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2025

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2025

knative-prow bot merged commit 097de3d into knative:main Sep 8, 2025
78 of 79 checks passed

Alexander-Kita deleted the optimize-dryrun branch September 8, 2025 15:00

Conversation

Alexander-Kita commented Aug 11, 2025 • edited by dprotaso Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow bot commented Aug 11, 2025

Uh oh!

knative-prow bot commented Aug 11, 2025

Uh oh!

dprotaso commented Aug 14, 2025

Uh oh!

codecov bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dprotaso left a comment

Choose a reason for hiding this comment

Uh oh!

Alexander-Kita commented Aug 24, 2025

Uh oh!

dprotaso left a comment

Choose a reason for hiding this comment

Uh oh!

dprotaso Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

Alexander-Kita Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

Alexander-Kita Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alexander-Kita commented Aug 29, 2025

Uh oh!

dprotaso left a comment

Choose a reason for hiding this comment

Uh oh!

dprotaso Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Sep 3, 2025

Uh oh!

Alexander-Kita commented Sep 6, 2025

Uh oh!

dprotaso commented Sep 8, 2025

Uh oh!

dprotaso commented Sep 8, 2025

Uh oh!

knative-prow bot commented Sep 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Alexander-Kita commented Aug 11, 2025 •

edited by dprotaso

Loading

codecov bot commented Aug 14, 2025 •

edited

Loading

Alexander-Kita Aug 28, 2025 •

edited

Loading

dprotaso Aug 28, 2025 •

edited

Loading