Skip to content
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.

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

Bump controller runtime to avoid mistakenly defaulting to wrong mapper #2896

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

akalenyu
Copy link
Collaborator

What this PR does / why we need it:
Today, controller runtime mistakenly ignores the inherited Manager default
dynamic mapper and uses a discovery mapper instead:
kubernetes-sigs/controller-runtime#2491
This means that if some CRD was not available on the cdi-controller startup,
Even if it got installed after, we would still get IsNoMatch when trying to access it.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
https://bugzilla.redhat.com/show_bug.cgi?id=2237470

Special notes for your reviewer:

Release note:

BugFix: If a CRD wasn't available during controller startup, our client would keep failing to get the objects,
mistakenly returning NoMatchError even if the CRD was installed since

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/S labels Sep 12, 2023
@akalenyu
Copy link
Collaborator Author

akalenyu commented Sep 12, 2023

/hold
lets get thoughts first, maybe we can live with discovery mapper peacefully for a while
we will switch to latest ctrl runtime in main soon, so this is mainly about backporting to releases

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2023
@alromeros
Copy link
Collaborator

Just wondering if this PR would still be necessary if your controller-runtime fix merges. As I understand, your PR kubernetes-sigs/controller-runtime#2491 fixes the issue in controller runtime v0.14, which is used in our older maintained releases. We don't have to worry about future releases since v0.15-0.16 doesn't have this issue and we plan to update soon. Is this PR still necessary, then?

@akalenyu
Copy link
Collaborator Author

akalenyu commented Sep 13, 2023

Just wondering if this PR would still be necessary if your controller-runtime fix merges. As I understand, your PR kubernetes-sigs/controller-runtime#2491 fixes the issue in controller runtime v0.14, which is used in our older maintained releases. We don't have to worry about future releases since v0.15-0.16 doesn't have this issue and we plan to update soon. Is this PR still necessary, then?

So we have the Bugzilla linked in this PR that triggered this investigation to begin with;
Basically, OpenShift routes were not available for some reason at the controller startup time, and since we use
the "wrong" mapper (non-dynamic one) our client can never get routes anymore until the controller pod is restarted.
There could be other pain points with using a non-dynamic mapper, or, we may be able to live with it
until controller runtime PR merges, a 0.14.z is made, and that is used in CDI.

Just wondering if this PR would still be necessary if your controller-runtime fix merges.

Yeah, this is the main question. If we decide that it's too painful that the client never recovers,
we may not be able to wait until the controller runtime release makes it's way to us.

@akalenyu akalenyu changed the title Work around controller runtime mistakenly defaulting to wrong mapper Bump controller runtime to avoid mistakenly defaulting to wrong mapper Sep 14, 2023
Today, controller runtime mistakenly ignores the inherited Manager default
dynamic mapper and uses a discovery mapper instead:
kubernetes-sigs/controller-runtime#2491
This means that if some CRD was not available on the cdi-controller startup,
Even if it got installed after, we would still get IsNoMatch when trying to access it.

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Aligned with kubernetes-sigs/controller-runtime#2559

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@akalenyu
Copy link
Collaborator Author

akalenyu commented Nov 8, 2023

/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2023
@awels
Copy link
Member

awels commented Nov 8, 2023

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2023
@awels
Copy link
Member

awels commented Nov 8, 2023

/cherrypick release-v1.58

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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:

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

@kubevirt-bot
Copy link
Contributor

@awels: once the present PR merges, I will cherry-pick it on top of release-v1.58 in a new PR and assign it to you.

In response to this:

/cherrypick release-v1.58

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.

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2023
@akalenyu
Copy link
Collaborator Author

akalenyu commented Nov 9, 2023

/retest

1 similar comment
@akalenyu
Copy link
Collaborator Author

akalenyu commented Nov 9, 2023

/retest

@kubevirt-bot kubevirt-bot merged commit ab15715 into kubevirt:main Nov 9, 2023
18 checks passed
@kubevirt-bot
Copy link
Contributor

@awels: new pull request created: #2977

In response to this:

/cherrypick release-v1.58

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.

@akalenyu
Copy link
Collaborator Author

akalenyu commented Nov 9, 2023

/cherrypick release-v1.57

@kubevirt-bot
Copy link
Contributor

@akalenyu: #2896 failed to apply on top of branch "release-v1.57":

Applying: Bump controller runtime to avoid mistakenly defaulting to wrong mapper
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bump controller runtime to avoid mistakenly defaulting to wrong mapper
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-v1.57

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants