Add a simulated dev/test environment and simulator-based e2e CI - #105
Add a simulated dev/test environment and simulator-based e2e CI#105vishesh92 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 50.05% 50.56% +0.50%
==========================================
Files 4 4
Lines 975 981 +6
==========================================
+ Hits 488 496 +8
+ Misses 473 471 -2
Partials 14 14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9e0a52b to
b3a11f1
Compare
b3a11f1 to
193dabc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
cloudstack_loadbalancer.go:487
- The netErr branch returns the wrong error variable (it returns err from the previous GetPublicIpAddressByID call). If the first call succeeded (err==nil) but GetNetworkByID fails, this currently returns a nil error and masks the failure.
network, _, netErr := cs.client.Network.GetNetworkByID(ip.Associatednetworkid, cloudstack.WithProject(cs.projectID))
if netErr != nil {
klog.Errorf("Failed to fetch the network for id: %v", ip.Associatednetworkid)
return "", err
}
193dabc to
f656173
Compare
f656173 to
98f7d4a
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 5 comments.
98f7d4a to
0ac220a
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 6 comments.
| # environment, so `make test-e2e` reaches the same simulator `make e2e-up` | ||
| # published rather than assuming the default port. | ||
| SIM_HOST_PORT ?= 8080 | ||
| CS_API_URL ?= http://localhost:$(SIM_HOST_PORT)/client/api |
|
|
||
| # Simulator-based e2e environment; see docs/development.md | ||
| e2e-up: | ||
| hack/e2e/up.sh |
0ac220a to
80f3681
Compare
The repository had no way to exercise the CCM end to end. The only "run against real CloudStack" hook was configFromEnv() in cloudstack_test.go, which skips unless CS_API_URL and friends are set, and nothing set them. As a result EnsureLoadBalancer, UpdateLoadBalancer and EnsureLoadBalancerDeleted -- the three functions holding nearly all of the load balancer branching -- had no test coverage at all, and the README pointed at a Docker Hub image (cloudstack/simulator) that no longer exists. Add hack/e2e, which brings up a CloudStack simulator, deploys its advanced zone, mints admin API keys, creates a kind cluster and deploys CloudStack VMs matching its nodes, then runs the CCM against both. CloudStack calls go through cmk, so the scripts run the same commands the documentation tells you to run, and cmk's own async job handling removes any need to poll queryAsyncJobResult. docs/development.md walks through the same steps by hand so the environment is understandable rather than magic. Add a Go e2e suite under test/e2e covering load balancer lifecycle, node initialization, service annotations and the VPC/network ACL path. It is behind the e2e build tag, so it stays out of `make test` and `go build ./...`, and it needs no new module dependencies. Run all of it in CI as a matrix of the latest two Kubernetes minors against CloudStack 4.22.1.0 and 4.20.2.0. The CloudStack axis is not only version coverage: 4.22 and later update a load balancer rule's CIDR list in place while earlier releases delete and recreate the rule, so both branches are exercised. Cells run in parallel and share a single image build, and the simulator and kind node images are cached between runs, so the workflow costs about as much wall-clock as a single run. Fix two latent bugs in the load balancer path that the new suite exposed, both affecting CloudStack projects. updateNetworkACL fetched the network and its ACL list without the project, so every LoadBalancer service on a VPC owned by a project failed with "error fetching Network with ID" and never got an ingress address. getNetworkIDFromIPAddress had the same omission, breaking load balancer deletion for projects; it also reported a failed network lookup as success by returning the wrong error variable, and guarded on Networkid while looking up Associatednetworkid. Either could hand the caller an empty network ID, which GetNetworkByID does not reject but looks up as an unfiltered network list, so it could resolve to an arbitrary network instead of failing. Also add the local cloud-config, cmk-config and kube-config files to .gitignore. They hold live credentials and were previously untracked but not ignored. Fixes #4 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
80f3681 to
f5f9f69
Compare
Fixes #4
What this does
Issue #4 asked for documentation on running kube-apiserver and
cloudstack-simulator locally, "preferring containers so automated test
workloads can be implemented later." This does both halves.
make e2e-upbrings up a complete, unmocked stack in about seven minutes: akind cluster, the CloudStack simulator with its advanced zone deployed,
CloudStack VMs matching the kind nodes, and the CCM built from the checkout.
docs/development.mdwalks through the same steps manually, so the environmentcan be understood and debugged rather than just invoked.
make e2e-up kubectl create deployment web --image=nginx kubectl expose deployment web --port=80 --type=LoadBalancer kubectl get svc web -w # EXTERNAL-IP arrives from 192.168.2.0/24 make test-e2e make e2e-downWhy it is worth the files
EnsureLoadBalancer,UpdateLoadBalancerandEnsureLoadBalancerDeletedhadno tests. Only their helpers did, and those mostly pass
gomock.Any()forrequest parameters, so what the CCM actually sends to CloudStack was largely
unverified. The new suite is 15 tests covering load balancer lifecycle, node
initialization, annotations and session affinity, and the VPC/network ACL path
against a real management server.
It lives in
test/e2ebehind thee2ebuild tag, so it stays out ofmake testandgo build ./..., and it needs no new module dependencies.Two bugs this found
Both affect anyone running the CCM against a CloudStack project, and both
are fixed here:
updateNetworkACLfetched the network and its ACL list without projectscoping. On a VPC owned by a project, every LoadBalancer service failed with
error fetching Network with ID: ... No match foundand never received aningress address.
getNetworkIDFromIPAddresshad the same omission on two calls, which breaksload balancer deletion for projects.
The fix is four added
cloudstack.WithProject(...)arguments, matching whatthe neighbouring call sites already do. Note this is distinct from the
could not find networkcase in the Troubleshooting section, which is inassociatePublicIPAddressand was already scoped correctly.CI
.github/workflows/e2e-simulator.ymlruns on pull requests and pushes tomain, as a 2x2 matrix:The CloudStack axis buys branch coverage, not just version coverage: 4.22+
updates a rule's CIDR list in place, 4.20 deletes and recreates it. A shared
build job compiles the image once and the four cells run in parallel, so the
workflow takes roughly as long as a single run (~15 minutes). Failures upload
simulator logs, CCM logs, node and service dumps, and CloudStack-side state.
Not added as a required check in
.asf.yamlyet — worth letting it proveitself stable first.
Known limitation
kind starts kubelet with
--provider-id=kind://..., and Kubernetes only lets aprovider ID be set once, so the CCM never assigns
external-cloudstack://<uuid>in this environment.TestNode_ProviderIDdetects that, logs the value it would have assigned, and reports itself as
skipped, so the gap stays visible instead of quietly passing. It is called out
in the docs.
Notes for reviewers
Debuggingsection moved todocs/development.md, where thelaunch.jsonsnippet now points at the harness-generatedcloud-config-hostandkubeconfig. The README keeps a pointer to it.cloudstack/simulatorDocker Hub link is corrected toapache/cloudstack-simulator..gitignorenow covers the localcloud-config,cmk-configandkube-configfiles. They hold live credentials and were previouslyuntracked but not ignored.
cloudstack_loadbalancer_test.gogained agomock.Any()argument for the new variadic project option, matching theexisting project-scoped call sites.
Testing
Ran the full harness locally against
apache/cloudstack-simulator:make test,golangci-lint runandgo build ./...are cleanThe CCM's
k8s.io/*v0.24 libraries worked fine against a v1.37 API server.🤖 Generated with Claude Code