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

Add helm mode test coverage for AKS BYOCNI #1578

Merged
merged 2 commits into from
May 11, 2023
Merged

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented May 4, 2023

Fixes: #1566

ended up adding an extra commit to make waitForRelay() more robust: e2e7202 cc @cilium/sig-hubble

Signed-off-by: Michi Mutsuzaki [email protected]

@michi-covalent michi-covalent temporarily deployed to ci May 4, 2023 22:09 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from bcf5605 to ba7312d Compare May 5, 2023 21:14
@michi-covalent michi-covalent temporarily deployed to ci May 5, 2023 21:14 — with GitHub Actions Inactive
@michi-covalent michi-covalent changed the title Use --helm-set to configure AKS installation Use --helm-set to configure AKS BYOCNI installation May 5, 2023
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from ba7312d to 3b4374c Compare May 5, 2023 21:20
@michi-covalent michi-covalent temporarily deployed to ci May 5, 2023 21:20 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from 3b4374c to e6aaf96 Compare May 5, 2023 21:25
@michi-covalent michi-covalent temporarily deployed to ci May 5, 2023 21:26 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from e6aaf96 to d664742 Compare May 5, 2023 21:52
@michi-covalent michi-covalent temporarily deployed to ci May 5, 2023 21:52 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from d664742 to e0d7b5e Compare May 5, 2023 22:18
@michi-covalent michi-covalent temporarily deployed to ci May 5, 2023 22:18 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from e0d7b5e to 9a2f21a Compare May 9, 2023 17:31
@michi-covalent michi-covalent changed the title Use --helm-set to configure AKS BYOCNI installation Add helm mode test coverage for AKS BYOCNI May 9, 2023
@michi-covalent michi-covalent temporarily deployed to ci May 9, 2023 17:33 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from 576fe97 to 8f6074e Compare May 9, 2023 22:20
@michi-covalent michi-covalent temporarily deployed to ci May 9, 2023 22:20 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from 8f6074e to b9a8ae7 Compare May 9, 2023 22:27
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from b9a8ae7 to aa5c95c Compare May 9, 2023 22:29
@michi-covalent michi-covalent temporarily deployed to ci May 9, 2023 22:29 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from aa5c95c to b8428f0 Compare May 9, 2023 23:04
@michi-covalent michi-covalent temporarily deployed to ci May 9, 2023 23:04 — with GitHub Actions Inactive
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from b8428f0 to 36c2421 Compare May 10, 2023 00:01
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from 36c2421 to 6ac85eb Compare May 10, 2023 00:03
@michi-covalent michi-covalent temporarily deployed to ci May 10, 2023 00:03 — with GitHub Actions Inactive
@michi-covalent michi-covalent marked this pull request as ready for review May 10, 2023 00:38
@michi-covalent michi-covalent requested a review from a team as a code owner May 10, 2023 00:38
@michi-covalent michi-covalent requested review from a team as code owners May 10, 2023 00: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.

Relay changes look good to me conceptually. I assume we're seeing the linked issues partially because port-forward is unreliable on AKS?

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Relay check changes LGTM, I like @viktor-kurchenko suggestion about splitting into two functions.

connectivity/check/action.go Outdated Show resolved Hide resolved
- Call ServerStatus with a shorter timeout than what's passed into
  waitForRelay so that we can call it multiple times before the context
  gets cancelled. Sometimes ServerStatus simply gets stuck until the
  context gets cancelled, in which case we end up calling ServerStatus
  only once before giving up.
- Sometimes GetFlows() returns EOF even though we specify the follow
  flag. This seems to  happen when both res.NumUnavailableNodes and
  res.NumConnectedNodes are nil. Add an extra check to make sure that
  Hubble Relay started receiving flows before returning from
  waitForRelay(). Here is a sample failure that was caused by GetFlows()
  returning EOF: https://github.com/cilium/cilium-cli/actions/runs/4928750068/jobs/8812246128#step:15:286

Signed-off-by: Michi Mutsuzaki <[email protected]>
@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 May 11, 2023
@michi-covalent michi-covalent force-pushed the pr/michi/aks-struggle branch from d9c019a to 9cb9a27 Compare May 11, 2023 04:15
@michi-covalent michi-covalent temporarily deployed to ci May 11, 2023 04:15 — with GitHub Actions Inactive
@michi-covalent
Copy link
Contributor Author

removed the test commit. time to 🚀

@michi-covalent michi-covalent merged commit 9d9323d into main May 11, 2023
@michi-covalent michi-covalent deleted the pr/michi/aks-struggle branch May 11, 2023 04:16
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.

Helm mode install doesn't support AKS
5 participants