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.

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

Fix RichReference Ordering #2880

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Fix RichReference Ordering #2880

merged 1 commit into from
Nov 1, 2022

Conversation

awgreene
Copy link
Member

Problem: The operator CR's status includes a list of componenets owned by the operator. The list of components are ordered by GVK, but the order of objects with the same GVK may change. If an operator owns many components, there is a high likelyhood that OLM will continuously update the status of the operator CR because the order of its components have changed

Solution: Order an operators component references so OLM does not attempt to update the status of the operator CR.

Signed-off-by: Alexander Greene greene.al1991@gmail.com

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2022
@openshift-ci
Copy link

openshift-ci bot commented Oct 27, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link

openshift-ci bot commented Oct 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2022
@awgreene
Copy link
Member Author

Fixes #2874

@awgreene awgreene force-pushed the fix-operator-controller branch 2 times, most recently from 3b10544 to 4043df9 Compare October 27, 2022 02:45
@awgreene awgreene mentioned this pull request Oct 27, 2022
Closed
@awgreene awgreene force-pushed the fix-operator-controller branch 2 times, most recently from 87e311e to e694a2b Compare October 27, 2022 13:48
@awgreene
Copy link
Member Author

I'll have to bump the k8s version to v0.25.0 to use the changes introduced in operator-framework/api#271

awgreene
awgreene commented Oct 27, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@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 Oct 27, 2022
@awgreene awgreene force-pushed the fix-operator-controller branch 4 times, most recently from f45bab5 to 4eddd8c Compare October 27, 2022 15:30
@awgreene awgreene marked this pull request as ready for review October 31, 2022 16:53
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2022
@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 Oct 31, 2022
@awgreene awgreene force-pushed the fix-operator-controller branch 2 times, most recently from b0a6ee7 to 508d4d2 Compare October 31, 2022 16:57
@dtfranz
Copy link
Contributor

dtfranz commented Oct 31, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@awgreene
Copy link
Member Author

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@openshift-ci
Copy link

openshift-ci bot commented Oct 31, 2022

New changes are detected. LGTM label has been removed.

@timflannagan
Copy link
Contributor

LGTM once #2880 (comment) is resolved and tests are green. Just want to make sure we're not introducing a debug artifact into the reconciliation logic, and I think we could get the current status view from inspecting the actual object itself.

@awgreene
Order an operator CR's status.Component.Refs array
ec1e04b 
Problem: The operator CR's status includes a list of componenets owned
by the operator. The list of components are ordered by GVK, but the
order of objects with the same GVK may change. If an operator owns many
components, there is a high likelyhood that OLM will continuously
update the status of the operator CR because the order of its
components have changed,

Solution: Order an operators component references so OLM does not
attempt to update the status of the operator CR.

Signed-off-by: Alexander Greene <greene.al1991@gmail.com>
@awgreene
Copy link
Member Author

awgreene commented Nov 1, 2022

Thanks for the review @timflannagan, we should be good to go!

@dtfranz
Copy link
Contributor

dtfranz commented Nov 1, 2022

/lgtm

1 similar comment
@oceanc80
Copy link
Contributor

oceanc80 commented Nov 1, 2022

/lgtm

@awgreene awgreene added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2022
@openshift-merge-robot openshift-merge-robot merged commit 793a7cc into operator-framework:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timflannagan timflannagan timflannagan left review comments

@dtfranz dtfranz dtfranz left review comments

@asmacdo asmacdo asmacdo left review comments

@joelanford joelanford joelanford left review comments

@gallettilance gallettilance Awaiting requested review from gallettilance

@oceanc80 oceanc80 Awaiting requested review from oceanc80

Assignees

@dtfranz dtfranz

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.
7 participants
@awgreene @dtfranz @timflannagan @oceanc80 @joelanford @asmacdo @openshift-merge-robot