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

test(e2e): wait for deployment to exist in csv replacement test #833

Merged
merged 3 commits into from May 1, 2019
Merged

test(e2e): wait for deployment to exist in csv replacement test #833

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
819cc9c
test(e2e): wait for deployment to exist in csv replacement test
ecordell Apr 29, 2019
c334aaa
fix(olm): don't requeue operatorgroups for deleted copied csvs
ecordell Apr 30, 2019
a6eb04b
fix(permissions): the aggregated edit role should grant view access
ecordell Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion pkg/controller/operators/olm/operator.go
View file
Open in desktop
Expand Up @@ -396,7 +396,14 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {

defer func(csv v1alpha1.ClusterServiceVersion) {
logger.Debug("removing csv from queue set")
a.csvQueueSet.Remove(csv.GetName(), csv.GetNamespace())
if err := a.csvQueueSet.Remove(csv.GetName(), csv.GetNamespace()); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Nice! It seems like it was missing from the original, but aren't CSV keys in other queues as well (e.g. csvCopyQueue and csvGCQueue)? Should we handle removing those as well? Could we wrap the delete handlers in our queue abstractions to have them automatically dropped after they return?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are good ideas for a follow up :)

I was going to add it here but we don't even expose the other queues outside of their indexers, so that would be a bigger change

logger.WithError(err).Debug("error removing from queue")
}

if clusterServiceVersion.IsCopied() {
Copy link
Member

Choose a reason for hiding this comment

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

This looks good. One structural thing is that we could close over a var copied *bool initialized to false, set it below when we check clusterServiceVersion.IsCopied(), and use that here to avoid checking again (not that it's very resource intensive).

logger.Debug("deleted csv is copied. skipping operatorgroup requeue")
return
}

// Requeue all OperatorGroups in the namespace
logger.Debug("requeueing operatorgroups in namespace")
Expand Down
17 changes: 17 additions & 0 deletions test/e2e/csv_e2e_test.go
View file
Open in desktop
Expand Up @@ -256,6 +256,19 @@ func awaitCSV(t *testing.T, c versioned.Interface, namespace, name string, check
return fetched, err
}

func waitForDeployment(t *testing.T, c operatorclient.ClientInterface, name string) error {
return wait.Poll(pollInterval, pollDuration, func() (bool, error) {
_, err := c.GetDeployment(testNamespace, name)
if err != nil {
if k8serrors.IsNotFound(err) {
return false, nil
}
return false, err
}
return true, nil
})
}

func waitForDeploymentToDelete(t *testing.T, c operatorclient.ClientInterface, name string) error {
return wait.Poll(pollInterval, pollDuration, func() (bool, error) {
t.Logf("waiting for deployment %s to delete", name)
Expand Down Expand Up @@ -2687,6 +2700,10 @@ func TestUpdateCSVModifyDeploymentName(t *testing.T) {
_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Update(fetchedCSV)
require.NoError(t, err)

// Wait for new deployment to exist
err = waitForDeployment(t, c, strategyNew.DeploymentSpecs[0].Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you're missing require.NoError(t, err) below it.

require.NoError(t, err)

// Wait for updated CSV to succeed
_, err = fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker)
require.NoError(t, err)
Expand Down