Ensures that service is compatible with machine #78
Conversation
If the requested service is "ndt7" or "ndt7_client", then the Autojoin API should not return, for example, Wehe servers. Prometheus (well, gcp-service-discovery) queries the Autojoin API for script-exporter targets for ndt7 e2e testing. The list endpoint should not return machines running experiments that are not compatible with the requested service.
Pull Request Test Coverage Report for Build 20797505529Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
robertodauria
left a comment
robertodauria
left a comment
There was a problem hiding this comment.
@robertodauria reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nkinkade).
handler/handler.go line 451 at r1 (raw file):
continue } if service != "" && !strings.HasPrefix(service, h.Service) {
Can you say more about why this checks the prefix and not exact matches?
Please also double-check that it's working as intended - HasPrefix takes the full string as first argument and the prefix as second.
If this is correct, I think it's worth commenting that service=ndt7_client should still return servers with service=ndt7 and why.
|
@roberto: I'm not 100% sure why, but for some reason when Stephen was setting up script_exporter targets for the Autojoin clients he set the service name for gcp-service-discovery to be ndt7_client, instead of just ndt7: https://github.com/m-lab/prometheus-support/blob/main/k8s/autojoin/deployments/prometheus.yml#L111 But he uses service "ndt7" for blackbox targets. I wasn't 100% sure why he had done that, so rather than try to figure it out and undo those changes, I just decided to match on the "ndt7" prefix, which should work for both services "ndt7" and "ndt7_client", and should also potentially work for other service types down the road. My test that it was working as expected, was to query the Autojoin API after I pushed my changes and the sandbox Autojoin API instance redeployed with my changes, and then to make sure that my test wehe BYOS deployment was no longer in the list, which it wasn't: But if you query Autojoin API without "service=ndt7[_client]", the wehe server is listed: https://autojoin-dot-mlab-sandbox.appspot.com/autojoin/v0/node/list?format=script-exporter |
|
@nkinkade Understood. Could you please explain it in a code comment? |
See the change in this commit to understand what the comment was added for, which may not have previously been obvious.
|
@robertodauria: I added an explanatory comment. Let me know whether that is sufficient for you, and anyone else, to understand that block of code. |
If the requested service is "ndt7" or "ndt7_client", then the Autojoin API should not return, for example, Wehe servers.
Prometheus (well, gcp-service-discovery) queries the Autojoin API for script-exporter targets for ndt7 e2e testing. The list endpoint should not return machines running experiments that are not compatible with the requested service.
This wasn't necessary before, but we are investigating running experiments other than ndt7 on BYOS machines. Therefore, the Autojoin API should only return ndt BYOS machines for ndt7 e2e testing.
This change is