-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: vl3_latest
Are you sure you want to change the base?
CI integration of networkservicemesh integration tests (NSMNSE-79) #23
Conversation
…ger tests from jenkins for now
… if that helps them run
@@ -1,36 +0,0 @@ | |||
--- |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what it looks like without -u root
: https://engci-private-sjc.cisco.com/jenkins/eti-sre/job/AppN/job/amimam/job/networkservicemesh/job/devop_jenkins/143/console
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure will do!
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 |
Thanks for your feedback Tim! :) |
…anges between this repo and upstream repo
@@ -2,23 +2,27 @@ | |||
* | |||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
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