⚠ Status condition clean up #379

anik120 · 2024-09-06T12:36:52Z

RFC: ClusterCatalog Status Conditions implementation.

Closes 363 Closes 364 Closes 365

codecov · 2024-09-06T13:33:18Z

Codecov Report

Attention: Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 36.05%. Comparing base (e1d3605) to head (e36d516).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rnal/controllers/core/clustercatalog_controller.go	97.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #379      +/-   ##
==========================================
- Coverage   37.25%   36.05%   -1.20%     
==========================================
  Files          14       14              
  Lines         816      796      -20     
==========================================
- Hits          304      287      -17     
  Misses        463      463              
+ Partials       49       46       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/controllers/core/clustercatalog_controller.go

internal/controllers/core/clustercatalog_controller_test.go

grokspawn · 2024-09-06T15:04:09Z

Seems like we should also update here and here and update the demos.
If you prefer, we can do this as a follow-on issue, but we should generally strive to keep the demos up to date.

internal/controllers/core/clustercatalog_controller.go

ankitathomas

great work!

There are a few more mentions of the Unpacking condition we should probably clean up in the README.md, docs/fetching-catalog-contents.md, test/tools/imageregistry/pre-upgrade-setup.sh and hack/scripts/gzip-demo-script.sh and hack/scripts/demo-script.sh.

We'd want to at least update the pre-upgrade-setup script since that's part of upgrade testing, but it'd be good to keep everything else up to date

api/core/v1alpha1/clustercatalog_types.go

anik120 · 2024-09-06T17:03:18Z

great work!

There are a few more mentions of the Unpacking condition we should probably clean up in the README.md, docs/fetching-catalog-contents.md, test/tools/imageregistry/pre-upgrade-setup.sh and hack/scripts/gzip-demo-script.sh and hack/scripts/demo-script.sh.

We'd want to at least update the pre-upgrade-setup script since that's part of upgrade testing, but it'd be good to keep everything else up to date

Thanks for pointing this out @ankitathomas ! I updated most of it, except for the pre-upgrade-setup.sh file. Correct me if I'm wrong, but this has to remain unchanged in this PR, since main (pre this commit) still has the Unpacking condition, which mean the script will expect that condition. There needs to be a follow up PR, that has to change this script.

Or did I get that wrong? The pre-upgrade test is passing with this setting though....

Also, I'll look into the updating of the actual demo file after lunch, but for now I ran into this issue, not sure if @grokspawn can help

