Fix Deep Copy issue in getting controller reference #124116

HiranmoyChowdhury · 2024-03-30T10:48:58Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR is to fix the Deep Copy issue in this func. As in the same directory a function named "GetControllerOfNoCopy" present which is supposed to return the original controller reference and the function named "GetControllerOf" is supposed to return the controller reference after making Deep Copy. But, It just returns the copy reference of the controller which doesn't make deep copy of "Controller" and "BlockOwnerDeletion" field inside of "Owner Reference" struct. After making changes in copied controller reference(such as, *copiedControllerRef.BlockOwnerDeletion = false), the original value also changes.

Does this PR introduce a user-facing change?

NONE

Fix Deep Copy issue in getting controller reference

linux-foundation-easycla · 2024-03-30T10:49:02Z

The committers listed above are authorized under a signed CLA.

✅ login: HiranmoyChowdhury / name: Hiranmoy Das Chowdhury (0cd2588)

k8s-ci-robot · 2024-03-30T10:49:03Z

Please note that we're already in Test Freeze for the release-1.30 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.30.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sat Mar 30 08:14:50 UTC 2024.

k8s-ci-robot · 2024-03-30T10:49:07Z

Welcome @HiranmoyChowdhury!

It looks like this is your first PR to kubernetes/kubernetes 🎉. 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/kubernetes 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 · 2024-03-30T10:49:07Z

Hi @HiranmoyChowdhury. Thanks for your PR.

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

HiranmoyChowdhury · 2024-03-30T10:58:10Z

/easycla

k8s-triage-robot · 2024-03-30T11:16:04Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

justinsb · 2024-04-01T13:09:31Z

/sig apimachinery

k8s-ci-robot · 2024-04-01T13:09:33Z

@justinsb: The label(s) sig/apimachinery cannot be applied, because the repository doesn't have them.

In response to this:

/sig apimachinery

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.

justinsb · 2024-04-01T13:10:15Z

/sig api-machinery

HiranmoyChowdhury · 2024-04-02T04:01:01Z

These changes do not affect previously written code or projects but will benefit future projects

cici37 · 2024-04-04T16:57:58Z

/ok-to-test
/remove-sig api-machinery

This is a API change PR. Please follow the process here: https://github.com/kubernetes/community/blob/master/sig-architecture/api-review-process.md#mechanics . Thanks!

sttts · 2024-04-07T14:32:13Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/controller_ref_test.go

