test: skip image polling test if running in a kind cluster #2425
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dinhxuanvu, 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
bot
added
the
approved
|
/lgtm |
|
I assumed the name of the kind control plane node to be static across clusters but it can be different depending on the name of the cluster. |
test/e2e/util_test.go
Outdated
| _, err := client.KubernetesInterface().CoreV1().Nodes().Get(context.TODO(), "kind-control-plane", metav1.GetOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return false, nil | ||
| } | ||
| // error finding node | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
@exdx Following up your comment in this PR mentioning that we can't rely on a static control plane node name - can we instead just list out the nodes, and check whether any node is prefixed with kind-*? We could also try something like the following:
// fire off a list command
nodes, err := ...
if err != nil {
...
}
if len(nodes) == 0 {
...
}
var isKindCluster bool
for _, node := range nodes.Items {
if !strings.HasPrefix(node.GetName(), "kind-") {
continue
}
if !strings.HasSuffix(node.GetName(), "-control-plane") {
continue
}
isKindCluster = true
}
return isKindCluster, nilVery loosely created that inline, so YMMV, but that should let us avoid needing to use any regex?
There was a problem hiding this comment.
(I realize you can cleanup that implementation further, but the general idea around checking whether the node has kind-*-control-plane present could work?)
There was a problem hiding this comment.
I think that this is a good approach and gets the test to skip if running on just about any kind cluster.
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/lgtm cancel |
The name of the kind cluster is generated by the e2e test - couldn't that name be threaded into the test? |
That's an alternate approach -- it would prevent the test from running when using the make directive on the local kind cluster. I like to deploy OLM and run tests via ginkgo directly, in which case the name of the kind node is just the default |
Signed-off-by: Daniel Sover <[email protected]>
| if err != nil { | ||
| // error finding nodes | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
Might be easier to just default to false, and avoid returning an error entirely, if we run into an error while listing out the nodes. This would allow call sites to avoid needing to check whether there's an error (and improve readability decreasing the number of conditional chains). Thoughts?
There was a problem hiding this comment.
Makes sense, I had it that way originally, but a failure listing the nodes is an error. I think it should be treated as one, even if it makes the function more complex
| for _, node := range nodes.Items { | ||
| if !strings.HasPrefix(node.GetName(), "kind-") { | ||
| continue | ||
| } | ||
| if !strings.HasSuffix(node.GetName(), "-control-plane") { | ||
| continue | ||
| } | ||
| return true, nil | ||
| } | ||
| return false, nil |
There was a problem hiding this comment.
👍 Testing this logic out locally and it seemed to catch the default kind node name and any other variations, like `kind-123432-control-plane, and correctly filtered out cluster environment we don't care about in the context of this function.
|
/lgtm |
| @@ -918,3 +918,21 @@ func HaveMessage(goal string) gtypes.GomegaMatcher { | |||
| return plan.Status.Message | |||
| }, ContainSubstring(goal)) | |||
| } | |||
|
|
|||
| func inKind(client operatorclient.ClientInterface) (bool, error) { | |||
| nodes, err := client.KubernetesInterface().CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{}) | |||
There was a problem hiding this comment.
We may want to also use context.Background() instead of context.TODO() here.
There was a problem hiding this comment.
Yes, I wasn't sure about whether to thread a context through -- might be ok leaving as is for now
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
Holding so the bot doesn't go crazy. /hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Closing as #2445 superseded this PR as there's merge conflicts and this is a real nice QoL improvement for running the e2e suite locally so I wanted to push it through. Thanks for tackling this work 🎉 |
Signed-off-by: Daniel Sover [email protected]
Description of the change:
This change skips the image polling test when running with a kind cluster. Previously this test was skipped on upstream CI only (which is also kind), but not locally, which caused the test to fail when running locally on a kind cluster.
The reason for the failure seems to be the context for the
kubectl port-forwardcommand to work is not properly configured in the test. It's worth exploring potentially trying to getting the right context at test execution time and seeing if the test works correctly, as it does on minikube and openshift.Motivation for the change:
Closes #2420
Reviewer Checklist
/doc