Skip to content

Commit

Permalink
Merge pull request #659 from njhale/fix/deprecate-channel
Browse files Browse the repository at this point in the history 
Elide channels starting with deprecated bundles
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored May 18, 2021
2 parents 0ae0cd4 + 0147a44 commit 42d8e42
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 102 deletions.
52 changes: 23 additions & 29 deletions pkg/registry/populator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,7 @@ func TestDeprecateBundle(t *testing.T) {
expected expected
}{
{
description: "BundleDeprecated/IgnoreIfNotInIndex",
description: "IgnoreIfNotInIndex",
args: args{
bundles: []string{
"quay.io/test/etcd.0.6.0",
Expand Down Expand Up @@ -776,7 +776,7 @@ func TestDeprecateBundle(t *testing.T) {
},
},
{
description: "BundleDeprecated/SingleChannel",
description: "ChannelRemoved",
args: args{
bundles: []string{
"quay.io/test/prometheus.0.15.0",
Expand All @@ -790,13 +790,11 @@ func TestDeprecateBundle(t *testing.T) {
"quay.io/test/etcd.0.9.0/stable",
"quay.io/test/etcd.0.9.2/stable",
"quay.io/test/etcd.0.9.2/alpha",
"quay.io/test/prometheus.0.15.0/preview",
"quay.io/test/prometheus.0.15.0/stable",
"quay.io/test/prometheus.0.22.2/preview",
"quay.io/test/prometheus.0.15.0/preview",
},
deprecatedBundles: []string{
"quay.io/test/prometheus.0.15.0/preview",
"quay.io/test/prometheus.0.15.0/stable",
},
remainingPkgChannels: pkgChannel{
"etcd": []string{
Expand All @@ -806,35 +804,35 @@ func TestDeprecateBundle(t *testing.T) {
},
"prometheus": []string{
"preview",
"stable",
},
},
},
},
{
description: "BundleDeprecated/ChannelRemoved",
description: "ChannelRemoved/ErrorOnDefault",
args: args{
bundles: []string{
"quay.io/test/etcd.0.9.2",
"quay.io/test/prometheus.0.22.2",
},
},
expected: expected{
err: nil,
err: errors.NewAggregate([]error{fmt.Errorf("error deprecating bundle quay.io/test/prometheus.0.22.2: %s", registry.ErrRemovingDefaultChannelDuringDeprecation)}),
remainingBundles: []string{
"quay.io/test/etcd.0.9.2/alpha",
"quay.io/test/etcd.0.9.0/alpha",
"quay.io/test/etcd.0.9.0/beta",
"quay.io/test/etcd.0.9.0/stable",
"quay.io/test/etcd.0.9.2/stable",
"quay.io/test/etcd.0.9.2/alpha",
"quay.io/test/prometheus.0.22.2/preview",
"quay.io/test/prometheus.0.14.0/preview",
"quay.io/test/prometheus.0.14.0/stable",
"quay.io/test/prometheus.0.15.0/preview",
"quay.io/test/prometheus.0.15.0/stable",
"quay.io/test/prometheus.0.14.0/preview",
"quay.io/test/prometheus.0.14.0/stable",
},
deprecatedBundles: []string{
"quay.io/test/etcd.0.9.2/alpha",
"quay.io/test/etcd.0.9.2/stable",
},
deprecatedBundles: []string{},
remainingPkgChannels: pkgChannel{
"etcd": []string{
"beta",
"alpha",
"stable",
},
Expand All @@ -860,9 +858,7 @@ func TestDeprecateBundle(t *testing.T) {
require.NoError(t, err)

deprecator := sqlite.NewSQLDeprecatorForBundles(store, tt.args.bundles)
err = deprecator.Deprecate()
fmt.Printf("error: %s\n", err)
require.Equal(t, tt.expected.err, err)
require.Equal(t, tt.expected.err, deprecator.Deprecate())

// Ensure remaining bundlePaths in db match
bundles, err := querier.ListBundles(context.Background())
Expand Down Expand Up @@ -938,7 +934,7 @@ func TestAddAfterDeprecate(t *testing.T) {
"prometheus.0.15.0",
},
deprecate: []string{
"quay.io/test/prometheus.0.15.0",
"quay.io/test/prometheus.0.14.0",
},
add: []string{
"prometheus.0.22.2",
Expand All @@ -947,13 +943,15 @@ func TestAddAfterDeprecate(t *testing.T) {
expected: expected{
err: nil,
remaining: []string{
"quay.io/test/prometheus.0.22.2/preview",
"quay.io/test/prometheus.0.15.0/preview",
"quay.io/test/prometheus.0.14.0/preview",
"quay.io/test/prometheus.0.15.0/stable",
"quay.io/test/prometheus.0.22.2/preview",
"quay.io/test/prometheus.0.14.0/stable",
},
deprecated: []string{
"quay.io/test/prometheus.0.15.0/preview",
"quay.io/test/prometheus.0.15.0/stable",
"quay.io/test/prometheus.0.14.0/preview",
"quay.io/test/prometheus.0.14.0/stable",
},
pkgChannels: pkgChannel{
"prometheus": []string{
Expand Down Expand Up @@ -984,18 +982,15 @@ func TestAddAfterDeprecate(t *testing.T) {
expected: expected{
err: nil,
remaining: []string{
"quay.io/test/prometheus.0.15.0/preview",
"quay.io/test/prometheus.0.15.0/stable",
"quay.io/test/prometheus.0.22.2/preview",
"quay.io/test/prometheus.0.15.0/preview",
},
deprecated: []string{
"quay.io/test/prometheus.0.15.0/preview",
"quay.io/test/prometheus.0.15.0/stable",
},
pkgChannels: pkgChannel{
"prometheus": []string{
"preview",
"stable",
},
},
},
Expand Down Expand Up @@ -1045,8 +1040,7 @@ func TestAddAfterDeprecate(t *testing.T) {
require.NoError(t, populate(tt.args.existing, nil))

deprecator := sqlite.NewSQLDeprecatorForBundles(load, tt.args.deprecate)
err = deprecator.Deprecate()
require.Equal(t, tt.expected.err, err)
require.Equal(t, tt.expected.err, deprecator.Deprecate())

require.NoError(t, populate(tt.args.add, tt.args.overwrite))

Expand Down
6 changes: 2 additions & 4 deletions pkg/sqlite/deprecate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,15 @@ func NewSQLDeprecatorForBundles(store registry.Load, bundles []string) *BundleDe

func (d *BundleDeprecator) Deprecate() error {
log := logrus.WithField("bundles", d.bundles)

log.Info("deprecating bundles")

var errs []error

for _, bundlePath := range d.bundles {
if err := d.store.DeprecateBundle(bundlePath); err != nil {
errs = append(errs, fmt.Errorf("error deprecating bundle %s: %s", bundlePath, err))
if !errors.Is(err, registry.ErrBundleImageNotInDatabase) && !errors.Is(err, registry.ErrRemovingDefaultChannelDuringDeprecation) {
return utilerrors.NewAggregate(append(errs, fmt.Errorf("error deprecating bundle %s: %s", bundlePath, err)))
break
}
errs = append(errs, fmt.Errorf("error deprecating bundle %s: %s", bundlePath, err))
}
}

Expand Down
125 changes: 67 additions & 58 deletions pkg/sqlite/load.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,9 +576,16 @@ func (s *sqlLoader) addPackageChannels(tx *sql.Tx, manifest registry.PackageMani
return fmt.Errorf("failed to add package %q: %s", manifest.PackageName, err.Error())
}

var errs []error
hasDefault := false
var (
errs []error
channels []registry.PackageChannel
hasDefault bool
)
for _, c := range manifest.Channels {
if deprecated, err := s.deprecated(tx, c.CurrentCSVName); err != nil || deprecated {
// Elide channels that start with a deprecated bundle
continue
}
if _, err := addChannel.Exec(c.Name, manifest.PackageName, c.CurrentCSVName); err != nil {
errs = append(errs, fmt.Errorf("failed to add channel %q in package %q: %s", c.Name, manifest.PackageName, err.Error()))
continue
Expand All @@ -590,12 +597,13 @@ func (s *sqlLoader) addPackageChannels(tx *sql.Tx, manifest registry.PackageMani
continue
}
}
channels = append(channels, c)
}
if !hasDefault {
errs = append(errs, fmt.Errorf("no default channel specified for %s", manifest.PackageName))
}

for _, c := range manifest.Channels {
for _, c := range channels {
res, err := addChannelEntry.Exec(c.Name, manifest.PackageName, c.CurrentCSVName, 0)
if err != nil {
errs = append(errs, fmt.Errorf("failed to add channel %q in package %q: %s", c.Name, manifest.PackageName, err.Error()))
Expand Down Expand Up @@ -1268,19 +1276,18 @@ func (s *sqlLoader) addBundleProperties(tx *sql.Tx, bundle *registry.Bundle) err
}

func (s *sqlLoader) rmChannelEntry(tx *sql.Tx, csvName string) error {
getEntryID := `SELECT entry_id FROM channel_entry WHERE operatorbundle_name=?`
rows, err := tx.QueryContext(context.TODO(), getEntryID, csvName)
rows, err := tx.Query(`SELECT entry_id FROM channel_entry WHERE operatorbundle_name=?`, csvName)
if err != nil {
return err
}

var entryIDs []int64
for rows.Next() {
var entryID sql.NullInt64
rows.Scan(&entryID)
entryIDs = append(entryIDs, entryID.Int64)
}
err = rows.Close()
if err != nil {
if err := rows.Close(); err != nil {
return err
}

Expand All @@ -1299,35 +1306,50 @@ func (s *sqlLoader) rmChannelEntry(tx *sql.Tx, csvName string) error {
return err
}

stmt, err := tx.Prepare("DELETE FROM channel_entry WHERE operatorbundle_name=?")
_, err = tx.Exec(`DELETE FROM channel WHERE head_operatorbundle_name=?`, csvName)
if err != nil {
return err
}
defer stmt.Close()

if _, err := stmt.Exec(csvName); err != nil {
return err
}

return nil
}

func getTailFromBundle(tx *sql.Tx, name string) (bundles []string, err error) {
func getTailFromBundle(tx *sql.Tx, head string) (bundles []string, err error) {
getReplacesSkips := `SELECT replaces, skips FROM operatorbundle WHERE name=?`
isDefaultChannelHead := `SELECT head_operatorbundle_name FROM channel
INNER JOIN package ON channel.name = package.default_channel
WHERE channel.head_operatorbundle_name = ?`

tail := make(map[string]struct{})
next := name
visited := map[string]struct{}{}
next := []string{head}

for len(next) > 0 {
// Pop the next bundle off of the queue
bundle := next[0]
next = next[1:] // Potentially inefficient queue implementation, but this function is only used when deprecate is called

for next != "" {
rows, err := tx.QueryContext(context.TODO(), getReplacesSkips, next)
// Check if next is the head of the defaultChannel
// If it is, the defaultChannel would be removed -- this is not allowed because we cannot know which channel to promote as the new default
var err error
if row := tx.QueryRow(isDefaultChannelHead, bundle); row != nil {
err = row.Scan(&sql.NullString{})
}
if err == nil {
// A nil error indicates that next is the default channel head
return nil, registry.ErrRemovingDefaultChannelDuringDeprecation
} else if err != sql.ErrNoRows {
return nil, err
}

rows, err := tx.QueryContext(context.TODO(), getReplacesSkips, bundle)
if err != nil {
return nil, err
}
var replaces sql.NullString
var skips sql.NullString

var (
replaces sql.NullString
skips sql.NullString
)
if rows.Next() {
if err := rows.Scan(&replaces, &skips); err != nil {
if nerr := rows.Close(); nerr != nil {
Expand All @@ -1341,49 +1363,32 @@ func getTailFromBundle(tx *sql.Tx, name string) (bundles []string, err error) {
}
if skips.Valid && skips.String != "" {
for _, skip := range strings.Split(skips.String, ",") {
tail[skip] = struct{}{}
if _, ok := visited[skip]; ok {
// We've already visited this bundle's subgraph
continue
}
visited[skip] = struct{}{}
next = append(next, skip)
}
}
if replaces.Valid && replaces.String != "" {
// check if replaces is the head of the defaultChannel
// if it is, the defaultChannel will be removed
// this is not allowed because we cannot know which channel to promote as the new default
rows, err := tx.QueryContext(context.TODO(), isDefaultChannelHead, replaces.String)
if err != nil {
return nil, err
}
if rows.Next() {
var defaultChannelHead sql.NullString
err := rows.Scan(&defaultChannelHead)
if err != nil {
if nerr := rows.Close(); nerr != nil {
return nil, nerr
}
return nil, err
}
if defaultChannelHead.Valid {
if nerr := rows.Close(); nerr != nil {
return nil, nerr
}
return nil, registry.ErrRemovingDefaultChannelDuringDeprecation
}
}
if err := rows.Close(); err != nil {
return nil, err
r := replaces.String
if _, ok := visited[r]; ok {
// We've already visited this bundle's subgraph
continue
}
next = replaces.String
tail[replaces.String] = struct{}{}
} else {
next = ""
visited[r] = struct{}{}
next = append(next, r)
}
}
var allTails []string

for k := range tail {
allTails = append(allTails, k)
// The tail is exactly the set of bundles we visited while traversing the graph from head
var tail []string
for v := range visited {
tail = append(tail, v)
}

return allTails, nil
return tail, nil

}

Expand Down Expand Up @@ -1427,16 +1432,20 @@ func (s *sqlLoader) DeprecateBundle(path string) error {
}

for _, bundle := range tailBundles {
err = s.rmChannelEntry(tx, bundle)
if err != nil {
if err := s.rmChannelEntry(tx, bundle); err != nil {
return err
}
err := s.rmBundle(tx, bundle)
if err != nil {
if err := s.rmBundle(tx, bundle); err != nil {
return err
}
}

// Remove any channels that start with the deprecated bundle
_, err = tx.Exec(fmt.Sprintf(`DELETE FROM channel WHERE head_operatorbundle_name="%s"`, name))
if err != nil {
return err
}

deprecatedValue, err := json.Marshal(registry.DeprecatedProperty{})
if err != nil {
return err
Expand Down
Loading

0 comments on commit 42d8e42

Please sign in to comment.