Bug 1732579: Best-effort loading #69

njhale · 2019-08-06T21:44:44Z

This PR represents a more concise attempt to make appregistry-server more permissive of errors by allowing more operations to be best-effort; allowing loading to progress while aggregating errors (instead of stopping immediately). In lieu of simple logging, error aggregation is necessary to preserve the strict behavior of the configmap-server and initializer commands since appregistry-server uses both underlying load methods (configmap and directory) in various scenarios.

openshift-ci-robot · 2019-08-06T21:57:02Z

@njhale: This pull request references an invalid Bugzilla bug:

expected dependent Bugzilla bug to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is POST 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 1737586: WIP: Best-effort loading

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.

ecordell · 2019-08-06T21:57:43Z

/bugzilla refresh

openshift-ci-robot · 2019-08-06T21:57:45Z

@ecordell: This pull request references an invalid Bugzilla bug:

expected dependent Bugzilla bug to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is POST 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:

/bugzilla refresh

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.

ecordell

Looks good! A couple of foods for thought

cmd/appregistry-server/main.go

pkg/sqlite/directory.go

ecordell · 2019-08-08T15:53:57Z

pkg/sqlite/load.go

 			}

 			if err := s.addProvidedAPIs(tx, channelEntryCSV, currentID); err != nil {
-				return err
+				errs = append(errs, err)
+				break


We should be able to continue processing even if the provided apis can't be added. It just means the registry won't be able to answer dependency questions about this particular bundle.

ecordell · 2019-08-08T15:54:46Z

pkg/sqlite/load.go

 			}

 			skips, err := channelEntryCSV.GetSkips()
 			if err != nil {
-				return err
+				errs = append(errs, err)
+				break


We should be able to continue processing even if we can't find the skips

openshift-ci-robot · 2019-08-08T16:16:24Z

@njhale: This pull request references a valid Bugzilla bug. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1732579: Best-effort loading

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.

ecordell · 2019-08-08T20:00:57Z

/lgtm

Support `oc` command

Make package loading best-effort by aggregating errors and continuing where applicable.

Make package and bundle loading best effort by aggregating errors and continuing where applicable.

shawn-hurley · 2019-08-08T20:59:57Z

pkg/sqlite/load.go

 			}

 			if replaces == "" {
 				// we've walked the channel until there was no replacement
 				break
 			}

-			replaced, err := s.getCSV(tx, replaces)
+			replacedChannelEntry, err := addChannelEntry.Exec(c.Name, manifest.PackageName, replaces, depth)


I would like to understand why the order of this changed?

The change in ordering lets us add a channel entry for a "missing" replaced CSV.

shawn-hurley · 2019-08-08T21:06:00Z

cmd/appregistry-server/main.go

+		logger.WithError(err).Warn("error loading app registry manifests")
+	}
+	if store == nil {
+		logger.Fatal("store failed to initialize")


If there are no bundles that are valid this will crash loop correct? Wondering if it makes sense to expose an empty catalog instead?

If there are no bundles, the store will not be nil. It should only be nil when there is an issue parsing the source and package names given in the command options. That signifies a misconfiguration of operator-registry itself, which I think should be fatal. I do think that option validation should be more explicit and done before Load is called, but the appregistry code should be going away soon anyway, and the objective was to make this PR slim.

All that being said, my previous PR exposes an "Empty" Query implementation. If you think it's best I can pull in that into this PR too.

I added the empty querier just to be safe.

njhale · 2019-08-08T22:25:25Z

/retest

ecordell

/lgtm

ecordell · 2019-08-08T16:37:56Z

pkg/sqlite/directory_test.go

 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v2"


Can we either:

Do this with the currently imported yaml library (ghodss) or the kube decoders

Remove the currently imported libraries (ghodss) and replace with this one?

At a cursory glance, there was no encoder to match the decoder in the k8s package.

I didn't look into the ghodss package. Will follow-up soon.

ecordell · 2019-08-08T22:52:26Z

pkg/sqlite/load.go

 		}
 	}
