disruptors: error out if there are no targets #267
Conversation
roobre
force-pushed
the
error-no-targets
branch
from
583e992
to
5bfbb82
Compare
|
Unit tests for API fail because the test setup does not initialize the fake Kubernetes with any resource. It should create a service backed by a pod. However, as the fake Kubernetes does not update the container's status, the disruptor constructor would still fail because the container never gets ready. The test setup could create the Pod with the agent's ephemeral container already added. This is a valid initial configuration because the disruptor checks if the container exists before injecting it. |
roobre
force-pushed
the
error-no-targets
branch
3 times, most recently
from
09916a0
to
4a2435d
Compare
roobre
changed the title
|
Marking this as draft again as I think this case can be covered by unit tests only. |
roobre
force-pushed
the
error-no-targets
branch
from
4a2435d
to
468f427
Compare
|
I've removed the E2E tests checking for this case in 8bdaaf3 as I think the unit tests should give enough coverage for this use case. @pablochacin LMK what you think about this, I'm fine with bringing them back if you think they're useful. Other than that, this should be ready to review. |
pkg/disruptors/pod.go
Outdated
| if len(p.Select.Labels) != 0 { | ||
| str += "including(" | ||
| for k, v := range p.Select.Labels { | ||
| str += fmt.Sprintf("%s=%s, ", k, v) | ||
| } | ||
| str = strings.TrimSuffix(str, ", ") | ||
| str += "), " | ||
| } | ||
|
|
||
| if len(p.Exclude.Labels) != 0 { | ||
| str += "excluding(" | ||
| for k, v := range p.Exclude.Labels { | ||
| str += fmt.Sprintf("%s=%s, ", k, v) | ||
| } | ||
| str = strings.TrimSuffix(str, ", ") | ||
| str += "), " | ||
| } |
There was a problem hiding this comment.
These two blocks seem identical except for the prefix ("excluding", "including"). Maybe it worth using a small function that returns the labels enclosed in the prefix or null is len if 0
There was a problem hiding this comment.
I didn't do this initially because I was afraid that the function declaration boilerplate was going to be larger than just duplicating, but after trying it I actaully think it does look nicer. LMK what think: 521437a
roobre
force-pushed
the
error-no-targets
branch
2 times, most recently
from
eab43c3
to
521437a
Compare
roobre
force-pushed
the
error-no-targets
branch
from
521437a
to
9d36af2
Compare
Description
This PR adds logic to error out if a
PodDisruptoror aServiceDisruptordo not have any targets, which would make injection noop.To avoid making the error too obscure, I've also added a bit of logic to render a
PodSelectoras a nicely formatted string.As a side effect of making the
PodDisruptorandServiceDisruptorconstructors return an error, the JS API tests (github.com/grafana/xk6-disruptor/pkg/api) would fail. To work around this, the setup code for the JS API tests now initizlize the K8s fake client with a service and a pod backing it.Fixes #247.
Checklist:
make lint) and all checks pass.make test) and all tests pass.make e2e-xxxforagent,disruptors,kubernetesorclusterrelated changes)