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] Test and build package using Azure Pipelines #1406

Merged
merged 7 commits into from
Feb 10, 2021
Merged

[ci] Test and build package using Azure Pipelines #1406

merged 7 commits into from
Feb 10, 2021

Conversation

jleveque
Copy link
Contributor

@jleveque jleveque commented Feb 6, 2021

Configure Azure Pipelines to run unit tests, build Python wheel and publish the test results, test coverage and resulting wheels.

Also clean up pytest.ini by adding .coveragerc file and add support for reporting branch coverage.

@jleveque jleveque self-assigned this Feb 6, 2021
@jleveque jleveque added the CI label Feb 6, 2021
@lguohan
Copy link
Contributor

lguohan commented Feb 8, 2021

can you check why are we running this command in the test environment?

    def test_neighbor_advertiser_slice(self, set_up):
        cmd = "sudo sysctl -w net.ipv6.conf.all.disable_ipv6=0"
>       subprocess.check_output(cmd.split())

this is in docker environment, sysctl -w won't work here.

@jleveque
Copy link
Contributor Author

jleveque commented Feb 8, 2021

can you check why are we running this command in the test environment?

    def test_neighbor_advertiser_slice(self, set_up):
        cmd = "sudo sysctl -w net.ipv6.conf.all.disable_ipv6=0"
>       subprocess.check_output(cmd.split())

this is in docker environment, sysctl -w won't work here.

@sumukhatv: It appears you added these changes a few days ago here. Can you explain why?

@sumukhatv
Copy link
Contributor

can you check why are we running this command in the test environment?

    def test_neighbor_advertiser_slice(self, set_up):
        cmd = "sudo sysctl -w net.ipv6.conf.all.disable_ipv6=0"
>       subprocess.check_output(cmd.split())

this is in docker environment, sysctl -w won't work here.

@sumukhatv: It appears you added these changes a few days ago here. Can you explain why?

I added this to test the neighbor_advertiser script for advertisement of IPv6 Link local address
#1402

In order to add an IPv6 address to the Vlan in the test, I had to set this net.ipv6.conf.all.disable_ipv6 to 0 because it was failing earlier and I came to know that it IPv6 is disabled by default inside docker. I think there is another way to do this, which is directly modifying the /etc/sysctl.conf. Should I do that instead?

@jleveque
Copy link
Contributor Author

jleveque commented Feb 8, 2021

can you check why are we running this command in the test environment?

    def test_neighbor_advertiser_slice(self, set_up):
        cmd = "sudo sysctl -w net.ipv6.conf.all.disable_ipv6=0"
>       subprocess.check_output(cmd.split())

this is in docker environment, sysctl -w won't work here.

@sumukhatv: It appears you added these changes a few days ago here. Can you explain why?

I added this to test the neighbor_advertiser script for advertisement of IPv6 Link local address
#1402

In order to add an IPv6 address to the Vlan in the test, I had to set this net.ipv6.conf.all.disable_ipv6 to 0 because it was failing earlier and I came to know that it IPv6 is disabled by default inside docker. I think there is another way to do this, which is directly modifying the /etc/sysctl.conf. Should I do that instead?

No, that also will not work because the tests are run in the slave container; you cannot modify the sysctl values at runtime. They must be added to the docker run command line. See here: docker/for-linux#197

@lguohan: Should we consider running the slave container with the appropriate --sysctl parameters, or should @sumukhatv instead mock functions which rely on this support?

Edit: Trying to understand why we didn't see this issue with Jenkins. I believe it is because with Jenkins, we add the --privileged option to the docker run command line, whereas Azure Pipelines doesn't run the container in privileged mode?

Edit 2: I just pushed a commit to add the --privileged flag as an option to the container in AP, and it seems to have worked! :)

@jleveque jleveque marked this pull request as ready for review February 9, 2021 06:23
@@ -9,11 +9,71 @@ trigger:
pool:
vmImage: ubuntu-20.04

container:
image: sonicdev-microsoft.azurecr.io:443/sonic-slave-buster:latest
options: --privileged
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not do this, but I will accept it as a workaround.

ssdutil
undebug
utilities_common
watchdogutil
Copy link
Contributor

Choose a reason for hiding this comment

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

we have to remember all the folder and update the folder when we have new one. is there way to include all of them?

Copy link
Contributor Author

@jleveque jleveque Feb 9, 2021

Choose a reason for hiding this comment

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

I tried this by using the omit = parameter here instead to skip directories we don't want to test. However, with this approach it appeared to only report coverage for directories which had > 0% coverage (i.e., directories which do not yet have any unit tests were not reported as having 0% coverage). This is no worse than the pervious solution, however, the list of directories is more organized and easy to read in .coveragerc than it was is pytest.ini.

I will continue looking into a way to make this better.

@lguohan
Copy link
Contributor

lguohan commented Feb 9, 2021

those tests should be added to sonic-mgmt since they are system level test. if we are testing those commands, it is really testing ubuntu 20.04 command line, it does not help us. we should move them out to sonic-mgmt repo. the system level test does not belong to this repo. @sumukhatv, can you take action on this?

Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

have some concern, but it is ok.

@sumukhatv
Copy link
Contributor

those tests should be added to sonic-mgmt since they are system level test. if we are testing those commands, it is really testing ubuntu 20.04 command line, it does not help us. we should move them out to sonic-mgmt repo. the system level test does not belong to this repo. @sumukhatv, can you take action on this?

It is very specific to the feature of advertising link local address. In order to unit-test the feature, we need to add the IPv6 address via command line. We can also have this in sonic-mgmt, but it is required for unit-testing the neighbor-advertiser script.

@jleveque
Copy link
Contributor Author

jleveque commented Feb 9, 2021

those tests should be added to sonic-mgmt since they are system level test. if we are testing those commands, it is really testing ubuntu 20.04 command line, it does not help us. we should move them out to sonic-mgmt repo. the system level test does not belong to this repo. @sumukhatv, can you take action on this?

It is very specific to the feature of advertising link local address. In order to unit-test the feature, we need to add the IPv6 address via command line. We can also have this in sonic-mgmt, but it is required for unit-testing the neighbor-advertiser script.

@sumukhatv: For the sake of unit testing, you can simply mock the calls to ip -6 ... and return contrived output. For example, this line: https://github.com/sumukhatv/sonic-utilities/blob/4cd83dffa9854be402cb1f3e3d14315e5f5a3af9/scripts/neighbor_advertiser#L239

@lguohan lguohan merged commit eb70ebc into sonic-net:master Feb 10, 2021
@jleveque jleveque deleted the enable_az_pipelines branch February 10, 2021 17:59
anand-kumar-subramanian pushed a commit to anand-kumar-subramanian/sonic-utilities that referenced this pull request Mar 2, 2021
xumia pushed a commit to xumia/sonic-utilities that referenced this pull request Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants