-
Notifications
You must be signed in to change notification settings - Fork 344
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
add simulator for scale tests #1493
Conversation
Thanks for your PR. The following commands are available:
|
Codecov Report
@@ Coverage Diff @@
## master #1493 +/- ##
==========================================
+ Coverage 63.31% 63.74% +0.42%
==========================================
Files 170 181 +11
Lines 14250 15511 +1261
==========================================
+ Hits 9023 9888 +865
- Misses 4292 4584 +292
- Partials 935 1039 +104
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@tnqn I've updated this pull request based on your suggestions. |
Makefile
Outdated
scale-manifest: | ||
@echo "===> Generating dev manifest for Antrea <===" | ||
$(CURDIR)/hack/generate-manifest.sh --mode dev --simulator > build/yamls/antrea.yml | ||
$(CURDIR)/hack/generate-manifest.sh --mode dev --ipsec > build/yamls/antrea-ipsec.yml |
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 think we just need one manifest for scale test.
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.
Agree. Since we actually don't release the yaml, I also think we don't need a target here.
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.
@tnqn @weiqiangt do you think it is better to add the yaml file than generating yaml file by scripts?
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.
IMO, I actually prefer the opposite way. We can just generate one once we're going to use the simulator YAML.
But one quick question, how can users tune the number of simulated nodes? Maybe when generating the simulator YAML, the generator should accept an argument to tune the number.
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.
The published yaml are for user consumption, I think we don't put yaml for tests there, otherwise there will be too many, like the one for code coverage.
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.
IMO, I actually prefer the opposite way. We can just generate one once we're going to use the simulator YAML.
But one quick question, how can users tune the number of simulated nodes? Maybe when generating the simulator YAML, the generator should accept an argument to tune the number.
@liu4480, do we have a way to tune the simulator replica number?
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.
@weiqiangt I 've proposed a solution, for example:
make scale-manifest REPLICA=10
this will modify the replica to 10 for simulator
/test-all |
/test-all |
Makefile
Outdated
@echo "===> Generating dev manifest for Antrea <===" | ||
$(CURDIR)/hack/generate-manifest.sh --mode dev --simulator > build/yamls/antrea-simulator.yml | ||
$(CURDIR)/hack/generate-manifest.sh --mode dev --ipsec > build/yamls/antrea-ipsec.yml | ||
cp $(CURDIR)/build/yamls/patches/simulator/antrea-agent-simulator.yml build/yamls/ |
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 don't merge the simulator yaml into "antrea-simulator.yml" when executing generate-manifest.sh? I think it's how other modes do.
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.
since simulator is not in antrea.yml, patch will fail due to it can not find simulator.
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.
@tnqn how about command like:
cat $(CURDIR)/build/yamls/patches/simulator/antrea-agent-simulator.yml >> build/yamls/antrea-simulator.yml
this can also merge antrea-agent-simulator.yml into antrea-simulator.yml
Makefile
Outdated
docker build -t harbor-repo.vmware.com/dockerhub-proxy-cache/antrea/antrea-ubuntu-simulator:$(DOCKER_IMG_VERSION) \ | ||
-f build/images/Dockerfile.simulator.build.ubuntu . | ||
docker tag harbor-repo.vmware.com/dockerhub-proxy-cache/antrea/antrea-ubuntu-simulator:$(DOCKER_IMG_VERSION) \ | ||
harbor-repo.vmware.com/dockerhub-proxy-cache/antrea/antrea-ubuntu-simulator |
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.
harbor-repo.vmware.com/dockerhub-proxy-cache/antrea/antrea-ubuntu-simulator
doesn't seem right for 2 reasons:
harbor-repo.vmware.com
is a private VMware registry, not accessible outside of the VMware corporate networkdockerhub-proxy-cache
is meant to be used as a cache to pull images; it doesn't make sense to build an image locally and tag it like this
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 edit https://github.com/vmware-tanzu/antrea/blob/master/.github/workflows/build.yml, then an updated image for the simulator will be pushed to dockerhub every time master is updated. This image will be accessible through the VMware internal dockerhub proxy cache (if you are on the corporate network) AND will automatically be mirrored to projects.registry.vmware.com
where everyone will be able to pull it with no rate limiting restrictions. The YAML files can reference projects.registry.vmware.com
as it is a public registry.
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.
@antoninbas thanks, just change it to antrea/antrea-simulator-ubuntu
path: /var/log | ||
containers: | ||
- name: simulator | ||
image: harbor-repo.vmware.com/dockerhub-proxy-cache/antrea/antrea-ubuntu-simulator:latest |
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.
see my comment above about the harbor-repo.vmware.com/dockerhub-proxy-cache
registry
also, when is this image ever pushed to dockerhub?
// Package main under directory cmd parses and validates user input, | ||
// instantiates and initializes objects imported from pkg, and runs | ||
// the process. |
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.
that doesn't look like a super useful comment to me. That's very generic and not specific to the simulator
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.
thanks, added some comment specific to simulator
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 don't think this is any more useful. See my comment for main.go.
"github.com/vmware-tanzu/antrea/pkg/version" | ||
) | ||
|
||
func createClientSet(kubeConfig *rest.Config) (clientset.Interface, aggregatorclientset.Interface, crdclientset.Interface, error) { |
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 cannot use CreateClients
from vmware-tanzu/antrea/pkg/k8s
?
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.
thanks for pointing, changed to vmware-tanzu/antrea/pkg/k8s
In addition to my review comments, I do not see any information about the simulator in the PR description & commit messages? There is also no information as markdown documentation on how devs can run the simulator. |
@antoninbas I added a README in cmd/simulator to describe how to run simulator, is it OK? |
@antoninbas @tnqn just addressed the new comments, is there other comments about this PR? |
/test-all |
Makefile
Outdated
@@ -279,6 +290,11 @@ manifest: | |||
$(CURDIR)/hack/generate-manifest-octant.sh --mode dev > build/yamls/antrea-octant.yml | |||
$(CURDIR)/hack/generate-manifest-windows.sh --mode dev > build/yamls/antrea-windows.yml | |||
|
|||
.PHONY: scale-manifest | |||
scale-manifest: |
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.
scale-manifest: | |
manifest-scale: |
to keep align with the other name "manifest-converage"
Makefile
Outdated
.PHONY: scale-manifest | ||
scale-manifest: | ||
@echo "===> Generating simulator manifest for Antrea <===" | ||
$(CURDIR)/hack/generate-manifest.sh --mode dev --simulator > build/yamls/antrea-simulator.yml |
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.
$(CURDIR)/hack/generate-manifest.sh --mode dev --simulator > build/yamls/antrea-simulator.yml | |
$(CURDIR)/hack/generate-manifest.sh --mode dev --simulator > build/yamls/antrea-scale.yml |
Since the target is named manifest-scale and the yaml includes not only simulator
@@ -0,0 +1,33 @@ | |||
apiVersion: apps/v1 |
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.
maybe name it nodeAffinity.yml
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.
addressed
"appliedGroup", | ||
} | ||
|
||
// Call watch by goroutine with wait.NonSlidingUntil |
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.
nit: the below code is self-explanatory for this comment
docs/antrea-agent-simulator.md
Outdated
# Run Antrea agent simulator | ||
|
||
## 1. build the simulator image | ||
```bash |
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 remember there are some format requirements for the doc to be rendered on website correctly.
For example, there needs to be an empty line before the code. Not sure there's CI job checking this. But you can follow other docs' style or standard markdown guide.
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.
there is a CI job and looks like it's failing on this PR, which is a good sign :)
docs/antrea-agent-simulator.md
Outdated
make WHAT=cmd/kubemark KUBE_BUILD_PLATFORMS=linux/amd64 | ||
cp ./_output/local/go/bin/linux_amd64/kubemark cluster/images/kubemark | ||
``` | ||
change the image in Dockerfile to other base images if pull failed, for example(antrea/base-ubuntu:2.14.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.
I think we don't need to mention this in doc specically. And actually I can pull the image.
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 put it in vmware's private image repository, shall I also push it to antrea?
docs/antrea-agent-simulator.md
Outdated
```bash | ||
make scale-manifest | ||
``` | ||
The above command will create one simulator, to change the number of simulators, please modify the yaml file, edit the replica of simulator |
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.
Please wrap the lines following other docs's width
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.
Overall LGTM, comments for the doc left.
docs/antrea-agent-simulator.md
Outdated
@@ -0,0 +1,74 @@ | |||
[toc] |
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.
Remove this? It appears in the rendered page
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.
btw if you don't want to include a TOC for a doc, you can just add it to hack/.notableofcontents
docs/antrea-agent-simulator.md
Outdated
|
||
## Table of Contents | ||
<!-- toc --> | ||
- [build the images](#build-the-images) |
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.
toc should be capitalized, so do the section headers below.
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.
thanks, since the markdown is simple, I just removed the table of content
/test-all |
1 similar comment
/test-all |
.github/workflows/build.yml
Outdated
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Build Antrea Docker image | ||
run: make manifest-scale build-scale-simulator |
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 "manifest-scale" is here? the workflow is to push the image, right?
// Package main under directory cmd parses and validates user input, | ||
// instantiates and initializes objects imported from pkg, and runs | ||
// the process. |
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 don't think this is any more useful. See my comment for main.go.
docs/antrea-agent-simulator.md
Outdated
2. Build the kubemark image | ||
|
||
If you alreay have a kubemark image, please ignore this step. | ||
|
||
```bash | ||
cd $KUBERNETES_PATH | ||
make WHAT=cmd/kubemark KUBE_BUILD_PLATFORMS=linux/amd64 | ||
cp ./_output/local/go/bin/linux_amd64/kubemark cluster/images/kubemark | ||
cd cluster/images/kubemark | ||
docker build -t kubemark:latest . | ||
``` |
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.
@tnqn I thought you wanted to push the kubemark image to our dockerhub / harbor registry?
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 have done that, where do you think the commands that generate the image should be put?
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.
Not sure we need to include them in the repository at all? If you want to document them somewhere for future updates to the image, I guess you can add a document under https://github.com/vmware-tanzu/antrea/tree/master/docs/maintainers
I mostly wanted to point out that the current steps are misleading:
- users don't need to build the image themselves
- if users do decide to build the image themselves, they also need to load it into the Nodes and change the tag, as the manifest file references
projects.registry.vmware.com/antrea/kubemark:v1.18.4
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 removed this part from antrea-agent-simulator.md, and move to docs/maintainer/kubemark.md
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
// The simulator binary is responsible to run simulated nodes for antrea agent. |
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.
// The simulator binary is responsible to run simulated nodes for antrea agent. | |
// The simulator binary is responsible for running simulated antrea agent. |
docs/antrea-agent-simulator.md
Outdated
|
||
## Create the yaml file | ||
|
||
This demo uses 1 simulator, this command will create a yaml file build/yamls/antrea-simulator.yml |
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 demo uses 1 simulator, this command will create a yaml file build/yamls/antrea-simulator.yml | |
This demo uses 1 simulator, this command will create a yaml file `build/yamls/antrea-scale.yml`. |
docs/antrea-agent-simulator.md
Outdated
## Create secret for kubemark | ||
|
||
```bash | ||
cp ~/.kube/config admin.conf |
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 think it's not necessary to make a copy first. User can choose their own kubeconfig wherever it is as the the following command indicates.
docs/antrea-agent-simulator.md
Outdated
kubectl apply -f build/yamls/antrea-scale.yml | ||
``` | ||
|
||
check the simulator |
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.
check the simulator | |
Check the simulated Nodes: |
docs/maintainers/build-kubemark.md
Outdated
@@ -0,0 +1,10 @@ | |||
# Build the kubemark image | |||
This documentation simply describes how to build kubemark image. |
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 documentation simply describes how to build kubemark image. | |
This documentation simply describes how to build the kubemark image used in [Antrea scale testing](/docs/antrea-agent-simulator.md). |
Otherwise people won't know what this is for.
Add an empty line before the text. And no indent is needed I think.
docs/maintainers/build-kubemark.md
Outdated
make WHAT=cmd/kubemark KUBE_BUILD_PLATFORMS=linux/amd64 | ||
cp ./_output/local/go/bin/linux_amd64/kubemark cluster/images/kubemark | ||
cd cluster/images/kubemark | ||
docker build -t antrea/kubemark:latest . |
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.
docker build -t antrea/kubemark:latest . | |
docker build -t antrea/kubemark:v1.18.4 . |
docs/antrea-agent-simulator.md
Outdated
@@ -0,0 +1,46 @@ | |||
# Run Antrea agent simulator | |||
|
|||
This document describes how to run the Antrea agent simulator. The simulator is useful for Antrea scalability testing, without having to create a very large cluster. |
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.
Please wrap the text in 80 chars per line as other files do.
/test-all |
# Build the kubemark image | ||
This documentation simply describes how to build the kubemark image used in | ||
[Antrea scale testing](docs/antrea-agent-simulator.md) | ||
```bash |
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.
ditto
/test-all |
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.
LGTM
@antoninbas would you take a final look? |
RUN make antrea-agent-simulator | ||
|
||
|
||
FROM antrea/base-ubuntu:2.14.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.
does the simulator actually depend on this image? does it need Open vSwitch?
if not, it's better to just use ubuntu:20.04 as the base here; it will lead to a smaller image and will be easier to maintain IMO
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.
thanks, it does not need ovs, will change to ubuntu:20.04
docs/antrea-agent-simulator.md
Outdated
```bash | ||
make build-scale-simulator | ||
``` |
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.
nit: I don't think any of these codeblocks need to be indented
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.
@antoninbas thanks for the latest advices, just resolved
/test-all |
This patch implements agent simulator as add an independent bin, it runs with kubemark which simulates kubelet, and mainly watches NetworkPolicies, AddressGroups and AppliedToGroups from antrea controller and prints the events of these resources to log. With agent simulator, we do not need to lauch large cluster for scale test. The agent simulator uses labels and nodeaffinity to disable antrea-agent/antrea-controller running on the simulated nodes, and we can add some taints to not allow other pods to run on simulated nodes. To use agent simulator, please refer to docs/antrea-agent-simulator.md.
/test-all |
This patch implements agent simulator as add an independent bin, it runs with kubemark which simulates kubelet, and mainly watches NetworkPolicies, AddressGroups and AppliedToGroups from antrea controller and prints the events of these resources to log. With agent simulator, we do not need to lauch large cluster for scale test. The agent simulator uses labels and nodeaffinity to disable antrea-agent/antrea-controller running on the simulated nodes, and we can add some taints to not allow other pods to run on simulated nodes. To use agent simulator, please refer to docs/antrea-agent-simulator.md.
The binary simulates an antrea-agent in pod for scale test, it works with kubemark which simulates kubelet and kube-proxy. It mainly watches networkPolicies, addressGroups and appliedToGroups.
To run simulator , please refer to docs/antrea-agent-simulator.md