Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connectivity: Report IP families that are being tested #1440

Conversation

Neilblaze
Copy link
Contributor

The objective is to improve the connectivity reporting by displaying which IP families (i.e., v4 or v6) are being tested.
Additionally, each Action description should contain the family too. E.g., we could report curl-v4-1, curl-v6-2, etc.

Fixes #1410
cc: @brb, @gandro
Signed-off-by: Pratyay Banerjee

@Neilblaze Neilblaze requested a review from a team as a code owner March 7, 2023 15:57
@Neilblaze Neilblaze requested a review from nathanjsweet March 7, 2023 15:57
@Neilblaze Neilblaze marked this pull request as draft March 7, 2023 16:03
@Neilblaze
Copy link
Contributor Author

@brb Can you please provide some feedback on where I'm going wrong? 😅

Even one of the error log state this? → could not import github.com/cilium/cilium-cli/connectivity/tests


var ipFamStr string

t.ForEachIPFamily(func(ipFam check.IPFamily) {
Copy link
Member

@gandro gandro Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! You have a syntax error here. This func here is never closed, hence all the errors you see.

I'd recommend testing the code out locally too, that will make it easier to spot defects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah silly 😭
Thank you so much @gandro for the catch! Because of the fact that I work on two different PC's (Ubuntu 22.04 LTS + Windows 10), the mistake happened while I was shifting the code from one system to another. 😅

And thanks for the advice, I'll make sure to test the code out locally too to avoid such silly mistakes. 😄

Copy link
Contributor Author

@Neilblaze Neilblaze Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @gandro this → github.com/cilium/cilium-cli/connectivity/check is throwing 404. Additionally, how can I check the IP-class then? (refer to this)

Should I have to use this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proper import path should be "github.com/cilium/cilium-cli/connectivity/check". This is a Go import path and therefore not a valid GitHub URL. The GitHub URL for that file would be https://github.com/cilium/cilium-cli/tree/master/connectivity/check

Should I have to use this?

Yes, that's the one to use.

@maintainer-s-little-helper
Copy link

Commit c6e9d77 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper
Copy link

Commit 2fbe9aa does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper
Copy link

Commit 2fbe9aa does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper
Copy link

Commit 2fbe9aa does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@Neilblaze Neilblaze temporarily deployed to ci March 11, 2023 13:40 — with GitHub Actions Inactive
@Neilblaze Neilblaze force-pushed the connectivity-Report-which-IP-families-are-tested branch from b7be3cc to a248a04 Compare March 11, 2023 13:45
@Neilblaze Neilblaze temporarily deployed to ci March 11, 2023 13:45 — with GitHub Actions Inactive
@Neilblaze
Copy link
Contributor Author

Neilblaze commented Mar 11, 2023

@gandro I think it's fine now. Phew!
The implemented code could be optimized using the ternary operator. I tried this:

t.ForEachIPFamily(func(ipFam check.IPFamily) {
    ipFamStr = ipFam == check.IPFamilyV4 ? "v4" : "v6"
// ...

instead of

t.ForEachIPFamily(func(ipFam check.IPFamily) {
    if ipFam == check.IPFamilyV4 {
        ipFamStr = "v4"
} else if ipFam == check.IPFamilyV6 {
    ipFamStr = "v6"
}
// ...

It should work, but apparently, I was getting an error log stating "?" as illegal character. On doing some research, I found this on StackOverflow. Hence, I proceeded with the previous one. Also, instead of performing two checks, we can simply check whether it's ipv4 or not, else it should be ipv6.

If the above one sounds fine to you, then this can be implemented:

t.ForEachIPFamily(func(ipFam check.IPFamily) {
    if ipFam == check.IPFamilyV4 {
        ipFamStr = "v4"
    } else {
        ipFamStr = "v6"
    }
// ...

Once you confirm, then I'll push the final changes & squash it.
Refer to this. And apologies for creating this simple thing a mess. 😅

@Neilblaze Neilblaze force-pushed the connectivity-Report-which-IP-families-are-tested branch from c19ffc0 to 9029d0c Compare March 16, 2023 08:21
@maintainer-s-little-helper
Copy link

Commit 555bfaf does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@Neilblaze Neilblaze temporarily deployed to ci March 16, 2023 08:32 — with GitHub Actions Inactive
@Neilblaze Neilblaze force-pushed the connectivity-Report-which-IP-families-are-tested branch from 555bfaf to acebe27 Compare March 16, 2023 09:16
@Neilblaze Neilblaze temporarily deployed to ci March 16, 2023 09:16 — with GitHub Actions Inactive
@Neilblaze Neilblaze requested review from gandro and removed request for nathanjsweet March 16, 2023 09:21
@Neilblaze
Copy link
Contributor Author

Neilblaze commented Mar 16, 2023

@gandro All the tests are were passing (including EKS(ENI), GKE & Multicluster). But after I squashed all the commits together (after adding the DCO), those two are specifically failing. Could be a flake, would you mind re-triggering this job again? And if everything checks out, please merge :)

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. Yes, those look like unrelated flakes (it seems the github container registry has been struggling since yesterday, which is used for the actions too).

But before we validate CI, I think there are still a few things to address in the code. I've left some inline feedback.

In addition, there are more usages of ForEachIPFamily to patch (I recommend searchg for ForEachIPFamily in your IDE to make sure you get them all):

t.ForEachIPFamily(func(ipFam check.IPFamily) {

t.ForEachIPFamily(func(ipFam check.IPFamily) {

t.NewAction(s, "curl", client, server, ipFam).Run(func(a *check.Action) {

t.NewAction(s, "ping", client, server, ipFam).Run(func(a *check.Action) {

connectivity/tests/host.go Outdated Show resolved Hide resolved
connectivity/tests/pod.go Outdated Show resolved Hide resolved
connectivity/tests/service.go Outdated Show resolved Hide resolved
@Neilblaze Neilblaze force-pushed the connectivity-Report-which-IP-families-are-tested branch from d9db1fc to 844ac98 Compare March 17, 2023 17:05
@Neilblaze Neilblaze temporarily deployed to ci March 17, 2023 17:05 — with GitHub Actions Inactive
@Neilblaze
Copy link
Contributor Author

@gandro I underestimated how simple it was, and as a result, I delved deeply into every aspect, leading me into all this unnecessary trouble lol. Hopefully this time it should be fine.

@Neilblaze Neilblaze requested a review from gandro March 17, 2023 17:38
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yes, this looks good to me know. Let's run it through CI.

@gandro
Copy link
Member

gandro commented Mar 20, 2023

GKE failed connecting to google.com - possibly due to external factors. Restarting

https://github.com/cilium/cilium-cli/actions/runs/4449884381/jobs/7814654455?pr=1440

@gandro gandro requested a review from nathanjsweet March 20, 2023 12:17
@gandro
Copy link
Member

gandro commented Mar 20, 2023

Re-adding the required review from the CLI codeowners. Please do not remove automatically assigned code-owners, since not every reviewer (like myself) belongs to all codeowner teams.

@Neilblaze
Copy link
Contributor Author

Please do not remove automatically assigned code-owners, since not every reviewer (like myself) belongs to all codeowner teams.

@gandro Sorry about the confusion, but as far as I can remember, I'm pretty sure I didn't unassign anyone 😅, but it seems like GitHub auto-removes any other reviewers when we select this 👇🏼
image

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 20, 2023
@gandro
Copy link
Member

gandro commented Mar 20, 2023

Please do not remove automatically assigned code-owners, since not every reviewer (like myself) belongs to all codeowner teams.

@gandro Sorry about the confusion, but as far as I can remember, I'm pretty sure I didn't unassign anyone sweat_smile, but it seems like GitHub auto-removes any other reviewers when we select this 👇🏼

Oh, interesting, thanks for the clarification!

--

This PR is ready to be merged.

@tklauser tklauser merged commit 2ac56fc into cilium:master Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connectivity: Report which IP families are tested
4 participants