-	return tx.Commit()
+
+	if err := tx.Commit(); err != nil {


We should revisit this, and probably have more granular transactions. This one failing would nullify a lot of the work above to load as much as possible.

ecordell · 2019-08-08T23:01:25Z

/lgtm

openshift-ci-robot · 2019-08-08T23:01:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [ecordell,njhale]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

njhale · 2019-08-14T20:06:00Z

/cherry-pick release-4.1

openshift-cherrypick-robot · 2019-08-14T20:06:05Z

@njhale: #69 failed to apply on top of branch "release-4.1":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/sqlite/load.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/sqlite/load.go
CONFLICT (content): Merge conflict in pkg/sqlite/load.go
Patch failed at 0001 feat(sqlite): make package loading best-effort

In response to this:

/cherry-pick release-4.1

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-robot added the do-not-merge/work-in-progress label Aug 6, 2019

openshift-ci-robot requested review from dinhxuanvu and jpeeler August 6, 2019 21:44

openshift-ci-robot added approved size/L labels Aug 6, 2019

ecordell changed the title ~~WIP: Best-effort loading~~ Bug 1737586 WIP: Best-effort loading Aug 6, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress label Aug 6, 2019

ecordell changed the title ~~Bug 1737586 WIP: Best-effort loading~~ Bug 1737586: WIP: Best-effort loading Aug 6, 2019

openshift-ci-robot added the bugzilla/invalid-bug label Aug 6, 2019

njhale force-pushed the best-effort branch from d0b8aaf to c139d80 Compare August 7, 2019 14:07

njhale changed the title ~~Bug 1737586: WIP: Best-effort loading~~ Bug 1737586: Best-effort loading Aug 7, 2019

ecordell reviewed Aug 8, 2019

View reviewed changes

ecordell changed the title ~~Bug 1737586: Best-effort loading~~ Bug 1732579: Best-effort loading Aug 8, 2019

openshift-ci-robot added bugzilla/valid-bug and removed bugzilla/invalid-bug labels Aug 8, 2019

openshift-ci-robot added size/XL and removed size/L labels Aug 8, 2019

njhale force-pushed the best-effort branch from 6a2e5a1 to 4d2d23f Compare August 8, 2019 19:23

openshift-ci-robot assigned ecordell Aug 8, 2019

openshift-ci-robot added the lgtm label Aug 8, 2019

openshift-merge-robot and others added 4 commits August 8, 2019 16:03

Merge pull request operator-framework#65 from ecordell/catalog-cmd

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

d6c69c4

Support `oc` command

feat(sqlite): make package loading best-effort

6a8366a

Make package loading best-effort by aggregating errors and continuing where applicable.

feat(sqlite): make directory loading best-effort

065e51f

Make package and bundle loading best effort by aggregating errors and continuing where applicable.

feat(sqlite): make configmap loading best-effort

56265bf

Make package and bundle loading best effort by aggregating errors and continuing where applicable.

njhale force-pushed the best-effort branch from 4d2d23f to 56265bf Compare August 8, 2019 20:38

openshift-ci-robot removed the lgtm label Aug 8, 2019

shawn-hurley reviewed Aug 8, 2019

View reviewed changes

ecordell approved these changes Aug 8, 2019

View reviewed changes

openshift-ci-robot added the lgtm label Aug 8, 2019

feat(appregistry-server): use empty querier on fatal load error

9d997b8

openshift-ci-robot removed the lgtm label Aug 8, 2019

openshift-ci-robot added the lgtm label Aug 8, 2019

openshift-merge-robot merged commit 7b704fe into operator-framework:master Aug 9, 2019

njhale deleted the best-effort branch August 9, 2019 11:23

njhale restored the best-effort branch August 9, 2019 20:04

njhale mentioned this pull request Aug 9, 2019

Revert "Bug 1732579: Best-effort loading" #71

Closed

njhale mentioned this pull request Aug 19, 2019

[release-4.1] Bug 1732580: Backport best-effort #79

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1732579: Best-effort loading #69

Bug 1732579: Best-effort loading #69

njhale commented Aug 6, 2019

openshift-ci-robot commented Aug 6, 2019

ecordell commented Aug 6, 2019

openshift-ci-robot commented Aug 6, 2019

ecordell left a comment

ecordell Aug 8, 2019

ecordell Aug 8, 2019

openshift-ci-robot commented Aug 8, 2019

ecordell commented Aug 8, 2019

shawn-hurley Aug 8, 2019

njhale Aug 8, 2019

shawn-hurley Aug 8, 2019

njhale Aug 8, 2019 •

edited

Loading

njhale Aug 8, 2019

njhale commented Aug 8, 2019

ecordell left a comment

ecordell Aug 8, 2019

njhale Aug 8, 2019

ecordell Aug 8, 2019

ecordell commented Aug 8, 2019

openshift-ci-robot commented Aug 8, 2019

njhale commented Aug 14, 2019

openshift-cherrypick-robot commented Aug 14, 2019

Bug 1732579: Best-effort loading #69

Bug 1732579: Best-effort loading #69

Conversation

njhale commented Aug 6, 2019

openshift-ci-robot commented Aug 6, 2019

ecordell commented Aug 6, 2019

openshift-ci-robot commented Aug 6, 2019

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 8, 2019

ecordell commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale commented Aug 8, 2019

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell commented Aug 8, 2019

openshift-ci-robot commented Aug 8, 2019

njhale commented Aug 14, 2019

openshift-cherrypick-robot commented Aug 14, 2019

njhale Aug 8, 2019 •

edited

Loading