🐛 Change the type of `markers.Collector.byPackage`'s key from `string` to `*loader.Package` #792

ntoofu · 2023-04-08T18:58:53Z

As I described in #783 (comment) , this issue is caused by using Package.ID as a key for cache of markers. This PR adds a testcase to reproduce the issue and fixes it by using Package itself instead.

k8s-ci-robot · 2023-04-08T18:59:01Z

Welcome @ntoofu!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. 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-tools 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-04-08T18:59:02Z

Hi @ntoofu. 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.

Markers might be lost in generated CRD when the package containing markers is referenced by several packages and CRD is generated by those packages. For more details, see kubernetes-sigs#783.

Markers are saved per `*ast.TypeSpec`, which is specific to `Package` object. `*ast.TypeSpec` in another `Package` object is different even when the `Package.ID` is the same, and so collected markers cannot be reused for another `Package` object. Therefore, `Package` itself is used as a key for cache of markers insetead of `Package.ID`. For more details about the bug, see kubernetes-sigs#783.

camilamacedo86 · 2023-04-19T15:12:02Z

/ok-to-test

randomvariable · 2023-07-11T09:36:16Z

cc @sbueringer fixes kubernetes-sigs/cluster-api-provider-vsphere#1928

sbueringer · 2023-07-11T14:33:43Z

Thx for the extensive explanation here: #783 (comment)

/lgtm

vincepri · 2023-07-11T17:04:00Z

pkg/markers/collect.go

@@ -31,7 +31,7 @@ import (
 type Collector struct {
 	*Registry

-	byPackage map[string]map[ast.Node]MarkerValues
+	byPackage map[*loader.Package]map[ast.Node]MarkerValues


This stores the package based on the pointer's value, is there any other way that doesn't rely on where the memory is allocated?

Thank you for your reviewing!

There is possibly another way, but it may be complicated.

byPackage has two keys, package ID (string) and node (ast.Node). Package ID is not a pointer's value, while node is an interface and can be a struct containing pointers. That inconsistency is the root cause of this issue.

In the problematic situation, exactly the same package is loaded several times. Among each load, the package ID is the same but nodes are different objects. Parsing markers is skipped when the same package ID is already processed. Therefore, byPackage's node keys are created only at the first time. As a result, accessing byPackage using node as a key fails from the second load of the package.

To avoid to use pointer's value as byPackage's key, I think we have to do one of the following.

Stop using ast.Node as a key (but I have no idea about the alternative)

Do not skip parsing markers even if a package with the same ID is already parsed

Stop loading the same package multiple times (but I have no idea why the multiple load occurs)

Yup agree. The problem is definitely that we cache the markers (with pointers) by package ID and then try to use them for another package (which has different ast nodes).

(Basically byPackage is map[string]map[ast.Node][]markerComment, where ast.Node is an interface and can be e.g. *ast.File, *ast.Field, ...)

I think it should be not worse to use *loader.Package as a map key then it already is to use the ast.Node interface (aka e.g. *ast.File) as a map key one layer deeper)

That's fair, it's a bit triggering though in review 😄

Yeah 100% :)

vincepri · 2023-07-12T17:55:32Z

/approve

k8s-ci-robot · 2023-07-12T17:55:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ntoofu, vincepri

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~~ [vincepri]

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

sbueringer · 2023-07-12T18:02:58Z

/cherry-pick release-0.11

k8s-infra-cherrypick-robot · 2023-07-12T18:03:39Z

@sbueringer: new pull request created: #833

In response to this:

/cherry-pick release-0.11

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.

sbueringer · 2023-07-12T18:04:16Z

Oh don't need the fix in release-0.11 (although no objection to merge there).

Just missed that we have a v0.12 release because we don't have a release branch for v0.12 yet

sbueringer · 2023-07-13T05:18:38Z

I created a release-0.12 branch based on v0.12.0

/cherry-pick release-0.12

k8s-infra-cherrypick-robot · 2023-07-13T05:19:19Z

@sbueringer: new pull request created: #834

In response to this:

I created a release-0.12 branch based on v0.12.0

/cherry-pick release-0.12

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.

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 8, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 8, 2023

k8s-ci-robot requested review from droot and pwittrock April 8, 2023 18:59

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