+	// test deepCopy
+	if (c.Controller == controllerRef.Controller) ||
+		(c.BlockOwnerDeletion != nil && c.BlockOwnerDeletion == controllerRef.BlockOwnerDeletion) {
+		t.Errorf("deepCopy failed of GetControllerOf: %v", c)


nit:

Suggested change

t.Errorf("deepCopy failed of GetControllerOf: %v", c)

t.Errorf("GetControllerOf did not return deep copy: %v", c)

sttts · 2024-04-07T14:32:37Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/controller_ref_test.go

@@ -95,6 +96,11 @@ func TestGetControllerOf(t *testing.T) {
 	if c.Name != controllerRef.Name || c.UID != controllerRef.UID {
 		t.Errorf("Incorrect result of GetControllerOf: %v", c)
 	}
+	// test deepCopy


nit:

Suggested change

// test deepCopy

// test that all pointers are also deep copied

sttts · 2024-04-07T14:34:09Z

Looks right.

Nit: the release note is not really necessary as this is not a (non-developer) user-facing change.

sttts · 2024-04-07T14:35:34Z

/assign @kubernetes/api-reviewers

sttts · 2024-04-09T06:39:37Z

/lgtm
/approve

Needs an api reviewer approval.

k8s-ci-robot · 2024-04-09T06:39:43Z

LGTM label has been added.

Git tree hash: 99b0fa39a3c5c69a43904f3fbd1a9ab8146f4675

alexzielenski · 2024-04-09T19:18:39Z

/triage accepted
/cc deads2k jpbetz

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/controller_ref.go

liggitt · 2024-04-09T19:33:38Z

/approve
/milestone v1.31

k8s-ci-robot · 2024-04-09T19:33:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HiranmoyChowdhury, liggitt, sttts

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/publishing/OWNERS~~ [liggitt,sttts]
~~staging/src/k8s.io/apimachinery/pkg/apis/OWNERS~~ [liggitt]

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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels Mar 30, 2024

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 30, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 30, 2024

k8s-ci-robot requested review from justinsb and soltysh March 30, 2024 10:49

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 30, 2024

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 1, 2024

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Apr 5, 2024

HiranmoyChowdhury force-pushed the hiranmoy branch from f1a5391 to 6fc550d Compare April 7, 2024 11:11

sttts reviewed Apr 7, 2024

View reviewed changes

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 7, 2024

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Apr 7, 2024

HiranmoyChowdhury requested a review from dprotaso April 7, 2024 16:33

k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Apr 9, 2024

deep copy issue in getting controller is solved

0cd2588

HiranmoyChowdhury force-pushed the hiranmoy branch from 946d848 to 0cd2588 Compare April 9, 2024 05:14

HiranmoyChowdhury requested a review from sttts April 9, 2024 05:16

k8s-ci-robot assigned sttts Apr 9, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2024

k8s-ci-robot requested review from deads2k and jpbetz April 9, 2024 19:18

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 Apr 9, 2024

liggitt reviewed Apr 9, 2024

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/controller_ref.go Show resolved Hide resolved

k8s-ci-robot added this to the v1.31 milestone Apr 9, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2024

liggitt self-assigned this Apr 11, 2024

k8s-ci-robot merged commit 3261821 into kubernetes:master Apr 18, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Deep Copy issue in getting controller reference #124116

Fix Deep Copy issue in getting controller reference #124116

HiranmoyChowdhury commented Mar 30, 2024

linux-foundation-easycla bot commented Mar 30, 2024 •

edited

Loading

k8s-ci-robot commented Mar 30, 2024

k8s-ci-robot commented Mar 30, 2024

k8s-ci-robot commented Mar 30, 2024

HiranmoyChowdhury commented Mar 30, 2024

k8s-triage-robot commented Mar 30, 2024

justinsb commented Apr 1, 2024

k8s-ci-robot commented Apr 1, 2024

justinsb commented Apr 1, 2024

HiranmoyChowdhury commented Apr 2, 2024

cici37 commented Apr 4, 2024

sttts Apr 7, 2024

sttts Apr 7, 2024 •

edited

Loading

sttts commented Apr 7, 2024

sttts commented Apr 7, 2024

sttts commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

alexzielenski commented Apr 9, 2024

liggitt commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

	t.Errorf("deepCopy failed of GetControllerOf: %v", c)
	t.Errorf("GetControllerOf did not return deep copy: %v", c)

	// test deepCopy

	// test that all pointers are also deep copied

Fix Deep Copy issue in getting controller reference #124116

Fix Deep Copy issue in getting controller reference #124116

Conversation

HiranmoyChowdhury commented Mar 30, 2024

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

linux-foundation-easycla bot commented Mar 30, 2024 • edited Loading

k8s-ci-robot commented Mar 30, 2024

k8s-ci-robot commented Mar 30, 2024

k8s-ci-robot commented Mar 30, 2024

HiranmoyChowdhury commented Mar 30, 2024

k8s-triage-robot commented Mar 30, 2024

justinsb commented Apr 1, 2024

k8s-ci-robot commented Apr 1, 2024

justinsb commented Apr 1, 2024

HiranmoyChowdhury commented Apr 2, 2024

cici37 commented Apr 4, 2024

sttts Apr 7, 2024

Choose a reason for hiding this comment

sttts Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

sttts commented Apr 7, 2024

sttts commented Apr 7, 2024

sttts commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

alexzielenski commented Apr 9, 2024

liggitt commented Apr 9, 2024

k8s-ci-robot commented Apr 9, 2024

linux-foundation-easycla bot commented Mar 30, 2024 •

edited

Loading

sttts Apr 7, 2024 •

edited

Loading