Verify that a namespace is truly deleted after an API call #32
Conversation
rpalaznik
requested review from
akirillov,
farhan5900,
samvantran,
VladimirKravtsov and
alembiewski
There was a problem hiding this comment.
thanks, @rpalaznik. Looks good with a few nits.
Can you please clarify the fix? The error indicates that an operator instance is deployed to a namespace which is being deleted at that moment. Do we create it before the test? If so, why is it still not available, is it due to API requests queueing?
No, but an instance was never created in this test run, becase First, there is a clean up before installing the operator at this line At the end it calls DropNamespace method which is a simple kubernetes API call But when the API call returns, the namespace can still be in in being terminated state. Which causes re-creation of the namespace to fail at this line The error will be handled here https://github.com/mesosphere/kudo-spark-operator/blob/master/tests/basic_test.go#L62 and the error message will be listed at the end of the test But Hope this helps! |
|
thanks for such a detailed explanation, @rpalaznik. won't we hit the same issue while creating namespaces? shall we add retries to |
I don't think creation would cause a similar issue. Never encountered it so far, and I don't think it's good idea to add timeouts to every API call defensively. We can always do this if the issue arise, but I'd prefer to leave it as is for now. |
| } | ||
|
|
||
| k8sNamespace, err := spark.Clients.CoreV1().Namespaces().Get(spark.Namespace, v1.GetOptions{}) | ||
| if err != nil { | ||
| t.Error(err.Error()) | ||
| t.Fatal(err.Error()) |
There was a problem hiding this comment.
Getting an error during Get.NameSpace is considered fatal?
There was a problem hiding this comment.
This is just to stop test execution and prevent logging a message about successful operator installation to be printed, since it's may be not true depending on the error.
tests/utils/k8s.go
Outdated
| return retry(namespaceDeletionTimeout, namespaceDeletionCheckInterval, func() error { | ||
| _, err := clientSet.CoreV1().Namespaces().Get(name, metav1.GetOptions{}) | ||
| if err == nil { | ||
| return errors.New(fmt.Sprintf("Namespace %s is still there", name)) |
There was a problem hiding this comment.
| return errors.New(fmt.Sprintf("Namespace %s is still there", name)) | |
| return errors.New(fmt.Sprintf("Namespace '%s' still exists. Retrying delete of namespace...", name)) |
There was a problem hiding this comment.
Ok, but I dropped 'Retrying ...' part since it's not actually do that, just waits for the namespace to disappear completely.
There was a problem hiding this comment.
Ah good point.. Sorry my suggestion was incorrect then but you've already merged. It should be fine but maybe in a later fix update wording to match more correctly. Sorry about that!
tests/utils/k8s.go
Outdated
| } else if statusErr, ok := err.(*apiErrors.StatusError); !ok || statusErr.Status().Reason != metav1.StatusReasonNotFound { | ||
| return err | ||
| } else { | ||
| log.Info(fmt.Sprintf("Namespace %s is deleted", name)) |
There was a problem hiding this comment.
| log.Info(fmt.Sprintf("Namespace %s is deleted", name)) | |
| log.Info(fmt.Sprintf("Namespace '%s' successfully deleted", name)) |
What changes were proposed in this pull request and why are they needed?
Resolves integration test issues when a namespace is not immediately deleted after an API call, resulting in errors like this in further tests:
How were the changes tested?
With
make cluster-createandmake test