Bug 1833195: delete bundle objects after CSV gets deleted #1516
Conversation
|
@exdx: This pull request references Bugzilla bug 1833195, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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
bugzilla/severity-medium
|
/retest |
1 similar comment
|
/retest |
|
Unit tests are failing. |
didn't check things again after rebasing off master 🤦 |
exdx
force-pushed
the
fix/bug-1833195
branch
2 times, most recently
from
f58008c
to
362fcff
Compare
There was a problem hiding this comment.
Can you add tests for upgrades?
What happens if:
- I have a configmap in version 1 and an updated version of the same configmap in version 2
- I have a configmap in version 1 and a different (name) configmap in version 2
(the old should be deleted and the new should be created in both cases, but we should have tests to verify the behavior).
We should improve our docs here as well - unlike "managed" resources (deployment, rbac created from permissions block), we don't directly update / pivot ownerrefs for these "unmanaged" resources. That could be a tripping point for some.
exdx
force-pushed
the
fix/bug-1833195
branch
2 times, most recently
from
84f6ca2
to
aadf21a
Compare
|
Added two additional e2e tests for the 2 upgrade scenarios @ecordell mentioned. Also added the ownership code to service objects in the bundle as well for consistency across all bundle objects OLM resolves. |
|
/retest |
|
Tests should pass once Quay is back in a healthy state |
|
/retest |
|
/lgtm |
| Eventually(func() error { | ||
| _, err = fetchSubscription(operatorClient, testNamespace, subName, subscriptionStateAtLatestChecker) | ||
| return err | ||
| }).Should(BeNil()) |
There was a problem hiding this comment.
Nit, but the error matchers, like Succeed, would also report if the actual value doesn't have an error type. The message in the test output will be a bit nicer, too.
| Expect(err).ToNot(HaveOccurred(), "could not create catalog source") | ||
|
|
||
| // Create a Subscription for package | ||
| _ = createSubscriptionForCatalog(operatorClient, source.GetNamespace(), subName, source.GetName(), packageName, channelName, "", v1alpha1.ApprovalAutomatic) |
There was a problem hiding this comment.
Does the cleanup func need to be called in an AfterEach if it's non-nil?
| Eventually(func() bool { | ||
| cfg, err := kubeClient.GetConfigMap(testNamespace, configmapName) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| // check data in configmap to ensure it is the new data (configmap was updated in the newer bundle) | ||
| // new value in the configmap is "updated-very-much" | ||
| data := cfg.Data["special.how"] | ||
| return data == "updated-very-much" | ||
| }).Should(BeTrue()) |
There was a problem hiding this comment.
I think both Expect and Eventually/Consistently can handle multiple return values and implicitly fail if anything after the first return value is non-nil, which would let you write this:
| Eventually(func() bool { | |
| cfg, err := kubeClient.GetConfigMap(testNamespace, configmapName) | |
| if err != nil { | |
| return false | |
| } | |
| // check data in configmap to ensure it is the new data (configmap was updated in the newer bundle) | |
| // new value in the configmap is "updated-very-much" | |
| data := cfg.Data["special.how"] | |
| return data == "updated-very-much" | |
| }).Should(BeTrue()) | |
| Eventually(func() (string, error) { | |
| cfg, err := kubeClient.GetConfigMap(testNamespace, configmapName) | |
| if err != nil { | |
| return "", err | |
| } | |
| return cfg.Data["special.how"], nil | |
| }).Should(Equal("updated-very-much")) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene, benluddy, exdx 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@exdx: All pull requests linked via external trackers have merged: operator-framework/operator-lifecycle-manager#1516. Bugzilla bug 1833195 has been moved to the MODIFIED state. In response to this:
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. |
Description of the change:
OLM now supports secrets and configmaps in the bundle. These should have ownerrefs associated with the CSV in the bundle. When the CSV is deleted, these objects should be deleted as well. Since they are namespaced scoped, the kube gc will delete them automatically.
Motivation for the change:
Reviewer Checklist
/docs