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 integration of networkservicemesh integration tests (NSMNSE-79) #23

Open
wants to merge 64 commits into
base: vl3_latest
Choose a base branch
from

Conversation

amberimam
Copy link

Description

  • Add a jenkinsfile to this repo which installs all necessary dependencies for the networkservicemesh/cloudtest tool to run golang tests in the test/integration directory, and then runs single-cluster, interdomain and benchmark tests.
  • These tests can be configured to run upon any push to the networkservicemesh repo

Motivation and Context

We need to harden the cisco-app-networking fork of networkservicemesh and having these automated tests in place will help us to catch bugs more quickly.

https://jira-eng-sjc4.cisco.com/jira/browse/NSMNSE-79

Amber Imam added 30 commits January 27, 2021 12:48
@@ -1,36 +0,0 @@
---
Copy link
Collaborator

@tiswanso tiswanso Feb 5, 2021

Choose a reason for hiding this comment

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

Do we need to delete these files? Does it effect anything to keep them?

In general with the networkservicemesh repo we want to try to have as small a footprint as possible (within reason) due to the need to periodically rebase to newer versions of the upstream.

Copy link
Author

Choose a reason for hiding this comment

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

I can add them back in. I just thought it would be easier to understand what's going on if we didn't have unused files lying around.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It'll be easiest to maintain the upstream rebases if we stick as much to purely additive changes as possible. If these files change upstream and we've deleted them in our downstream fork then we'll likely have to resolve the conflicts manually every rebase.

Copy link
Author

Choose a reason for hiding this comment

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

I see that makes sense. I can add back in the .cloudtest.yaml file as well I guess, even though the jenkinsfile is not using that one.

timeout: 300 # 5 minutes to start cluster
stop-delay: 10
env:
- KIND_CLUSTER_NAME=cloudtest-kind-$(rands10) # Generate a uniq cluster name
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does the injection of the $(rands10) actually work? Does cloudtest process the yaml file content in a way that a bash shell would exec that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see that a makefile target is wrapping usage of the env var with $() exec... interesting

Copy link

Choose a reason for hiding this comment

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

Cloudtest does do extra processing.
https://github.com/networkservicemesh/cloudtest/blob/master/docs/README.md#environment-variables-processing

you get environment variables and a handful of other useful commands, like rands10. I'm not sure if this is executed by bash or not, they may have just borrowed the syntax.

//The Jenkinsfile runs entire build in a golang container and mounts the /var/run/docker.sock file to allow access to the host docker within the container
node('wcm') {
def build_ok = true
docker.image('golang:1.15-buster').inside('-u root --net=host -v /var/run/docker.sock:/var/run/docker.sock') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll possibly have an issue with using -u root in the long run. Got some feedback on that from the SRE team that it could be causing content to be left on the jenkins slave that the container runs on.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, but I can leave the -u root there for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you could try without -u root to see if it's still working the same that'd be good. To get kind cluster create and go get / go mod stuff to run in nsm-nse I had to add the following to env vars:

        HOME        = "${WORKSPACE}"
        GOCACHE     = "/go/.cache"

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I can give that a go and see what happens.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Ideally, we would have access to an ephemeral docker host, or the build agent would be ephemeral, so that we can easily guarantee that our environments are consistent. There's too many hands touching those build agents to have any sort of guarantee that they're going to be in a consistent state.

CGO_ENABLED = 0
GO111MODULE = on
}
stage ('Install Docker Client') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I refactored what was in nsm-nse to use the same base for our runner container we use in circleci to avoid having to install all these type of dependencies--docker client, kubectl, helm, kind, go, git, etc. I'm looking to get a more standard set of base container images curated by the SRE team that we can use as our jenkins agent setting. We don't have to change this now but it'll be something we want to follow-up on.

See here: https://github.com/cisco-app-networking/nsm-nse/blob/devop_jenkins/.jenkins/Jenkinsfile#L14

Copy link
Author

Choose a reason for hiding this comment

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

ok thanks, good to know.

.mk/kind.mk Outdated
@kind get kubeconfig --name $(KIND_CLUSTER_NAME) > $(KIND_CLUSTER_NAME)-kubeconfig
@touch $(CONFIG_LOCATION); \
kind get kubeconfig --name $(KIND_CLUSTER_NAME) > $(CONFIG_LOCATION); \
echo "$(CONFIG_LOCATION)"
Copy link
Collaborator

@tiswanso tiswanso Feb 5, 2021

Choose a reason for hiding this comment

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

Is tempdir an executable that generates a temporary directory? If so, I don't think the echo will actually show the right CONFIG_LOCATION value because it's going to re-exec the tempdir command which will generate a new dir. Also, I don't see how you can use the KIND_CLUSTER_NAME var like that and have it generate a name used for the kind create cluster command and then use it again for this kubeconfig get call.

Copy link
Author

@amberimam amberimam Feb 5, 2021

Choose a reason for hiding this comment

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

Good point! I will delete that echo line.

Copy link
Author

Choose a reason for hiding this comment

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

Why can't I reuse the KIND_CLUSTER_NAME var?

I found those changes to the kind.mk file in the upstream networkservicemesh/cloudtest repo: https://github.com/networkservicemesh/cloudtest/blob/master/examples/kind/Makefile#L39

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking that $(rands10) generates a random string each time it's invoked. But it's not a standard executable so maybe cloudtest does some kind of replace on it. Craziness... but I guess if it's the established pattern in cloudtest I'm ok with it for now until we figure out whether it's a problem.

test/go.mod Outdated
@@ -19,6 +19,7 @@ require (
github.com/networkservicemesh/networkservicemesh/test v0.0.0-00010101000000-000000000000
github.com/networkservicemesh/networkservicemesh/utils v0.3.0
github.com/onsi/gomega v1.10.3
github.com/packethost/packngo v0.5.1 // indirect
Copy link
Collaborator

Choose a reason for hiding this comment

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

was this something you actually added or something added by your IDE?

Copy link
Author

Choose a reason for hiding this comment

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

My IDE added it. Should I remove it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes and also revert the diffs to go.sum

Copy link
Author

Choose a reason for hiding this comment

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

Sure will do!

@tiswanso
Copy link
Collaborator

tiswanso commented Feb 5, 2021

I took a look at the cloudtest repo and see that you copied a lot of the .cloudtest content from there. It uses the same kind clustername and configdir env var approach with the $(rands10) and $(tempdir). I'm a bit skeptical about that approach working properly but not because of anything you did, specifically. Based on what you mentioned about the multiple kind interdomain testing, it's probably something not working for at least the cases when there's more than 1 cluster being started. A little disappointing in that hacky-ness but maybe it's something we can fix quickly and push upstream as well.

@amberimam
Copy link
Author

Thanks for your feedback Tim! :)

@@ -2,23 +2,27 @@
*

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you comment on how these changes came about?

Copy link
Author

Choose a reason for hiding this comment

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

I basically rearranged them in alphabetical order, and then added some folders and files that were needed by cloudtest.

Copy link
Author

Choose a reason for hiding this comment

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

I was using a dockerfile for local testing on my mac before I started using the cisco-hosted-jenkins environment.

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.

10 participants