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

Add OperatorCondition status sync and update operator upgradeable check #2160

Merged

Conversation

dinhxuanvu
Copy link
Member

@dinhxuanvu dinhxuanvu commented May 13, 2021

OperatorCondition controller will update its status to refect the
changes on its spec regarding the operator conditions that are reported
by the operators themselves. In turn, operators can read the status
to confirm if OLM has processed the spec changes.

OLM will not take actions on upgradeable condition if the status is
stale using ObservedGeneration and Generation check.

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

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1927340.

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

@dinhxuanvu dinhxuanvu requested review from ecordell and awgreene May 13, 2021 06:54
@openshift-ci openshift-ci bot requested review from gallettilance and hasbro17 May 13, 2021 06:54
@dinhxuanvu dinhxuanvu changed the title Add OperatorCondition status sync and update operator upgradeable check Bug 1927340: Add OperatorCondition status sync and update operator upgradeable check May 13, 2021
@openshift-ci openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 13, 2021
@openshift-ci
Copy link

openshift-ci bot commented May 13, 2021

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

  • expected the bug to target the "4.8.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 1927340: Add OperatorCondition status sync and update operator upgradeable check

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.

@timflannagan
Copy link
Member

The BZ bot will automatically move the BZ state to POST once this PR lands but we only want that behavior for downstream PRs. I'm going to add a link to the BZ in your PR description instead.

/retitle Add OperatorCondition status sync and update operator upgradeable check

@openshift-ci openshift-ci bot changed the title Bug 1927340: Add OperatorCondition status sync and update operator upgradeable check Add OperatorCondition status sync and update operator upgradeable check May 13, 2021
@openshift-ci openshift-ci bot removed bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 13, 2021
@openshift-ci
Copy link

openshift-ci bot commented May 13, 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:

Add OperatorCondition status sync and update operator upgradeable check

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.

@dinhxuanvu
Copy link
Member Author

The proposal: https://hackmd.io/9wG20hu5TU-y1HrkhvcsZQ?view FYI

@dinhxuanvu dinhxuanvu force-pushed the opcon-gen branch 5 times, most recently from ea0767c to 99861b9 Compare May 25, 2021 20:59
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.

Please remove the FIT test, looks good otherwise.

test/e2e/operator_condition_e2e_test.go Outdated Show resolved Hide resolved
@dinhxuanvu dinhxuanvu force-pushed the opcon-gen branch 2 times, most recently from f5c2fc0 to 2cda4a0 Compare May 26, 2021 17:18
@dinhxuanvu dinhxuanvu force-pushed the opcon-gen branch 6 times, most recently from d24c663 to 537a9a3 Compare June 2, 2021 14:09
@dinhxuanvu dinhxuanvu force-pushed the opcon-gen branch 7 times, most recently from 8ac44ee to faa73fe Compare June 8, 2021 10:19
OperatorCondition controller will update its status to refect the
changes on its spec regarding the operator conditions that are reported
by the operators themselves. In turn, operators can read the status
to confirm if OLM has processed the spec changes.

OLM will not take actions on upgradeable condition if the status is
stale using ObservedGeneration and Generation check.

Signed-off-by: Vu Dinh <vudinh@outlook.com>
1. Operator can create/update/patch OperatorCondition but not its
status.
2. Update the e2e test case to update OperatorCondition's spec
directly instead of its status

Signed-off-by: Vu Dinh <vudinh@outlook.com>
1. Update codegen to generate operatorv2 client code

2. Vendor operator-framework/api 0.9.0

3. Adopt the new changes on OperatorCondition API from api repo.

4. Update OperatorCondition v2 references across the codebase

5. Update e2e test case for OperatorCondition

Signed-off-by: Vu Dinh <vudinh@outlook.com>
@dinhxuanvu
Copy link
Member Author

@ecordell PTAL

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.

A few nits, but it looks great!

pkg/lib/ownerutil/util.go Show resolved Hide resolved
test/e2e/operator_condition_e2e_test.go Show resolved Hide resolved
test/e2e/operator_condition_e2e_test.go Show resolved Hide resolved
@awgreene
Copy link
Member

awgreene commented Jun 9, 2021

Overall this all looks good to me, most of the points I raised are nits so I'll add the /approve label.

@awgreene awgreene added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

@kevinrizza
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2021
@openshift-merge-robot openshift-merge-robot merged commit f9beca5 into operator-framework:master Jun 9, 2021
@dinhxuanvu
Copy link
Member Author

/cherry-pick release-4.7

@openshift-cherrypick-robot

@dinhxuanvu: #2160 failed to apply on top of branch "release-4.7":

Applying: Add OperatorCondition status sync and update operator upgradeable check
Applying: Update OperatorCondition Role and e2e test case
Applying: Address various issues with OperatorCondition codebase
Using index info to reconstruct a base tree...
M	deploy/chart/crds/0000_50_olm_00-operatorconditions.crd.yaml
M	go.mod
M	go.sum
M	pkg/controller/operators/adoption_controller.go
M	pkg/controller/operators/olm/operator.go
M	pkg/lib/operatorlister/lister.go
M	vendor/github.com/operator-framework/api/crds/zz_defs.go
M	vendor/github.com/operator-framework/api/pkg/validation/internal/operatorhub.go
M	vendor/github.com/operator-framework/api/pkg/validation/validation.go
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 vendor/github.com/operator-framework/api/pkg/validation/validation.go
CONFLICT (content): Merge conflict in vendor/github.com/operator-framework/api/pkg/validation/validation.go
Auto-merging vendor/github.com/operator-framework/api/pkg/validation/internal/operatorhub.go
CONFLICT (content): Merge conflict in vendor/github.com/operator-framework/api/pkg/validation/internal/operatorhub.go
Auto-merging vendor/github.com/operator-framework/api/crds/zz_defs.go
CONFLICT (content): Merge conflict in vendor/github.com/operator-framework/api/crds/zz_defs.go
Auto-merging pkg/lib/operatorlister/lister.go
Auto-merging pkg/controller/operators/olm/operator.go
Auto-merging pkg/controller/operators/adoption_controller.go
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging deploy/chart/crds/0000_50_olm_00-operatorconditions.crd.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Address various issues with OperatorCondition codebase
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:

/cherry-pick release-4.7

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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants