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

Calico CI #202

Closed
wants to merge 7 commits into from
Closed

Calico CI #202

wants to merge 7 commits into from

Conversation

Mixaster995
Copy link
Contributor

@Mixaster995 Mixaster995 commented Jan 11, 2022

Vladimir Popov added 6 commits January 19, 2022 09:05
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
@Mixaster995 Mixaster995 changed the title Calico Calico CI Jan 20, 2022
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
@denis-tingaikin
Copy link
Member

@edwarnicke Finally Calico passes CI.

As I can see this PR adds a separate job that runs a few of our tests on packet setup with Calico CNI plugin.

For me, these CI changes look overcomplicated, but I think we can go with them for now.

Can we merge this and consider simplification a bit after?

Please share your thoughts.

@@ -1,7 +1,7 @@
---
version: 1.0
root: "./.tests/cloud_test/"
timeout: 7200 # 2 hour total total timeout
timeout: 10800 # 3 hour total total timeout
Copy link
Member

Choose a reason for hiding this comment

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

3 hours is a long timeout... why is it taking that long?

strategy:
fail-fast: false
matrix:
calico: ["off", "on"]
Copy link
Member

Choose a reason for hiding this comment

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

Might it not make more sense semantically to have this matrix be a matrix for CNI plugin, where calico-vpp is just one of the choices? That would let us have an easy visible path for other CNIs to be added later. Thoughts?

@@ -79,6 +83,7 @@ jobs:
- name: Install cloudtest # 3. Install cloudtest
run: |
go get github.com/networkservicemesh/cloudtest@master
# GOPROXY=direct go get github.com/Mixaster995/cloudtest@no-cleanup
Copy link
Member

Choose a reason for hiding this comment

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

Should this commented line be removed?

suite.Run(t, new(multiforwarder.Suite))
}

func TestHeal(t *testing.T) {
if isCalico() {
t.Skip("not available with Calico")
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are heal tests not available with Calico?

if !isCalico() {
t.Skip("not available without Calico")
}
suite.Run(t, new(calico.Suite))
Copy link
Member

Choose a reason for hiding this comment

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

Why does calico have its own independent suite instead of simply running the standard test suite?

@edwarnicke
Copy link
Member

My overall thoughts here are:

  1. It might be much more productive to think of this in terms of testing across different CNIs, rather than a binary Calico/NotCalico binary
  2. I'm a little confused why Calico has its own custom suite instead of simply using the standard test suites. I'm also confused why healing/sriov/etc are disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants