Skip to content

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

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

Merged
openshift-merge-robot 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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion deploy/chart/templates/0000_50_olm_09-aggregated.clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,8 @@ metadata:
rbac.authorization.k8s.io/aggregate-to-view: "true"
rules:
- apiGroups: ["operators.coreos.com"]
resources: ["clusterserviceversions", "catalogsources", "installplans", "subscriptions", "packagemanifests"]
resources: ["clusterserviceversions", "catalogsources", "installplans", "subscriptions", "operatorgroups"]
verbs: ["get", "list", "watch"]
- apiGroups: ["packages.operators.coreos.com"]
resources: ["packagemanifests"]
verbs: ["get", "list", "watch"]
9 changes: 8 additions & 1 deletion pkg/controller/operators/olm/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Copy Markdown
Member

@njhale njhale Apr 30, 2019

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
Copy Markdown
Member Author

@ecordell ecordell Apr 30, 2019

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
Copy Markdown
Member

@njhale njhale Apr 30, 2019

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
Original file line number Diff line number Diff line change
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
Copy Markdown

@soltysh soltysh Apr 29, 2019

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