connectivity: add retry to external targets

[brlbil]

Adds --retry and --retry-delay flags to the connectivity test.
They are used in external pod-to-world and po-to-cidr tests to try to reconnect
on failures thus making flaky tests less likely.

[squeed]

Hmm, it seems like the retries for cases where failure is expected are causing the tests to take too long.

[squeed]

Is there an easy way to disable retries when the test is expected to fail?

[brlbil]

The default retry policy is 3 times and 3s delay. We can reduce this number, but first I will check the test cases for expected fails and see if we approach them differently.

[brlbil]

Is there an easy way to disable retries when the test is expected to fail?

@squeed I have made changes that retry options are added for commands in which successful result is expected.
Also, the second commit fixes a couple of comments.

[tklauser]

Also, the second commit fixes a couple of comments.

Could you please squash the two commits? It looks like the 2nd commit only changes comments that got introduced/changed in the 1st commit.

[tklauser]

It feels a bit strange this this is essentially all OR destIP matches OR (desPort matches AND label matches). Though it currently doesn't have a practical consequence, I'd say it's more straight forward to apply the options if either all is true OR all specified options are true, i.e. the AND of all specified options. Something like:

func (rc *retryCondition) CurlOptions(peer check.TestPeer, ipFam check.IPFamily, pod check.Pod, params check.Parameters) []string {                                                                                                                
        if params.Retry == 0 { 
                return []string{}
        } 

        opts := []string{
                "--retry", strconv.FormatInt(int64(connectRetry), 10),
                "--retry-all-errors", // add --retry-all-errors to retry on all possible errors
        } 

        if retryDelay := params.RetryDelay.Seconds(); retryDelay > 0.0 { 
                opts = append(opts, "--retry-delay", strconv.FormatFloat(retryDelay, 'f', -1, 64))
        } 
        if rc.all { 
                return opts
        } 
        if rc.destIP != "" && peer.Address(ipFam) != rc.destIP { 
                return []string{}
        } 
        if rc.destPort != 0 && peer.Port() != rc.destPort { 
                return []string{}
        } 
        for n, v := range rc.podLabels { 
                if !pod.HasLabel(n, v) { 
                        return []string{}
                } 
        } 

        return opts
}

That way it's independent of the test cases these options are currently used with and allows for easier extensibility in the future.

What do you think?

[brlbil]

This implementation is much more elegant 👍 but it is missing one thing if no options are set it still sets the retry options. So I added a check earlier to return in this case

	if !rc.all && rc.destIP == "" && rc.destPort == 0 {
		return []string{}
	}

I am assuming this change is okay with you and pushing the changes 😃

[tklauser]

but it is missing one thing if no options are set it still sets the retry options. So I added a check earlier to return in this case

Ah, good point. I missed that. Shouldn't we also consider pod labels in the options not being set (ref. my inline comment)?

[brlbil]

I am guessing the failing test is related to #1527