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

Add system constraint provider logic to resolver #2523

Merged
merged 1 commit into from
Dec 16, 2021
Merged

Add system constraint provider logic to resolver #2523

merged 1 commit into from
Dec 16, 2021

Conversation

perdasilva
Copy link
Collaborator

Signed-off-by: Per G. da Silva perdasilva@redhat.com

Description of the change:
This PR adds a systemConstraintsProvider to the resolver. It also modifies the resolver to apply any constraints comming from the systemContraintsProvider. Secondly, it add initHooks to the OperatorStepResolver. This allows the downstream to modify resolver behaviour.

Motivation for the change:
For 4.10 we'd like to have a way for the resolver to respect the maxOpenShiftVersion property that can be provided by bundles to ensure that operator supports the cluster onto which it is being installed. This is done by injecting an OpenShift systemConstraintsProvider into the satResolver using an initHook on the downstream.

The init hooks provide a lightweight way of doing this from downstream without affecting the upstream or providing (in the upstream) a way for users to reach the systemConstraintsProvider. At least until we want to open this more generally.

Note: This PR must be merged sync'ed with the downstream before the downstream PR can be cut

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

@perdasilva perdasilva mentioned this pull request Dec 13, 2021
Closed
5 tasks
@akihikokuroda
Copy link
Member

I put a fix for the e2e error in #2517

@perdasilva
Copy link
Collaborator Author

/hold until #2418 is merged and this had been rebased

@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 Dec 13, 2021
anik120
anik120 requested changes Dec 13, 2021
View reviewed changes
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

@perdasilva I can see the commit here that modifies the kubeconfig flag. Should that be a separate PR if it's not related to what the PR title/summary talks about, unless it's absolutely necessary for that commit to be in this PR?

Also, is the downstream PR ready? It's hard to grasp how the init hook will be used or what code changes here are motivated by the eventual downstream code changes without looking at that code beside this.

pkg/controller/registry/resolver/resolver.go Outdated Show resolved Hide resolved
@perdasilva perdasilva force-pushed the cluster-constraints-v3 branch 2 times, most recently from d5f2f1f to 0a4119e Compare December 13, 2021 22:00
joelanford
joelanford reviewed Dec 13, 2021
View reviewed changes
pkg/api/client/client.go Outdated Show resolved Hide resolved
pkg/lib/operatorclient/client.go Outdated Show resolved Hide resolved
@perdasilva
Copy link
Collaborator Author

@perdasilva I can see the commit here that modifies the kubeconfig flag. Should that be a separate PR if it's not related to what the PR title/summary talks about, unless it's absolutely necessary for that commit to be in this PR?

Also, is the downstream PR ready? It's hard to grasp how the init hook will be used or what code changes here are motivated by the eventual downstream code changes without looking at that code beside this.

@anik120
I've removed the commit with the flag fixes. Here's a PR: perdasilva/operator-framework-olm#1 that contains both up and downs stream changes. (the upstream part may slightly differ due to changes in response to reviewer comments - but should be fine).

@perdasilva
Copy link
Collaborator Author

/unhold

@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 Dec 14, 2021
dinhxuanvu
dinhxuanvu requested changes Dec 14, 2021
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

Looking good. A few comments.

pkg/controller/registry/resolver/resolver.go Show resolved Hide resolved
pkg/controller/registry/resolver/resolver.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/solver/constraint_provider.go Show resolved Hide resolved
njhale
njhale approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link

openshift-ci bot commented Dec 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhale, perdasilva

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 Dec 14, 2021
estroz
estroz reviewed Dec 14, 2021
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Great work @perdasilva

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@estroz
Copy link
Member

estroz commented Dec 14, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@perdasilva
Copy link
Collaborator Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

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

3 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.

@perdasilva
Copy link
Collaborator Author

/retest

@perdasilva
Add system constraint provider logic to resolver and add step resolve…
25e15a8 
…r init hooks

Signed-off-by: Per G. da Silva <perdasilva@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2021
@timflannagan
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 15, 2021
@perdasilva
Copy link
Collaborator Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

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

3 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.

@perdasilva
Copy link
Collaborator Author

/retest

@openshift-bot
Copy link
Contributor

/retest-required

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

4 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-merge-robot openshift-merge-robot merged commit b414e6e into operator-framework:master Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanford joelanford joelanford left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu requested changes

@estroz estroz estroz left review comments

@anik120 anik120 anik120 requested changes

@njhale njhale njhale approved these changes

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

@benluddy benluddy Awaiting requested review from benluddy

Assignees

@timflannagan timflannagan

@estroz estroz

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.

None yet
10 participants
@perdasilva @akihikokuroda @estroz @openshift-bot @timflannagan @joelanford @anik120 @njhale @dinhxuanvu @openshift-merge-robot