Bump go 1.22.2->1.22.5 and add E2E tests #639
Conversation
|
Hi @carreter. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
k8s-ci-robot
added
the
size/XXL
|
@avrittrohwer is on vacation this week, is anyone else available to review? |
|
Blocks #635 FYI |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@tallclair for visibility. Working on cleaning up the PR to see if we can avoid upgrading the go version and minimize the vendored dependencies! |
|
Seems like Here are the new packages that are getting pulled into @cheftako Where can I check if these are ok to include? |
| @@ -1,7 +1,6 @@ | |||
| module sigs.k8s.io/apiserver-network-proxy | |||
|
|
|||
| go 1.22 | |||
| toolchain go1.22.4 | |||
There was a problem hiding this comment.
hm, is there a reason for removing the go1.22.4 toolchain?
There was a problem hiding this comment.
go mod tidy did this automatically. Reverted.
Makefile
Outdated
| @@ -80,6 +80,10 @@ test: | |||
| test-integration: build | |||
| go test -mod=vendor -race ./tests -agent-path $(PWD)/bin/proxy-agent | |||
|
|
|||
| .PHONY: test-e2e | |||
| test-e2e: docker-build | |||
| go test -mod=vendor ./e2e -agent-image ${AGENT_FULL_IMAGE}-$(TARGETARCH):${TAG} -server-image ${SERVER_FULL_IMAGE}-$(TARGETARCH):${TAG} | |||
There was a problem hiding this comment.
is there a reason not to run with -race enabled?
There was a problem hiding this comment.
Nope, just didn't know about it. Added the -race flag.
e2e/templates_test.go
Outdated
| ) | ||
|
|
||
| // renderServerTemplate renders a template from e2e/templates/server into a kubernetes object. | ||
| func renderServerTemplate(file string, params any) (client.Object, *schema.GroupVersionKind, error) { |
There was a problem hiding this comment.
thoughts on having just a single renderTemplate where callers pass a file path relative to templates like "agent/service.yaml"?
There was a problem hiding this comment.
Sounds like this would be significantly easier to use. Will do!
e2e/templates_test.go
Outdated
| @@ -0,0 +1,65 @@ | |||
| package e2e | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
thoughts on putting this logic in main_test.go? When reading through main_test.go it was not immediately obvious where these funcs were defined
e2e/main_test.go
Outdated
| envfuncs.CreateCluster(kindCluster, kindClusterName), | ||
| envfuncs.LoadImageToCluster(kindClusterName, *agentImage), | ||
| envfuncs.LoadImageToCluster(kindClusterName, *serverImage), | ||
| func(ctx context.Context, cfg *envconf.Config) (context.Context, error) { |
There was a problem hiding this comment.
thoughts on making this a named renderAndApplyManifests() func?
e2e/templates/agent/statefulset.yaml
Outdated
| @@ -0,0 +1,82 @@ | |||
| apiVersion: apps/v1 | |||
| kind: StatefulSet | |||
There was a problem hiding this comment.
is there a reason for using a StatefulSet?
There was a problem hiding this comment.
StatefulSets give the pods a unique identifier we can reach them with.
metricsFamilies, err := getMetrics(fmt.Sprintf("%v-%v:%v/metrics", agentPod.Name, agentServiceHost, adminPort))
e2e/singleserver_singleagent_test.go
Outdated
| Replicas: 1, | ||
| Image: *serverImage, | ||
| Args: []KeyValue{ | ||
| {Key: "log-file", Value: "/var/log/konnectivity-server.log"}, |
There was a problem hiding this comment.
nit: you could write these without Key and Value e.g. {"log-file", "/var/log/konnectivity-server.log"}
e2e/main_test.go
Outdated
| os.Exit(testenv.Run(m)) | ||
| } | ||
|
|
||
| func getMetrics(url string) (map[string]*io_prometheus_client.MetricFamily, error) { |
There was a problem hiding this comment.
thoughts on having this return a friendlier type? Current usage always fetches a gauge so we could have this be getMetrics(url, name) (int, error). That would keep the int(connectionsMetric.GetMetric()[0].GetGauge().GetValue()) complexity all in this func
There was a problem hiding this comment.
Created a getMetricsGaugeValue() func
e2e/multiserver_multiagent_test.go
Outdated
|
|
||
| for _, agentPod := range agentPods.Items { | ||
|
|
||
| metricsFamilies, err := getMetrics(fmt.Sprintf("%v-%v:%v/metrics", agentPod.Name, agentServiceHost, adminPort)) |
There was a problem hiding this comment.
thought on pulling this into a agentsAreConnected() func? That way we could reuse in singleserver_singleagent_test as well
|
@avrittrohwer Addressed your comments. Working on including this in the GitHub workflows now. |
|
/retest |
|
Excluded the e2e tests from the main test runners (fast-test and test) in the Makefile. Hopefully that fixes this. |
|
/retest |
There was a problem hiding this comment.
Could you add a short README.md to the e2e directory that goes over what maint_test.go sets up (RBAC and service) and what each specific test is expected to set up?
There was a problem hiding this comment.
Done, could you take a look?
e2e/README.md
Outdated
There was a problem hiding this comment.
nit: I don't see 'KinD' capitalization used on the linked docs, keep it 'kind' for consistency
|
Hmm, unsure why the tests are failing suddenly. They were working locally. Will do some digging. |
|
/retest |
go.mod
Outdated
| @@ -21,7 +21,9 @@ require ( | |||
| k8s.io/client-go v0.30.2 | |||
| k8s.io/component-base v0.30.2 | |||
| k8s.io/klog/v2 v2.130.0 | |||
| sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.0 | |||
| sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.29.0 | |||
There was a problem hiding this comment.
pleaser restore v0.0.0 to avoid confusion. Note there is a replace directive below.
e2e/metrics_assertions_test.go
Outdated
|
|
||
| metricsParser := &expfmt.TextParser{} | ||
| metricsFamilies, err := metricsParser.TextToMetricFamilies(resp.Body) | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Not critical but I think the defer should really be right after the http.Get error check.
e2e/metrics_assertions_test.go
Outdated
| func getMetricsGaugeValue(url string, name string) (int, error) { | ||
| resp, err := http.Get(url) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not get metrics: %w", err) |
There was a problem hiding this comment.
Debuggability is easier if you include the url in with the error.
e2e/metrics_assertions_test.go
Outdated
| var serverPods *corev1.PodList | ||
| err := client.Resources().List(ctx, serverPods, resources.WithLabelSelector("k8s-app=konnectivity-server")) | ||
| if err != nil { | ||
| t.Fatalf("couldn't get server pods: %v", err) |
There was a problem hiding this comment.
Again debuggability goes up if you have a reference to relevant request (server pods) with the error.
There was a problem hiding this comment.
Added a reference to the label selector being used to discover the pods.
e2e/metrics_assertions_test.go
Outdated
| for _, serverPod := range serverPods.Items { | ||
| numConnections, err := getMetricsGaugeValue(fmt.Sprintf("%v:%v/metrics", serverPod.Status.PodIP, adminPort), "konnectivity_network_proxy_server_ready_backend_connections") | ||
| if err != nil { | ||
| t.Fatalf("couldn't get agent metric 'konnectivity_network_proxy_server_ready_backend_connections' for pod %v", serverPod.Name) |
There was a problem hiding this comment.
nout found is the obvious error but there could be other reasons we got an error here and we did not check the error type. I would suggest logging it.
e2e/metrics_assertions_test.go
Outdated
| } | ||
|
|
||
| if numConnections != expectedConnections { | ||
| t.Errorf("incorrect number of connected agents (want: %v, got: %v)", expectedConnections, numConnections) |
There was a problem hiding this comment.
%v can mask some interesting errors in a case like this. They may be unequal because they are of different types but their toString method generates the same string. If you believe they should both be ints then it is better to use %d.
| {"server-port", "8090"}, | ||
| {"agent-port", "8091"}, | ||
| {"health-port", "8092"}, | ||
| {"admin-port", strconv.Itoa(adminPort)}, |
There was a problem hiding this comment.
Not wrong but its not clear to me why your type converting what appears to be a constant?
There was a problem hiding this comment.
Compiler complains that adminPort is an int if I don't conver it.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avrittrohwer, carreter, cheftako, ipochi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Made some E2E tests that:
kindclusterCurrently do this for two scenarios: 1 agent + 1 server and 3 agents + 3 servers.
Had to run
go mod tidyandgo mod vendoralong the way to pull in some dependencies, couldn't figure out how to do this without bumping the go patch version. Hope this is ok, I can try and rework it if not!