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

CI support for unit tests requiring IPv6 (switched main build to vm execution instead of docker) #803

Closed
wants to merge 2 commits into from

Conversation

andy31415
Copy link
Contributor

@andy31415 andy31415 commented May 21, 2020

This is a follow up on previous attempts to re-enable inet tests that require IPv6 support

Problem

Tests that require IPv6 do not run in CI

Summary of Changes

Based on https://circleci.com/docs/2.0/faq/#can-i-use-ipv6-in-my-tests, enabling running tests as a machine executor instead of docker (this has a speed penalty, I believe of 1-2 minutes, but did not exactly check)

  • Executing as machine for "main build"
  • Enabled a inet test that requires ipv6
  • added script to save test log files in case of log failure
  • ran ldconfig to pick up new folder of /usr/local/lib where we put our crypto libs

@andy31415 andy31415 force-pushed the 01_ipv6_ci_tests branch 6 times, most recently from 5f248f2 to 61780b1 Compare May 21, 2020 16:53
@@ -1,6 +1,6 @@
main-build:
docker:
- image: connectedhomeip/chip-build:0.2.11
machine:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to spin up a VM for some tests, let's add that to our line up (add a new executor), not break/change the current build.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the whole of the project should enable and assume IPv6 support. I don't think this should be a corner case in the same vein as mbedTLS, LwIP, ESP32, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if there is a benefit to running the build in both docker and VM, whever VM does more (E.g. more unit tests) than docker.

The issue with not changing the current build would be that I would need to add extra logic/arguments in what tests are being run, so that the current build does not run on things that do not support ipv6. That seems counter-productive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Build times are the challenge here.

I was planning on taking a stab at this, by making a linux builder than just runs the docker build, that way we have standard builds across everything here, and IPv6 - and have everything do that. This was just about to come to the top of my stack... happy to take a shot.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like we're aligned on approach, at a high-level. I think @andy31415 is currently trying to tackle that. Perhaps you two can tag team?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only switches the main-build, not the rest (or at least I think it does, I may have missed some linkage). I think that includes build & test for linux, but not the other parts.

The part that is being solved is that we do not currently have unit tests on core layers. This seems to trump a 1 minute CI delay since code reviews generally take longer than that anyway.

Could we agree to slot this and have any other test-only move separate? Alternatively could we set a deadline for figuring out if there is a better way to run only tests on a VM and do time comparisons?

I would like to make sure progress is being made without blocking things for an unspecified amount of time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mac builds will be done on premises, hosted by Apple, that's being worked don right now. I will add some changes to this PR to get this to where we want it to be, but as it is - I'm not sure I support switching all our builds over to this. I'm going to do this more surgically for just tests.

That's awesome of Apple to volunteer machines for this. Will this be tied into CircleCI somehow and controlled from the main project?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, this will follow CircleCI exactly. Some magic on the backend :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ping on this one - what were the results of the Circle CI discussion? can we use docker after all or is there some other solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given Project CHIP is developing an application stack on IPv6, and we need IPv6 support for even the most basic test framework, it seems like we must move forward with the only path CircleCI provides for IPv6 support: machine executors. Perhaps we can partition the use of machine executors to only apply to Inet and network testing, and use the turn-key sandboxed dockers for all other builds / unit tests.

@andy31415 andy31415 force-pushed the 01_ipv6_ci_tests branch 2 times, most recently from 310265c to e37da5b Compare May 21, 2020 19:16
@andy31415 andy31415 marked this pull request as ready for review May 21, 2020 19:19
@andy31415 andy31415 changed the title Investigate ipv6 CI support CI support for unit tests requiring IPv6 (switched main build to vm execution instead of docker) May 21, 2020
Changes:
  - use MACHINE EXECUTUION for CI to have ipv6
  - add "save logs" as artifacts in case of tests failures
  - enabled a test that uses ipv6 (but not all ... that will be done in
    the future)
  - add ldconfig to "install packages" since it seems that new libs
    were not picked up in /usr/local/lib by default because that folder
    is missing in the default ubuntu image we are running on
