⚠ Update fake client deletionTimestamp behavior #2316

lleshchi · 2023-05-10T14:21:29Z

The current fake client methods diverge from the behavior of real k8s. This results in scenarios where an object can be created or modified to have a deletionTimestamp and no finalizers, with a delay in garbage collection, causing inaccurate or ambiguous behavior in test cases that use the fake client.

Bring fake client behavior closer in line with real k8s behavior by modifying the following:

Update() and Patch() will reject changes that set a previously-unset deletionTimestamp
Create() will ignore a deletionTimestamp on an object
Build() will error if initialized with an object that has a deletionTimestamp but no finalizers; but will accept a deletionTimestamp if it has finalizers.

linux-foundation-easycla · 2023-05-10T14:21:33Z

The committers listed above are authorized under a signed CLA.

✅ login: lleshchi / name: Leah Leshchinsky (d83c801)

k8s-ci-robot · 2023-05-10T14:21:37Z

Welcome @lleshchi!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-05-10T14:21:38Z

Hi @lleshchi. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

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.

vincepri · 2023-05-10T14:32:15Z

/ok-to-test

pkg/client/fake/client.go

go.mod

2uasimojo · 2023-05-15T16:08:12Z

This is looking solid to me. One minor query inline. Also, I'm not digging deep into the test cases -- leaving that for the experts :)

pkg/client/fake/client.go

pkg/client/fake/client_test.go

pkg/client/fake/client.go

pkg/client/fake/client_test.go

sbueringer · 2023-05-17T14:53:21Z

@lleshchi Could it be that you forgot to push the new version? :)

lleshchi · 2023-05-17T15:01:00Z

@sbueringer Was waiting on a clarification from @vincepri regarding one of the review comments. Otherwise, pushing changes now :)

vincepri · 2023-05-17T15:04:39Z

Which one? :)

lleshchi · 2023-05-17T15:10:43Z

@vincepri #2316 (comment)

Apologies, I reach out in slack about this. Should have posted here as well

The current fake client methods diverge from the behavior of real k8s. This results in scenarios where an object can be created or modified to have a deletionTimestamp and no finalizers, with a delay in garbage collection, causing inaccurate or ambiguous behavior in test cases that use the fake client. Bring fake client behavior closer in line with real k8s behavior by modifying the following: - Update() and Patch() will reject changes that set a previously-unset deletionTimestamp - Create() will ignore a deletionTimestamp on an object - Build() will error if initialized with an object that has a deletionTimestamp but no finalizers; but will accept a deletionTimestamp if it has finalizers. Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>

sbueringer · 2023-05-17T17:02:33Z

Thank you!

/lgtm

sbueringer · 2023-05-17T17:06:10Z

/approve

k8s-ci-robot · 2023-05-17T17:06:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lleshchi, sbueringer

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:

~~OWNERS~~ [sbueringer]

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

Rewrite unittests to accommodate changes in controller-runtime v0.15.0 and create a hive specific scheme. One such major change involves the behavior of the fake client deletionTimestamp. The fake client will panic if initialized with an object that has a DeletionTimestamp and no finalizer, and will silently ignore a DeletionTimestamp on an object in a Create request, matching the behavior of the kube-apiserver. Tests have been rewritten to work with this new behavior. kubernetes-sigs/controller-runtime#2316 This update also includes a requirement to register a fake client WithStatusSubresource in order to successfully update the Status subresource of an object. Due to this change, the helper function NewFakeClientBuilder() is added to pkg/test/fake to allow for easier setup of a fake client with the necessary sub-resources. kubernetes-sigs/controller-runtime#2259 Lastly, a new singleton scheme has been introduced to the codebase. The hive specific scheme is registered once, with all necessary packages, and used throughout the codebase, removing the need to specify unique schemes for each use case, and decreasing the overall amount of imports. Signed-off-by: Leah Leshchinsky <lleshchi@redhat.com>

Fake client won't create objects after kubernetes-sigs/controller-runtime#2316 if they have DeletionTimeStamp without finalizers Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com>

* Update k8s APIs to v1.28 Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com> * Satisfy fake clients requirement of finalizers with DeletionTimeStamp Fake client won't create objects after kubernetes-sigs/controller-runtime#2316 if they have DeletionTimeStamp without finalizers Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com> * Revert change in all_e2e_test.go Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com> * Refactored code Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com> --------- Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com>