ntoofu added 2 commits April 9, 2023 12:49

Add a testcase to reproduce bug of markers

32940b6

Markers might be lost in generated CRD when the package containing markers is referenced by several packages and CRD is generated by those packages. For more details, see kubernetes-sigs#783.

ntoofu changed the title ~~🐛 Fix #783~~ 🐛 Change the type of markers.Collector.byPackage's key from string to *loader.Package Apr 9, 2023

ntoofu force-pushed the fix-783 branch from 8c3189c to b73fdf7 Compare April 9, 2023 03:57

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 9, 2023

k8s-ci-robot 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 Apr 19, 2023

camilamacedo86 closed this Apr 20, 2023

camilamacedo86 reopened this Apr 20, 2023

ntoofu mentioned this pull request Jul 6, 2023

make verify-crds intermittently produces different output kubernetes-sigs/cluster-api-provider-vsphere#1928

Closed

k8s-ci-robot assigned sbueringer Jul 11, 2023

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

vincepri reviewed Jul 11, 2023

View reviewed changes

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

k8s-ci-robot merged commit a9f7560 into kubernetes-sigs:master Jul 12, 2023

k8s-infra-cherrypick-robot mentioned this pull request Jul 12, 2023

[release-0.11] 🐛 Change the type of markers.Collector.byPackage's key from string to *loader.Package #833

Closed

k8s-infra-cherrypick-robot mentioned this pull request Jul 13, 2023

[release-0.12] 🐛 Change the type of markers.Collector.byPackage's key from string to *loader.Package #834

Merged

sbueringer mentioned this pull request Jul 13, 2023

🌱 Bump controller-tools to v0.12.1 kubernetes-sigs/cluster-api-provider-vsphere#2004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Change the type of `markers.Collector.byPackage`'s key from `string` to `*loader.Package` #792

🐛 Change the type of `markers.Collector.byPackage`'s key from `string` to `*loader.Package` #792

ntoofu commented Apr 8, 2023

k8s-ci-robot commented Apr 8, 2023

k8s-ci-robot commented Apr 8, 2023

camilamacedo86 commented Apr 19, 2023

randomvariable commented Jul 11, 2023

sbueringer commented Jul 11, 2023

vincepri Jul 11, 2023

ntoofu Jul 12, 2023 •

edited

Loading

sbueringer Jul 12, 2023 •

edited

Loading

sbueringer Jul 12, 2023

vincepri Jul 12, 2023

sbueringer Jul 12, 2023

vincepri commented Jul 12, 2023

k8s-ci-robot commented Jul 12, 2023

sbueringer commented Jul 12, 2023

k8s-infra-cherrypick-robot commented Jul 12, 2023

sbueringer commented Jul 12, 2023

sbueringer commented Jul 13, 2023

k8s-infra-cherrypick-robot commented Jul 13, 2023

🐛 Change the type of markers.Collector.byPackage's key from string to *loader.Package #792

🐛 Change the type of markers.Collector.byPackage's key from string to *loader.Package #792

Conversation

ntoofu commented Apr 8, 2023

k8s-ci-robot commented Apr 8, 2023

k8s-ci-robot commented Apr 8, 2023

camilamacedo86 commented Apr 19, 2023

randomvariable commented Jul 11, 2023

sbueringer commented Jul 11, 2023

vincepri Jul 11, 2023

Choose a reason for hiding this comment

ntoofu Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

sbueringer Jul 12, 2023

Choose a reason for hiding this comment

vincepri Jul 12, 2023

Choose a reason for hiding this comment

sbueringer Jul 12, 2023

Choose a reason for hiding this comment

vincepri commented Jul 12, 2023

k8s-ci-robot commented Jul 12, 2023

sbueringer commented Jul 12, 2023

k8s-infra-cherrypick-robot commented Jul 12, 2023

sbueringer commented Jul 12, 2023

sbueringer commented Jul 13, 2023

k8s-infra-cherrypick-robot commented Jul 13, 2023

🐛 Change the type of `markers.Collector.byPackage`'s key from `string` to `*loader.Package` #792

🐛 Change the type of `markers.Collector.byPackage`'s key from `string` to `*loader.Package` #792

ntoofu Jul 12, 2023 •

edited

Loading

sbueringer Jul 12, 2023 •

edited

Loading