Remove resources when terminating the environment#519
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
approved
knative-prow
bot
added
the
size/M
| @@ -200,12 +200,25 @@ func (f *Feature) References() []corev1.ObjectReference { | |||
| // | |||
| // Expected to be used as a StepFn. | |||
| func (f *Feature) DeleteResources(ctx context.Context, t T) { | |||
There was a problem hiding this comment.
This function still has the comment:
// It doesn't fail when a referenced resource couldn't be deleted.
// Use References to get the undeleted resources.
We're now changing the behavior. When some resource couldn't be deleted we're failing the test. Is this intentional?
A few lines below we still assign this but it will be probably unused because we now fail the test with t.Fatal immediately:
f.refs = refFailedDeletion
There was a problem hiding this comment.
weren't we failing even without this patch?
err := wait.Poll(time.Second, 4*time.Minute, func() (bool, error) { // ... }
if err != nil {
LogReferences(refFailedDeletion...)(ctx, t)
t.Fatalf("failed to wait for resources to be deleted: %v", err)
}There was a problem hiding this comment.
Hmm. OK. So this condition was broken earlier. And the comment is not valid for a long time. So, now I wonder why f.refs = refFailedDeletion is still there (probably a bug).
Let's remove the invalid comment for now. Not sure what to do about the refs.
pkg/feature/feature.go
Outdated
| defer f.refsMu.Unlock() | ||
|
|
||
| f.refs = refFailedDeletion | ||
| return refFailedDeletion, nil |
There was a problem hiding this comment.
Can refFailedDeletion ever be non-nil here? (whenever refFailedDeletion is assigned, an error is returned anyway?)
There was a problem hiding this comment.
Yes, that's correct and an existing problem, I'll drop it
knative-prow-robot
added
the
needs-rebase
When calling env.Finish() the resources that have been created by such environment should be removed. So far, we were relying on the namespace deletion to clean up resources, however, if a precreated namespace is used for multiple tests and multiple environments, the cluster is full of resources that are not needed anymore since those tests were successfully run. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
pierDipi
force-pushed
the
remove-resources-when-environment-is-terminated
branch
from
a7f0e65 to
8414e88
Compare
knative-prow-robot
removed
the
needs-rebase
|
/lgtm Thanks! |
|
/cherry-pick release-1.10 |
|
/cherry-pick release-1.9 |
|
@pierDipi: new pull request created: #531 DetailsIn 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. |
|
@pierDipi: #519 failed to apply on top of branch "release-1.9": DetailsIn 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. |
|
/cherry-pick release-1.9 |
…#519) * Remove resources when terminating the environment When calling env.Finish() the resources that have been created by such environment should be removed. So far, we were relying on the namespace deletion to clean up resources, however, if a precreated namespace is used for multiple tests and multiple environments, the cluster is full of resources that are not needed anymore since those tests were successfully run. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Remove refFailedDeletion since it's useless Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Remove wrong comment Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
|
@pierDipi: new pull request created: #532 DetailsIn 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. |
pierDipi
restored the
remove-resources-when-environment-is-terminated
branch
4 participants
When calling env.Finish() the resources that have been created by such environment should be removed.
So far, we were relying on the namespace deletion to clean up resources, however, if a precreated namespace is used for multiple tests and multiple environments, the cluster is full of resources that are not needed anymore since those tests were successfully run.