@woody-apple woody-apple self-requested a review May 21, 2020 19:59
Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

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

I will add some changes to this PR to get this to where we want it to be, but as it is - I'm not sure I support switching all our builds over to this. I'm going to do this more surgically for just tests.

Chatting with CircleCI folks a little later today (post 5:30 PM)

# TODO: some tests are stranded as CI servers historically did not fully
# support IPv6. Supporting these is a WIP.
#
# CI documentation on IPv6:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd avoid putting CI-specific comments in here.


if CHIP_TARGET_STYLE_UNIX
# Build and run these test targets only on standalone device targets
noinst_PROGRAMS = \
TestLwIPDNS \
TestInetEndPoint \
TestInetLayer \
TestInetLayerDNS \
Copy link
Contributor

Choose a reason for hiding this comment

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

No move of this one too? See issue #323 and issue #329.

@woody-apple
Copy link
Contributor

I will add some changes to this PR to get this to where we want it to be, but as it is - I'm not sure I support switching all our builds over to this. I'm going to do this more surgically for just tests.

Chatting with CircleCI folks a little later today (post 5:30 PM)

Note to self: Build times are now 4:30 instead of 1:15, need to target around 1:30

@gerickson
Copy link
Contributor

Two things in this PR I would have expected to see, but didn't:

  1. Something like this: https://github.com/openweave/openweave-core/blob/master/.travis/before_install.sh#L123
  2. Something like this:
 - run:
         name: Enable IPv6
         command: |
           cat \<<'EOF' | sudo tee /etc/docker/daemon.json
           {
             "ipv6": true,
             "fixed-cidr-v6": "2001:db8:1::/64"
           }
           EOF
           sudo service docker restart

from this: https://github.com/project-chip/connectedhomeip/pull/500/files#diff-f46f63d104f0a7937fdc4b992e461f8b.

@andy31415
Copy link
Contributor Author

Two things in this PR I would have expected to see, but didn't:

  1. Something like this: https://github.com/openweave/openweave-core/blob/master/.travis/before_install.sh#L123
  2. Something like this:
 - run:
         name: Enable IPv6
         command: |
           cat \<<'EOF' | sudo tee /etc/docker/daemon.json
           {
             "ipv6": true,
             "fixed-cidr-v6": "2001:db8:1::/64"
           }
           EOF
           sudo service docker restart

from this: https://github.com/project-chip/connectedhomeip/pull/500/files#diff-f46f63d104f0a7937fdc4b992e461f8b.

From the CI docs I read "You can also configure Docker to assign IPv6 address to containers, to test services with IPv6 setup. You can enable it globally by configuring docker daemon like the following:" and interpreting it as "if you are using docker, you can make it assign ipv6.

My belief is that this is if we use docker inside the VM (that sounds odd though), however we use machine execution, so docker should not need touching.

@woody-apple
Copy link
Contributor

From the CI docs I read "You can also configure Docker to assign IPv6 address to containers, to test services with IPv6 setup. You can enable it globally by configuring docker daemon like the following:" and interpreting it as "if you are using docker, you can make it assign ipv6.

My belief is that this is if we use docker inside the VM (that sounds odd though), however we use machine execution, so docker should not need touching.

Agree, however, the issue here is having easy ways to share built output to reduce build time, as well as make these scripts easy to build on/expand on. I'm going to try to attack this ASAP.

@woody-apple
Copy link
Contributor

FYI, @andy31415 and I chatted, going to try to see if I can have different executors for tests vs build

@andy31415
Copy link
Contributor Author

FYI, @andy31415 and I chatted, going to try to see if I can have different executors for tests vs build

@woody-apple did we succeed or have other plans to try?

@turon
Copy link
Contributor

turon commented Jun 26, 2020

Deprecates #500.

@woody-apple woody-apple marked this pull request as draft June 30, 2020 06:21
@woody-apple
Copy link
Contributor

Converting to Draft, I need to test out the recommended fixes here, behind :(

@woody-apple
Copy link
Contributor

Marking this as closed given #1372

@woody-apple woody-apple closed this Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants