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

Remove oudated subscription update logic to improve resolution delay #2369

Merged
merged 1 commit into from
Sep 29, 2021
Merged

Remove oudated subscription update logic to improve resolution delay #2369

merged 1 commit into from
Sep 29, 2021

Conversation

dinhxuanvu
Copy link
Member

Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display UpgradeAvailable status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the UpgradeAvailable status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh vudinh@outlook.com

Description of the change:

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive

@openshift-ci openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Sep 21, 2021
@openshift-ci
Copy link

openshift-ci bot commented Sep 21, 2021

@dinhxuanvu: This pull request references Bugzilla bug 2002276, which is invalid:

  • expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2002276: Remove oudated subscription update logic to improve resolution delay

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.

@openshift-ci openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 21, 2021
@dinhxuanvu dinhxuanvu changed the title Bug 2002276: Remove oudated subscription update logic to improve resolution delay Remove oudated subscription update logic to improve resolution delay Sep 21, 2021
@openshift-ci openshift-ci bot removed the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Sep 21, 2021
@openshift-ci
Copy link

openshift-ci bot commented Sep 21, 2021

@dinhxuanvu: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Remove oudated subscription update logic to improve resolution delay

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.

@openshift-ci openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 21, 2021
@openshift-ci
Copy link

openshift-ci bot commented Sep 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu

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 Sep 21, 2021
@dinhxuanvu dinhxuanvu force-pushed the sub-update branch 2 times, most recently from 576f314 to f43be74 Compare September 22, 2021 18:20
@kevinrizza
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@dinhxuanvu dinhxuanvu closed this Sep 23, 2021
@dinhxuanvu dinhxuanvu reopened this Sep 23, 2021
@dinhxuanvu
Remove oudated subscription update logic to improve resolution delay
27ef353 
Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display `UpgradeAvailable` status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the `UpgradeAvailable` status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2021
@dinhxuanvu
Copy link
Member Author

@kevinrizza Need a re-lgtm if you don't mind. Thanks.

@dinhxuanvu dinhxuanvu closed this Sep 24, 2021
@dinhxuanvu dinhxuanvu reopened this Sep 24, 2021
@kevinrizza
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021
benluddy
benluddy reviewed Sep 27, 2021
View reviewed changes
pkg/controller/operators/catalog/operator.go
Comment on lines 1107 to 1109
if err := querier.Queryable(); err != nil {
return nil, false, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed, too, right?

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 81e7a60 into operator-framework:master Sep 29, 2021
@nunnatsa
Copy link

nunnatsa commented Oct 3, 2021

@dinhxuanvu - HCO upgrade tests start failing in prow after merging this PR. The test subscribes to a previous version and then performs an upgrade to the PR version (the version under test). Could you please check?

@dinhxuanvu
Copy link
Member Author

@nunnatsa Can you create a thread on Slack and tag me on it? I need some further logging and info to identify what the issue may be.

@nunnatsa
Copy link

nunnatsa commented Oct 3, 2021

@nunnatsa Can you create a thread on Slack and tag me on it? I need some further logging and info to identify what the issue may be.

There are no relevant logs from OLM, because OLM just ignores the change in the subscription.

perdasilva pushed a commit to perdasilva/operator-lifecycle-manager that referenced this pull request Sep 13, 2022
@dinhxuanvu @perdasilva
Remove oudated subscription update logic to improve resolution delay (o…
0a5cb84 
…perator-framework#2369)

Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display `UpgradeAvailable` status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the `UpgradeAvailable` status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
perdasilva pushed a commit to perdasilva/operator-lifecycle-manager that referenced this pull request Sep 13, 2022
@dinhxuanvu @perdasilva
Remove oudated subscription update logic to improve resolution delay (o…
e34aa22 
…perator-framework#2369)

Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display `UpgradeAvailable` status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the `UpgradeAvailable` status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
perdasilva pushed a commit to perdasilva/operator-lifecycle-manager that referenced this pull request Sep 13, 2022
@dinhxuanvu @perdasilva
Remove oudated subscription update logic to improve resolution delay (o…
e7cddf7 
…perator-framework#2369)

Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display `UpgradeAvailable` status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the `UpgradeAvailable` status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
perdasilva pushed a commit to perdasilva/operator-lifecycle-manager that referenced this pull request Sep 13, 2022
@dinhxuanvu @perdasilva
Remove oudated subscription update logic to improve resolution delay (o…
0fcb146 
…perator-framework#2369)

Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display `UpgradeAvailable` status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the `UpgradeAvailable` status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
openshift-merge-robot pushed a commit that referenced this pull request Nov 22, 2022
@perdasilva @dinhxuanvu
Remove oudated subscription update logic to improve resolution delay (#…
da9d438 
…2369) (#2856)

Currently, olm logic checks for upgrade in subscription via another
obsolete API that is no longer in use for dependency solution. As a
result, sometimes, subscriptions display `UpgradeAvailable` status but
there will be no upgrades as the upgrade is not valid in the resolver.
Also, the `UpgradeAvailable` status is used to trigger the new resolution
even though that status is no longer a valid indicator of having a pending
upgrade. This leads to unwanted upgrade delay when the obsolete API works
properly.

This commit will remove the code that is using this obsolete API and
allow the resolution to happen when there is a subscription change.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>

Signed-off-by: Vu Dinh <vudinh@outlook.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
Co-authored-by: Vu Dinh <vdinh@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benluddy benluddy benluddy left review comments

@timflannagan timflannagan Awaiting requested review from timflannagan

Assignees

@kevinrizza kevinrizza

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.
6 participants
@dinhxuanvu @kevinrizza @openshift-bot @nunnatsa @benluddy @openshift-merge-robot