apf: change controller to use SSA for patches #110173

tkashem · 2022-05-23T14:31:53Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This brings back #107456 to see if it introduces flakes/timeouts in the integration tests

reproduce the integration test timeout that this PR introduces
check the very large log file of the integration test and determine the root cause

Which issue(s) this PR fixes:

Fixes #107727

Special notes for your reviewer:

Does this PR introduce a user-facing change?

-->

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tkashem · 2022-05-23T15:07:18Z

/hold

tkashem · 2022-05-23T15:30:42Z

suggestions:

kubernetes/staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

Lines 602 to 615 in aa49dff

    
           if a.group.OpenAPIModels != nil && utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) { 
        
           	reqScope.FieldManager, err = fieldmanager.NewDefaultFieldManager( 
        
           		a.group.TypeConverter, 
        
           		a.group.UnsafeConvertor, 
        
           		a.group.Defaulter, 
        
           		a.group.Creater, 
        
           		fqKindToRegister, 
        
           		reqScope.HubGroupVersion, 
        
           		subresource, 
        
           		resetFields, 
        
           	) 
        
           	if err != nil { 
        
           		return nil, nil, fmt.Errorf("failed to create field manager: %v", err) 
        
           	}

we probably have to remove utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply from the conditional

kubernetes/staging/src/k8s.io/apiserver/pkg/features/kube_features.go

Line 216 in aa49dff

OpenAPIV3: {Default: true, PreRelease: featuregate.Beta},

kubernetes/staging/src/k8s.io/apiserver/pkg/features/kube_features.go

Line 198 in aa49dff

APIPriorityAndFairness: {Default: true, PreRelease: featuregate.Beta},

probably set LockToDefault:true

we will need to reproduce the issue first, apply these changes and show that it fixes the issue.

tkashem · 2022-05-23T15:40:52Z

cc @aojea @MikeSpreitzer @wojtek-t

tkashem · 2022-05-24T02:03:44Z

/test kubernetes-integration

k8s-ci-robot · 2022-05-24T02:03:46Z

@tkashem: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-kubernetes-conformance-kind-ga-only-parallel
/test pull-kubernetes-coverage-unit
/test pull-kubernetes-dependencies
/test pull-kubernetes-dependencies-go-canary
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-gce-big-performance
/test pull-kubernetes-e2e-gce-canary
/test pull-kubernetes-e2e-gce-network-proxy-http-connect
/test pull-kubernetes-e2e-gce-no-stage
/test pull-kubernetes-e2e-gce-scale-performance-manual
/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-e2e-gce-ubuntu-containerd-canary
/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-files-remake
/test pull-kubernetes-integration
/test pull-kubernetes-integration-go-canary
/test pull-kubernetes-kubemark-e2e-gce-scale
/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-typecheck
/test pull-kubernetes-unit
/test pull-kubernetes-unit-go-canary
/test pull-kubernetes-update
/test pull-kubernetes-verify
/test pull-kubernetes-verify-go-canary
/test pull-kubernetes-verify-govet-levee

The following commands are available to trigger optional jobs:

/test check-dependency-stats
/test pull-kubernetes-conformance-image-test
/test pull-kubernetes-conformance-kind-ga-only
/test pull-kubernetes-conformance-kind-ipv6-parallel
/test pull-kubernetes-cross
/test pull-kubernetes-e2e-capz-azure-disk
/test pull-kubernetes-e2e-capz-azure-disk-vmss
/test pull-kubernetes-e2e-capz-azure-file
/test pull-kubernetes-e2e-capz-azure-file-vmss
/test pull-kubernetes-e2e-capz-conformance
/test pull-kubernetes-e2e-capz-ha-control-plane
/test pull-kubernetes-e2e-capz-windows-containerd
/test pull-kubernetes-e2e-containerd-gce
/test pull-kubernetes-e2e-gce-alpha-features
/test pull-kubernetes-e2e-gce-correctness
/test pull-kubernetes-e2e-gce-csi-serial
/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-e2e-gce-kubetest2
/test pull-kubernetes-e2e-gce-network-proxy-grpc
/test pull-kubernetes-e2e-gce-storage-disruptive
/test pull-kubernetes-e2e-gce-storage-slow
/test pull-kubernetes-e2e-gce-storage-snapshot
/test pull-kubernetes-e2e-gci-gce-autoscaling
/test pull-kubernetes-e2e-gci-gce-ingress
/test pull-kubernetes-e2e-gci-gce-ipvs
/test pull-kubernetes-e2e-kind-canary
/test pull-kubernetes-e2e-kind-dual-canary
/test pull-kubernetes-e2e-kind-ipv6-canary
/test pull-kubernetes-e2e-kind-ipvs-dual-canary
/test pull-kubernetes-e2e-kind-multizone
/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-ubuntu-gce-network-policies
/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-local-e2e
/test pull-kubernetes-node-crio-cgrpv2-e2e
/test pull-kubernetes-node-crio-cgrpv2-e2e-kubetest2
/test pull-kubernetes-node-crio-e2e
/test pull-kubernetes-node-crio-e2e-kubetest2
/test pull-kubernetes-node-e2e-containerd-alpha-features
/test pull-kubernetes-node-e2e-containerd-features
/test pull-kubernetes-node-e2e-containerd-features-kubetest2
/test pull-kubernetes-node-e2e-containerd-kubetest2
/test pull-kubernetes-node-kubelet-credential-provider
/test pull-kubernetes-node-kubelet-serial-containerd
/test pull-kubernetes-node-kubelet-serial-containerd-kubetest2
/test pull-kubernetes-node-kubelet-serial-cpu-manager
/test pull-kubernetes-node-kubelet-serial-cpu-manager-kubetest2
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv1
/test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
/test pull-kubernetes-node-kubelet-serial-hugepages
/test pull-kubernetes-node-kubelet-serial-memory-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager-kubetest2
/test pull-kubernetes-node-memoryqos-cgrpv2
/test pull-kubernetes-node-swap-fedora
/test pull-kubernetes-node-swap-fedora-serial
/test pull-kubernetes-node-swap-ubuntu-serial
/test pull-kubernetes-unit-experimental
/test pull-publishing-bot-validate

Use /test all to run the following jobs that were automatically triggered:

pull-kubernetes-conformance-kind-ga-only-parallel
pull-kubernetes-conformance-kind-ipv6-parallel
pull-kubernetes-dependencies
pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-e2e-gce-ubuntu-containerd
pull-kubernetes-e2e-kind
pull-kubernetes-e2e-kind-ipv6
pull-kubernetes-integration
pull-kubernetes-node-e2e-containerd
pull-kubernetes-typecheck
pull-kubernetes-unit
pull-kubernetes-verify
pull-kubernetes-verify-govet-levee

In response to this:

/test kubernetes-integration

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.

tkashem · 2022-05-24T02:04:14Z

/test pull-kubernetes-integration

tkashem · 2022-05-24T12:01:07Z

/test pull-kubernetes-integration

wojtek-t · 2022-05-24T13:38:46Z

@tkashem - I don't think we should be trying to reproduce by running integration tests at this point.

IIRC, this PR was causing panics of kube-apiserver - can you please start with checking the logs that those are not happening (or maybe rather first confirm it was the case previously)?

tkashem · 2022-05-24T14:28:54Z

IIRC, this PR was causing panics of kube-apiserver - can you please start with checking the logs that those are not happening (or maybe rather first confirm it was the case previously)

Yes, I already saw the panic yesterday from the first passing integration job. I am re-running the integration test to see if it has other effects.

leilajal · 2022-05-24T20:11:28Z

/triage accepted

tkashem · 2022-05-29T12:23:39Z

/test pull-kubernetes-integration

tkashem · 2022-06-02T12:05:03Z

/test pull-kubernetes-integration

tkashem · 2022-09-14T02:58:58Z

/assign @MikeSpreitzer

wojtek-t · 2022-09-14T11:22:48Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go

@@ -462,6 +458,37 @@ func (cfgCtlr *configController) digestConfigObjects(newPLs []*flowcontrol.Prior
 	return suggestedDelay, utilerrors.NewAggregate(errs)
 }

+func updateOrApply(client flowcontrolclient.FlowSchemaInterface, fsu fsStatusUpdate, asFieldManager string) error {
+	if utilfeature.DefaultFeatureGate.Enabled(features.ServerSideApply) {


@liggitt - we're generally locking GA features to true and SSA is already GA, but not locked:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/features/kube_features.go#L234

What was the reason for it?

[I'm asking because if it was locked, then we wouldn't need this conditional logic here.]

cc @apelisse - I'd be in favor of locking/dropping the conditional apply gate logic

Opened #112748

@tkashem - this is now merged
Once the dependent PR are merged you can update this PR and get rid of this condition - this if will now always be true.

liggitt · 2022-09-15T16:38:55Z

I just noticed this PR referenced in #112306 (comment)

can we resolve the test flake in reset_fields_test.go and drop flowschemas from resetFieldsSkippedResources before re-enabling SSA in the APF controller?

tkashem · 2022-09-16T13:45:35Z

I just noticed this PR referenced in #112306 (comment)

can we resolve the test flake in reset_fields_test.go and drop flowschemas from resetFieldsSkippedResources before re-enabling SSA in the APF controller?

@liggitt are you referring to adding the patchStrategy annotation in Condition slices in FlowSchemaStatus and PriorityLevelConfigurationStatus of v1beta3 so strategic merge patch does not replace the entire conditions slice? #107574

Please see - #104842 (comment)

liggitt · 2022-09-16T13:49:10Z

@liggitt are you referring to adding the patchStrategy annotation in Condition slices in FlowSchemaStatus and PriorityLevelConfigurationStatus of v1beta3 so strategic merge patch does not replace the entire conditions slice? #107574

If that was the cause of and resolution to the patch test conflict/issue, then yes

would this order of operations make sense?

v1beta3 type with strategic merge patch annotations and switch the controller to v1beta3 (add v1beta3 for Priority And Fairness #112306)
remove flowschemas from resetFieldsSkippedResources (another PR)
switch the controller to SSA (this PR)

tkashem · 2022-09-19T18:04:45Z

/hold

depends on the following to merge:

MikeSpreitzer · 2022-10-04T03:21:51Z

@tkashem : the reasons for the hold on this are gone, right?

tkashem · 2022-10-08T12:23:01Z

the reasons for the hold on this are gone, right?

@MikeSpreitzer I wanted to make sure that #112575 resolves #104842

It's been a week, and https://storage.googleapis.com/k8s-triage/index.html?date=2022-10-01&job=unit&test=TestApplyResetFields yields zero result, so I am confident the flake has been resolved

Also with #112748 merged, SSA is always enabled, so I removed the conditiona updateOrApply method.

MikeSpreitzer

/LGTM
Thanks!

k8s-ci-robot · 2022-10-10T20:24:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, tkashem

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:

~~staging/src/k8s.io/apiserver/pkg/util/flowcontrol/OWNERS~~ [MikeSpreitzer]

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

tkashem · 2022-10-17T19:23:14Z

/hold cancel

all dependencies have merged, and search in ci yields yields zero fake

wojtek-t · 2022-11-02T11:43:22Z

for posterity - this LGTM.

MikeSpreitzer · 2022-11-04T16:53:13Z

And thanks all for persistence in getting this done.

k8s-ci-robot requested review from johnSchnake and lavalamp May 23, 2022 14:32

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 23, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 24, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2022

tkashem mentioned this pull request Sep 14, 2022

add v1beta3 for Priority And Fairness #112306

Merged

k8s-ci-robot assigned MikeSpreitzer Sep 14, 2022

wojtek-t reviewed Sep 14, 2022

View reviewed changes

wojtek-t self-assigned this Sep 14, 2022

tkashem force-pushed the apf-use-ssa branch from 564b524 to 5f2c603 Compare September 19, 2022 18:03

wojtek-t mentioned this pull request Sep 27, 2022

Lock ServerSideApply feature to true #112748

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2022

apiserver: use SSA for apf configuration client

ecc640f

tkashem force-pushed the apf-use-ssa branch from 5f2c603 to ecc640f Compare October 8, 2022 12:19

k8s-ci-robot 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 8, 2022

MikeSpreitzer approved these changes Oct 10, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2022

k8s-ci-robot merged commit b87802b into kubernetes:master Oct 17, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apf: change controller to use SSA for patches #110173

apf: change controller to use SSA for patches #110173

tkashem commented May 23, 2022 •

edited

tkashem commented May 23, 2022

tkashem commented May 23, 2022 •

edited

tkashem commented May 23, 2022

tkashem commented May 24, 2022

k8s-ci-robot commented May 24, 2022

tkashem commented May 24, 2022

tkashem commented May 24, 2022

wojtek-t commented May 24, 2022

tkashem commented May 24, 2022

leilajal commented May 24, 2022

tkashem commented May 29, 2022

tkashem commented Jun 2, 2022

tkashem commented Sep 14, 2022

wojtek-t Sep 14, 2022

liggitt Sep 15, 2022

wojtek-t Sep 27, 2022

wojtek-t Sep 28, 2022

liggitt commented Sep 15, 2022

tkashem commented Sep 16, 2022

liggitt commented Sep 16, 2022

tkashem commented Sep 19, 2022

MikeSpreitzer commented Oct 4, 2022

tkashem commented Oct 8, 2022

MikeSpreitzer left a comment

k8s-ci-robot commented Oct 10, 2022

tkashem commented Oct 17, 2022

wojtek-t commented Nov 2, 2022

MikeSpreitzer commented Nov 4, 2022

apf: change controller to use SSA for patches #110173

apf: change controller to use SSA for patches #110173

Conversation

tkashem commented May 23, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tkashem commented May 23, 2022

tkashem commented May 23, 2022 • edited

tkashem commented May 23, 2022

tkashem commented May 24, 2022

k8s-ci-robot commented May 24, 2022

tkashem commented May 24, 2022

tkashem commented May 24, 2022

wojtek-t commented May 24, 2022

tkashem commented May 24, 2022

leilajal commented May 24, 2022

tkashem commented May 29, 2022

tkashem commented Jun 2, 2022

tkashem commented Sep 14, 2022

wojtek-t Sep 14, 2022

Choose a reason for hiding this comment

liggitt Sep 15, 2022

Choose a reason for hiding this comment

wojtek-t Sep 27, 2022

Choose a reason for hiding this comment

wojtek-t Sep 28, 2022

Choose a reason for hiding this comment

liggitt commented Sep 15, 2022

tkashem commented Sep 16, 2022

liggitt commented Sep 16, 2022

tkashem commented Sep 19, 2022

MikeSpreitzer commented Oct 4, 2022

tkashem commented Oct 8, 2022

MikeSpreitzer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 10, 2022

tkashem commented Oct 17, 2022

wojtek-t commented Nov 2, 2022

MikeSpreitzer commented Nov 4, 2022

tkashem commented May 23, 2022 •

edited

tkashem commented May 23, 2022 •

edited