…ernetes libs to v0.30.0 (#3707) * go version, dep version * refactor for controller-runtime udpate * update tests, fix if statement the controller-runtime strips DeletionTimestamp from manifests on Create(). kubernetes-sigs/controller-runtime#2316 * refactor tests * remove placeholder comments * remove reconciler from WithOptions * remove DefaultNamespace cache option * remote `&& !hasGroupFinalizer` This was causing e2e tests to fail when an ingress did not have the group finalizer. The unit tests ing-1_been_deleted, and ing-6_been_deleted will need reworked due to changes in the controller-runtime that cause them to fail. * update unit tests for ctrl client/fake >0.15 controller-runtime >=0.15 does not support creating (or adding the field via Update()) objects with a DeletionTimestamp. To work around this we add an annotation `unit-test/delete` to mark the ingresses that we want to test deletion. We check for this annotation and then call Delete(). This will set the DeletionTimestamp to the current date and time so we use IgnoreOtherFields to skip then comparing want to got. relevant controller-runtime discussion/pr: - kubernetes-sigs/controller-runtime#2184 (comment) - kubernetes-sigs/controller-runtime#2316 * remove unused contexts * add DefaultNamespaces cache config * set opt.Cache.DefaultNamespaces conditionally If WatchNamespace is set to corev1.NamespaceAll we should not set DefaultNamespaces. This code assumes that only one namespace is specified for WatchNamespace. That decision was based on the help text for the flag `watch-namespace`. Related controller-runtime issue: kubernetes-sigs/controller-runtime#2628 * make crds * set go to v1.22.3

…ernetes libs to v0.30.0 (kubernetes-sigs#3707) * go version, dep version * refactor for controller-runtime udpate * update tests, fix if statement the controller-runtime strips DeletionTimestamp from manifests on Create(). kubernetes-sigs/controller-runtime#2316 * refactor tests * remove placeholder comments * remove reconciler from WithOptions * remove DefaultNamespace cache option * remote `&& !hasGroupFinalizer` This was causing e2e tests to fail when an ingress did not have the group finalizer. The unit tests ing-1_been_deleted, and ing-6_been_deleted will need reworked due to changes in the controller-runtime that cause them to fail. * update unit tests for ctrl client/fake >0.15 controller-runtime >=0.15 does not support creating (or adding the field via Update()) objects with a DeletionTimestamp. To work around this we add an annotation `unit-test/delete` to mark the ingresses that we want to test deletion. We check for this annotation and then call Delete(). This will set the DeletionTimestamp to the current date and time so we use IgnoreOtherFields to skip then comparing want to got. relevant controller-runtime discussion/pr: - kubernetes-sigs/controller-runtime#2184 (comment) - kubernetes-sigs/controller-runtime#2316 * remove unused contexts * add DefaultNamespaces cache config * set opt.Cache.DefaultNamespaces conditionally If WatchNamespace is set to corev1.NamespaceAll we should not set DefaultNamespaces. This code assumes that only one namespace is specified for WatchNamespace. That decision was based on the help text for the flag `watch-namespace`. Related controller-runtime issue: kubernetes-sigs/controller-runtime#2628 * make crds * set go to v1.22.3

* update the traffic test for ingress (#3725) * update the traffic test for ingress * make crds * prevent controller runtime complaining about SetupLogger() was never called (#3724) * Update go to v1.22, controller-runtime dependency to v0.18.2, and kubernetes libs to v0.30.0 (#3707) * go version, dep version * refactor for controller-runtime udpate * update tests, fix if statement the controller-runtime strips DeletionTimestamp from manifests on Create(). kubernetes-sigs/controller-runtime#2316 * refactor tests * remove placeholder comments * remove reconciler from WithOptions * remove DefaultNamespace cache option * remote `&& !hasGroupFinalizer` This was causing e2e tests to fail when an ingress did not have the group finalizer. The unit tests ing-1_been_deleted, and ing-6_been_deleted will need reworked due to changes in the controller-runtime that cause them to fail. * update unit tests for ctrl client/fake >0.15 controller-runtime >=0.15 does not support creating (or adding the field via Update()) objects with a DeletionTimestamp. To work around this we add an annotation `unit-test/delete` to mark the ingresses that we want to test deletion. We check for this annotation and then call Delete(). This will set the DeletionTimestamp to the current date and time so we use IgnoreOtherFields to skip then comparing want to got. relevant controller-runtime discussion/pr: - kubernetes-sigs/controller-runtime#2184 (comment) - kubernetes-sigs/controller-runtime#2316 * remove unused contexts * add DefaultNamespaces cache config * set opt.Cache.DefaultNamespaces conditionally If WatchNamespace is set to corev1.NamespaceAll we should not set DefaultNamespaces. This code assumes that only one namespace is specified for WatchNamespace. That decision was based on the help text for the flag `watch-namespace`. Related controller-runtime issue: kubernetes-sigs/controller-runtime#2628 * make crds * set go to v1.22.3 * restrict resolve resolveViaVPCENIs to fargate only (#3709) * Added helm envFrom value parameter for cluster-name (#3683) * Added helm envFrom value parameter for cluster-name * Update README.md file * Add envFrom configuration to values.yaml * Remove empty line in values.yaml * feat: disable default helm labels (#3574) * feat: disable helm labels * fix: doc * Update README.md (#3638) Remove extra / in crds --------- Co-authored-by: Luke Arntz <luke@blue42.net> Co-authored-by: Omer Aplatony <61663422+omerap12@users.noreply.github.com> Co-authored-by: Rémi BUISSON <remi-buisson@orange.fr> Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>

k8s-ci-robot requested review from inteon and joelanford May 10, 2023 14:21

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 10, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 10, 2023

lleshchi force-pushed the deletion_timestamp branch 2 times, most recently from 8522ea3 to eeea5bb Compare May 10, 2023 16:50

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

2uasimojo reviewed May 10, 2023

View reviewed changes

lleshchi force-pushed the deletion_timestamp branch from eeea5bb to c28b017 Compare May 14, 2023 01:53

sbueringer added this to the v0.15.x milestone May 15, 2023

lleshchi force-pushed the deletion_timestamp branch from c28b017 to b7d4a85 Compare May 15, 2023 15:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2023

vincepri reviewed May 15, 2023

View reviewed changes

pkg/client/fake/client.go Outdated Show resolved Hide resolved

pkg/client/fake/client_test.go Show resolved Hide resolved

sbueringer reviewed May 16, 2023

View reviewed changes

lleshchi force-pushed the deletion_timestamp branch from b7d4a85 to c9d9599 Compare May 17, 2023 15:03

lleshchi force-pushed the deletion_timestamp branch from c9d9599 to 7a66d58 Compare May 17, 2023 15:21

k8s-ci-robot assigned sbueringer May 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2023

k8s-ci-robot merged commit c6570c2 into kubernetes-sigs:main May 17, 2023

ary1992 mentioned this pull request Jul 15, 2023

Upgrade k8s.io/* to v0.27, sigs.k8s.io/controller-runtime to v0.15 gardener/gardener#7991

Closed

Sunnatillo mentioned this pull request Jul 26, 2023

⚠️ Bump CAPI to v1.5.0, controller-runtime to v0.15.0 and controller-tools to v0.12.1 metal3-io/ip-address-manager#280

Merged

RainbowMango mentioned this pull request Jul 27, 2023

Bump Kubernetes dependencies to v1.27.3 karmada-io/karmada#3730

Merged

Sunnatillo mentioned this pull request Aug 1, 2023

⚠️ Bump CAPI to v1.5.0 metal3-io/cluster-api-provider-metal3#1079

Merged

mboersma mentioned this pull request Aug 9, 2023

Bump CAPI to v1.5.0 kubernetes-sigs/cluster-api-provider-azure#3707

Merged

4 tasks

Tamas-Biro1 mentioned this pull request Sep 4, 2023

update Go dependencies IBM/staticroute-operator#92

Merged

shreyas-s-rao mentioned this pull request Jan 7, 2024

Update gardener/gardener to v1.86.0 gardener/etcd-druid#748

Merged

12 tasks

ranakan19 mentioned this pull request Mar 25, 2024

SANDBOX-559 K8s 1 27 member codeready-toolchain/member-operator#551

Open

ranakan19 mentioned this pull request Jul 5, 2024

SANDBOX-560 update controller-runtime to v0.15 codeready-toolchain/host-operator#1057

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ Update fake client deletionTimestamp behavior #2316

⚠ Update fake client deletionTimestamp behavior #2316

lleshchi commented May 10, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 10, 2023 •

edited

Loading

k8s-ci-robot commented May 10, 2023

k8s-ci-robot commented May 10, 2023

vincepri commented May 10, 2023

2uasimojo commented May 15, 2023

sbueringer commented May 17, 2023

lleshchi commented May 17, 2023

vincepri commented May 17, 2023

lleshchi commented May 17, 2023

sbueringer commented May 17, 2023

sbueringer commented May 17, 2023

k8s-ci-robot commented May 17, 2023

⚠ Update fake client deletionTimestamp behavior #2316

⚠ Update fake client deletionTimestamp behavior #2316

Conversation

lleshchi commented May 10, 2023 • edited Loading

linux-foundation-easycla bot commented May 10, 2023 • edited Loading

k8s-ci-robot commented May 10, 2023

k8s-ci-robot commented May 10, 2023

vincepri commented May 10, 2023

2uasimojo commented May 15, 2023

sbueringer commented May 17, 2023

lleshchi commented May 17, 2023

vincepri commented May 17, 2023

lleshchi commented May 17, 2023

sbueringer commented May 17, 2023

sbueringer commented May 17, 2023

k8s-ci-robot commented May 17, 2023

lleshchi commented May 10, 2023 •

edited

Loading

linux-foundation-easycla bot commented May 10, 2023 •

edited

Loading