Skip to content

Return copy of annotations in grpcCatalogSourceDecorator #2813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 11, 2022
Merged

Return copy of annotations in grpcCatalogSourceDecorator #2813

merged 2 commits into from
Jul 11, 2022

Conversation

perdasilva
Copy link
Collaborator

Signed-off-by: perdasilva [email protected]

Description of the change:
Recent changes adding the grpcCatalogSourceDecorator introduced a concurrent write issue on the annotations. This PR updates the Annotations method to return a copy of the annotations to avoid this issue.

Motivation for the change:
https://bugzilla.redhat.com/show_bug.cgi?id=2101357

Architectural changes:
None

Testing remarks:
Deployed on kind and not seeing the issue

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2022
@perdasilva perdasilva force-pushed the fix_concurrent_write branch from 8822702 to f580018 Compare July 7, 2022 12:30
awgreene
awgreene approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could generate the list of annotations once when creating the grpcCatalogSourceDecorator, but I don't think this will introduce a huge performance impact.

timflannagan
timflannagan approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on Alex's suggestion as well. Looking through the rest of these methods, it looks like we wouldn't have the same issue as labels. Do we need a test here?

@timflannagan
Copy link
Member

Feel free to remove the hold.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022
@perdasilva
Copy link
Collaborator Author

+1 on Alex's suggestion as well. Looking through the rest of these methods, it looks like we wouldn't have the same issue as labels. Do we need a test here?

I was trying to think how to add one, but wasn't sure the best way to test it. I'm also wondering if we should move the logic to the reconciler.

@awgreene
Copy link
Member

awgreene commented Jul 7, 2022

I think this is a small enough change that I'm comfortable merging as is.

@timflannagan
Copy link
Member

Yep, same here, and it's definitely helpful that you linked to the BZ so we're tracking some historical context over time.

@perdasilva perdasilva force-pushed the fix_concurrent_write branch from f580018 to 191b2dd Compare July 8, 2022 08:29
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2022
@perdasilva perdasilva force-pushed the fix_concurrent_write branch from 191b2dd to 6823170 Compare July 8, 2022 08:30
@perdasilva perdasilva force-pushed the fix_concurrent_write branch from 6823170 to e0cd528 Compare July 8, 2022 08:33
awgreene
awgreene reviewed Jul 8, 2022
View reviewed changes
awgreene
awgreene requested changes Jul 8, 2022
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one spelling mistake, otherwise looks good.

@perdasilva
Copy link
Collaborator Author

Found one spelling mistake, otherwise looks good.

Fixed!! good eyes =D

@timflannagan
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2022
@perdasilva @awgreene
Update pkg/controller/registry/reconciler/reconciler_test.go
a40d0b4 
Co-authored-by: Alexander Greene <[email protected]>
Signed-off-by: perdasilva <[email protected]>
@perdasilva perdasilva force-pushed the fix_concurrent_write branch from ab09aae to a40d0b4 Compare July 11, 2022 17:06
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022
awgreene
awgreene reviewed Jul 11, 2022
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to add a comment stating that the reconcile.Pod function will mutate the labels/annotations provided and updating all calls to provide copies, but making the function copy the arrays is probably safer for future developers that might rely on this method.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022
@timflannagan
Copy link
Member

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2022
@awgreene
Copy link
Member

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, perdasilva, timflannagan

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 [awgreene,perdasilva,timflannagan]

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

@openshift-ci openshift-ci bot merged commit 83e3ebf into operator-framework:master Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awgreene awgreene awgreene approved these changes

@timflannagan timflannagan timflannagan approved these changes

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

Assignees

@awgreene awgreene

@timflannagan timflannagan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@perdasilva @timflannagan @awgreene