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.

Sign up for GitHub

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

Jump to bottom

CI: update cilium-cli to v0.9.2 #17706

Merged
merged 1 commit into from Oct 29, 2021
Merged

CI: update cilium-cli to v0.9.2 #17706

merged 1 commit into from Oct 29, 2021

Conversation

tklauser
Copy link
Member

cilium hubble enable now correctly waits for Cilium status to be
ready. Thus, the extra cilium status --wait after enabling Hubble can
be avoided.

@tklauser tklauser added area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. labels Oct 26, 2021
@tklauser tklauser requested review from a team as code owners October 26, 2021 10:25
@tklauser
Copy link
Member Author

ci-aks

@tklauser
Copy link
Member Author

ci-eks

@tklauser
Copy link
Member Author

ci-awscni

@tklauser
Copy link
Member Author

ci-gke

@tklauser
Copy link
Member Author

ci-external-workloads

@tklauser
Copy link
Member Author

All affected GH action workflows passed:

Removing test commit.

@tklauser tklauser force-pushed the pr/tklauser/update-cilium-cli-0.9.2 branch from 530c275 to 23dad88 Compare October 26, 2021 14:42
joestringer
joestringer approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks good, except for one change that looks unrelated.

.travis.yml
@@ -37,6 +37,7 @@ addons:
packages:
- kernel-package
- gnupg
- ipset
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change necessary here?

Copy link
Member Author

@tklauser tklauser Oct 27, 2021 •
edited

Choose a reason for hiding this comment

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

This is unrelated and I forgot to revert before creating the commit. Will drop that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out I forgot to push the actual change doing this 🤦 sorry about that!

I can send a PR removing it again if needed. This was meant to silence some log warning related to missing ipset binary during privileged tests on Travis CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've had to rebase #17687 which also changes .travis.yaml, so I've added a commit to that PR to drop ipset again.

@tklauser tklauser force-pushed the pr/tklauser/update-cilium-cli-0.9.2 branch from 23dad88 to 84b6275 Compare October 27, 2021 06:30
nebril
nebril reviewed Oct 27, 2021
View reviewed changes
.github/workflows/conformance-multicluster.yaml
@@ -266,11 +266,6 @@ jobs:
cilium hubble enable --context ${{ steps.contexts.outputs.context1 }} ${{ steps.vars.outputs.hubble_enable_defaults }}
cilium hubble enable --context ${{ steps.contexts.outputs.context2 }} ${{ steps.vars.outputs.hubble_enable_defaults }} --relay=false
Copy link
Member

Choose a reason for hiding this comment

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

This slightly increase the test run, because first cluster hubble will wait for hubble to be ready and only then enable hubble in second cluster (and wait). Would be nice to parallelize this, but this is a nit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I guess we could run the first one with --wait=false to circumvent that and keep the cilium status --wait in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Incremental diff:

diff --git a/.github/workflows/conformance-multicluster-v1.10.yaml b/.github/workflows/conformance-multicluster-v1.10.yaml
index db0f4e4fde9d..a0ff7ab91c8e 100644
--- a/.github/workflows/conformance-multicluster-v1.10.yaml
+++ b/.github/workflows/conformance-multicluster-v1.10.yaml
@@ -260,8 +260,9 @@ jobs:
 
       - name: Enable Relay
         run: |
-          cilium hubble enable --context ${{ steps.contexts.outputs.context1 }} ${{ steps.vars.outputs.hubble_enable_defaults }}
+          cilium hubble enable --context ${{ steps.contexts.outputs.context1 }} ${{ steps.vars.outputs.hubble_enable_defaults }} --wait=false
           cilium hubble enable --context ${{ steps.contexts.outputs.context2 }} ${{ steps.vars.outputs.hubble_enable_defaults }} --relay=false
+          cilium status --wait --context ${{ steps.contexts.outputs.context1 }}
 
       - name: Enable cluster mesh
         run: |
diff --git a/.github/workflows/conformance-multicluster.yaml b/.github/workflows/conformance-multicluster.yaml
index 0b405066c0a1..43384814917a 100644
--- a/.github/workflows/conformance-multicluster.yaml
+++ b/.github/workflows/conformance-multicluster.yaml
@@ -263,8 +263,9 @@ jobs:
 
       - name: Enable Relay
         run: |
-          cilium hubble enable --context ${{ steps.contexts.outputs.context1 }} ${{ steps.vars.outputs.hubble_enable_defaults }}
+          cilium hubble enable --context ${{ steps.contexts.outputs.context1 }} ${{ steps.vars.outputs.hubble_enable_defaults }} --wait=false
           cilium hubble enable --context ${{ steps.contexts.outputs.context2 }} ${{ steps.vars.outputs.hubble_enable_defaults }} --relay=false
+          cilium status --wait --context ${{ steps.contexts.outputs.context1 }}
 
       - name: Enable cluster mesh
         run: |

@tklauser
CI: update cilium-cli to v0.9.2
4d3764b 
`cilium hubble enable` now correctly waits for Cilium status to be
ready. Thus, the extra `cilium status --wait` after enabling Hubble can
be avoided.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/update-cilium-cli-0.9.2 branch from 84b6275 to a56ce88 Compare October 27, 2021 13:42
@tklauser
Copy link
Member Author

ci-multicluster

@tklauser
Copy link
Member Author

tklauser commented Oct 27, 2021 •
edited

Multicluster test passed after latest changes (see #17706 (review) for context and incremental diff): https://github.com/cilium/cilium/runs/4022972414?check_suite_focus=true.

Removing test commit and marking as ready to merge.

@tklauser tklauser added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 27, 2021
@tklauser tklauser force-pushed the pr/tklauser/update-cilium-cli-0.9.2 branch from a56ce88 to 4d3764b Compare October 27, 2021 14:07
@nebril nebril merged commit b71d528 into master Oct 29, 2021
@nebril nebril deleted the pr/tklauser/update-cilium-cli-0.9.2 branch October 29, 2021 09:41
@tklauser tklauser mentioned this pull request Nov 1, 2021
Merged
@tklauser tklauser mentioned this pull request Dec 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebril nebril nebril approved these changes

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tklauser @joestringer @nebril