make demo-update           
hack/scripts/generate-asciidemo.sh
hack/scripts/generate-asciidemo.sh: line 40: [: /var/folders/9p/q0l_ddk11jqd05vb098xk9f40000gn/T/d.mVVVtAWPPU: binary operator expected
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5481  100  5481    0     0  17382      0 --:--:-- --:--:-- --:--:-- 17400
curl: (6) Could not resolve host: generate-asciidemo.DTJEN
chmod: generate-asciidemo.DTJEN/asciinema-rec_script: Not a directory
/var/folders/9p/q0l_ddk11jqd05vb098xk9f40000gn/T/d.mVVVtAWPPU: line 155: generate-asciidemo.DTJEN/asciinema-rec_script: Not a directory
asciinema: /Users/anbhatta/go/src/github.com/operator-framework/catalogd/hack/scripts/demo-script.sh already exists, aborting
asciinema: use --overwrite option if you want to overwrite existing recording
asciinema: use --append option if you want to append to existing recording
usage: asciinema [-h] [--version] {rec,play,cat,upload,auth} ...
asciinema: error: unrecognized arguments: generate-asciidemo.DTJEN/catalogd-demo.cast
hack/scripts/generate-asciidemo.sh: line 17: [: /var/folders/9p/q0l_ddk11jqd05vb098xk9f40000gn/T/d.mVVVtAWPPU: binary operator expected
make: *** [demo-update] Error 2

ankitathomas · 2024-09-06T17:41:39Z

Everything looks good!

There needs to be a follow up PR, that has to change this script.

If we're ok with the test breaking before we're done with the API review work, it may be easier to have this change on your PR and cut a new release immediately after, but you're right - we can also update the upgrade-e2e script in a followup PR if we want to keep the test passing.

grokspawn · 2024-09-06T19:08:15Z

make demo-update
hack/scripts/generate-asciidemo.sh
hack/scripts/generate-asciidemo.sh: line 40: [: /var/folders/9p/q0l_ddk11jqd05vb098xk9f40000gn/T/d.mVVVtAWPPU: binary operator expected
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 5481 100 5481 0 0 17382 0 --:--:-- --:--:-- --:--:-- 17400
curl: (6) Could not resolve host: generate-asciidemo.DTJEN
chmod: generate-asciidemo.DTJEN/asciinema-rec_script: Not a directory

This looks like your bash env wasn't able to test if the tempdir exists here.
You'd have to see why that test is failing.

internal/controllers/core/clustercatalog_controller.go

internal/controllers/core/clustercatalog_controller_test.go

grokspawn · 2024-09-06T19:37:29Z

Correct me if I'm wrong, but this has to remain unchanged in this PR

IMO, we're making these changes in full awareness that we are breaking the API. We have opted not to create an interim API version to avoid it, and in the past we've committed and tried to cut a new release of the component to get the test passing again.

internal/controllers/core/clustercatalog_controller.go

hack/scripts/demo-script.sh

everettraven

These changes LGTM, but we should probably get @grokspawn and @joelanford to do a final pass as well prior to merging

anik120 · 2024-09-18T10:26:18Z

@ankitathomas fyi I removed the changes to the demo scripts and related stuff. The cognitive load required for review was probably getting a little heavier because of those changes, at least for me. We'll most definitely have more high quality reviews on the changes you want to make there if we put it up as a separate PR anyway.

anik120 · 2024-09-18T10:26:31Z

@ankitathomas @joelanford @everettraven ready for review again.

docs/fetching-catalog-contents.md

everettraven · 2024-09-18T17:38:34Z

internal/controllers/core/clustercatalog_controller.go

 		}
+		updateStatusNotServing(&catalog.Status)


Could we add a comment here that talks about the issue that we encountered when we both modified the status and the ClusterCatalog finalizers? I think that issue still exists, we just won't hot-loop because of Joe's change and we would end up setting the status to the same thing (i.e no change) so we end up successfully removing the finalizer on the second reconcile.

Created #409. I don't think we need a TODO comment here besides the issue

test/tools/imageregistry/pre-upgrade-setup.sh

anik120 · 2024-09-19T13:24:53Z

codecov/project is complaining about a test not being included for the panic being introduced, that replaced dead/unused code (unpacking related) from main

README.md

[RFC: ClusterCatalog Status Conditions](https://docs.google.com/document/d/1Kg8ovX8d25OqGa5utwcKbX3nN3obBl2KIYJHYz6IlKU/edit) implementation. Closes [363](operator-framework#363) Closes [364](operator-framework#364) Closes [365](operator-framework#365) Co-authored-by: Ankita Thomas <ankithom@redhat.com> Co-authored-by: Anik Bhattacharjee <anbhatta@redhat.com>

Signed-off-by: Todd Short <todd.short@me.com>

joelanford · 2024-09-19T21:11:54Z

internal/controllers/core/clustercatalog_controller.go

@@ -308,10 +280,13 @@ func NewFinalizers(localStorage storage.Instance, unpacker source.Unpacker) (crf
 			panic("could not convert object to clusterCatalog")
 		}
 		if err := localStorage.Delete(catalog.Name); err != nil {
-			return crfinalizer.Result{}, updateStatusStorageDeleteError(&catalog.Status, err)
+			updateStatusProgressing(catalog, err)
+			return crfinalizer.Result{}, err


I think we need to return crfinalizer.Result{StatusUpdated: true} on any code path that changes the status conditions. Otherwise, I'm not sure we fall into the if finalizeResult.Updated || finalizeResult.StatusUpdated block in line 130.

Signed-off-by: Todd Short <todd.short@me.com>

anik120 requested a review from a team as a code owner September 6, 2024 12:36

anik120 force-pushed the remove-upack-delete branch from 58d5bcc to b64388b Compare September 6, 2024 13:29

anik120 changed the title ~~(feat) Status condition clean up~~ ✨ Status condition clean up Sep 6, 2024

anik120 force-pushed the remove-upack-delete branch from b64388b to 0b0c5dd Compare September 6, 2024 13:48

anik120 changed the title ~~✨ Status condition clean up~~ ⚠ Status condition clean up Sep 6, 2024

anik120 force-pushed the remove-upack-delete branch from 0b0c5dd to 78454fb Compare September 6, 2024 13:49

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller_test.go Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller_test.go Show resolved Hide resolved

grokspawn reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

ankitathomas reviewed Sep 6, 2024

View reviewed changes

api/core/v1alpha1/clustercatalog_types.go Show resolved Hide resolved

anik120 force-pushed the remove-upack-delete branch from 78454fb to e8e2fd3 Compare September 6, 2024 16:58

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller_test.go Outdated Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller_test.go Outdated Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

everettraven reviewed Sep 6, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

m1kola reviewed Sep 10, 2024

View reviewed changes

internal/controllers/core/clustercatalog_controller.go Outdated Show resolved Hide resolved

ankitathomas force-pushed the remove-upack-delete branch 2 times, most recently from 7760e8d to 7d2f636 Compare September 10, 2024 17:56

everettraven reviewed Sep 10, 2024

View reviewed changes

hack/scripts/demo-script.sh Outdated Show resolved Hide resolved

everettraven previously approved these changes Sep 10, 2024

View reviewed changes

anik120 mentioned this pull request Sep 18, 2024

🐛 unpack cache cleanup should ignore missing files #404

Merged

LalatenduMohanty mentioned this pull request Sep 18, 2024

Bump catalogd to get the latest API changes operator-framework/operator-controller#1201

Closed

everettraven reviewed Sep 18, 2024

View reviewed changes

docs/fetching-catalog-contents.md Outdated Show resolved Hide resolved

everettraven reviewed Sep 18, 2024

View reviewed changes

ankitathomas force-pushed the remove-upack-delete branch from 3167ee5 to d902ad3 Compare September 18, 2024 21:00

joelanford mentioned this pull request Sep 19, 2024

🐛 fix: do not silently drop finalizer updates when status is also updated operator-framework/operator-controller#1283

Merged

4 tasks

joelanford reviewed Sep 19, 2024

View reviewed changes

test/tools/imageregistry/pre-upgrade-setup.sh Outdated Show resolved Hide resolved

This was referenced Sep 19, 2024

Handle finalizers (creation and deletion) better #409

Closed

fetching-catalog-content.md is outdated #410

Open

anik120 force-pushed the remove-upack-delete branch from d902ad3 to fa7b181 Compare September 19, 2024 09:23

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2024

anik120 force-pushed the remove-upack-delete branch from fa7b181 to 5101830 Compare September 19, 2024 09:39

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2024

anik120 force-pushed the remove-upack-delete branch 2 times, most recently from 750f545 to 32dd761 Compare September 19, 2024 13:13

tmshort reviewed Sep 19, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

anik120 mentioned this pull request Sep 19, 2024

🐛 Handle finalizers (creation and deletion) better #411

Merged

anik120 and others added 2 commits September 19, 2024 16:01

Handle new finalizer code

a7a03d2

Signed-off-by: Todd Short <todd.short@me.com>

tmshort force-pushed the remove-upack-delete branch from 32dd761 to a7a03d2 Compare September 19, 2024 20:52

joelanford reviewed Sep 19, 2024

View reviewed changes

Set StatusUpdated to true for finalizers

e36d516

Signed-off-by: Todd Short <todd.short@me.com>

joelanford enabled auto-merge September 20, 2024 11:53

joelanford approved these changes Sep 20, 2024

View reviewed changes

joelanford added this pull request to the merge queue Sep 20, 2024

Merged via the queue into operator-framework:main with commit 2dc04d9 Sep 20, 2024
11 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ Status condition clean up #379

⚠ Status condition clean up #379

anik120 commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

grokspawn commented Sep 6, 2024

ankitathomas left a comment

anik120 commented Sep 6, 2024

ankitathomas commented Sep 6, 2024

grokspawn commented Sep 6, 2024

grokspawn commented Sep 6, 2024

everettraven left a comment

anik120 commented Sep 18, 2024

anik120 commented Sep 18, 2024

everettraven Sep 18, 2024

anik120 Sep 19, 2024

anik120 commented Sep 19, 2024 •

edited

Loading

joelanford Sep 19, 2024

⚠ Status condition clean up #379

⚠ Status condition clean up #379

Conversation

anik120 commented Sep 6, 2024 • edited Loading

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

grokspawn commented Sep 6, 2024

ankitathomas left a comment

Choose a reason for hiding this comment

anik120 commented Sep 6, 2024

ankitathomas commented Sep 6, 2024

grokspawn commented Sep 6, 2024

grokspawn commented Sep 6, 2024

everettraven left a comment

Choose a reason for hiding this comment

anik120 commented Sep 18, 2024

anik120 commented Sep 18, 2024

everettraven Sep 18, 2024

Choose a reason for hiding this comment

anik120 Sep 19, 2024

Choose a reason for hiding this comment

anik120 commented Sep 19, 2024 • edited Loading

joelanford Sep 19, 2024

Choose a reason for hiding this comment

anik120 commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

anik120 commented Sep 19, 2024 •

edited

Loading