Skip to content
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

feat(k6): add support for k8s loading/unloading of Seldon CRs #5563

Merged
merged 6 commits into from
May 7, 2024

Conversation

lc525
Copy link
Member

@lc525 lc525 commented May 2, 2024

This adds initial support for k8s via the USE_KUBE_CONTROL_PLANE environment variable:

  • models/pipelines/experiments in components/model.js are modified to also return CR yamls
  • updates functions in components/utils.js (including setupBase(...) and teardownBase(...) to use xk6-kubernetes, when configured
  • all scenarios already using setupBase(...) should work unchanged

Which issue(s) this PR fixes:

  • INFRA-949 (internal issue) Extend existing k6 scenarios to use xk6-kubernetes

TODO

  • Test functionality in kind
  • Fix getting model/pipeline/experiment CRs back from k8s
  • Fix model/pipeline/experiment deletion

This adds initial support for k8s via the `USE_KUBE_CONTROL_PLANE` environment
variable:

- models/pipelines/experiments in components/model.js are modified to also
return CR yamls
- updates functions in components/utils.js (including `setupBase(...)` and
`teardownBase(...)` to use xk6-kubernetes, when configured
- all scenarios already using `setupBase(...)` should work unchanged

**Which issue(s) this PR fixes:**
- INFRA-949 (internal issue) Extend existing k6 scenarios to use xk6-kubernetes

- [ ] Test functionality in kind
- [ ] Extend existing k6 scenarios to use xk6-kubernetes
@lc525 lc525 added the v2 label May 2, 2024
@lc525 lc525 changed the title feat(k6): add support for k8s loading/unloading of Seldon CRs feat(k6): add k6 support for k8s loading/unloading of Seldon CRs May 2, 2024
@lc525 lc525 changed the title feat(k6): add k6 support for k8s loading/unloading of Seldon CRs feat(k6): add support for k8s loading/unloading of Seldon CRs May 2, 2024
- works: model creation + load
- fails: getting model status via k8s (permissions)
- fails: deleting/tear down models (permissions)
- name: SCHEDULER_ENDPOINT
value: "${SCHEDULER_ENDPOINT}:9004"
- name: INFER_HTTP_ITERATIONS
value: "1"
- name: INFER_GRPC_ITERATIONS
value: "1"
- name: MODELNAME_PREFIX
value: "tfsimplea,pytorch_cifar10a,tfmnista,mlflow_winea,irisa"
value: "tfsimplea,pytorch-cifar10a,tfmnista,mlflow-winea,irisa"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

names changed because the ones with _ were not getting loaded in k8s

@lc525 lc525 marked this pull request as ready for review May 3, 2024 13:50
@lc525 lc525 requested a review from sakoush as a code owner May 3, 2024 13:50
@@ -97,32 +101,32 @@ export function getModelInferencePayload(modelName, inferBatchSize) {
const shape = [inferBatchSize, 16]
var httpBytes = []
var grpcBytes = []

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes from here to line 208 are linting changes

Copy link
Member

@sakoush sakoush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lovely! this is a great feature moving forward.

I guess the scenarios we care about can use USE_KUBE_CONTROL_PLANE, i.e. infer_constant_rate.js and infer_constant_vu.js.

tests/k6/Makefile Show resolved Hide resolved
@@ -0,0 +1,103 @@
import { Kubernetes } from "k6/x/kubernetes";
import { getConfig } from '../components/settings.js'
import {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the future we might want also to check the status via kube. not a blocker for this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, was already planning for that. The xk6-kubernetes extension is a bit wierd in that it doesn't have built-in functionality for waiting on a given condition/status. Also, it throws exceptions for any unexpected conditions, so I'll have to code that a bit defensively (via repeated resource gets).

}

export function loadModel(modelName, data, awaitReady=true) {
//console.log(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, will clean those up. They were there in the scheduler code so I've left them for dev debugging


export function loadModel(modelName, data, awaitReady=true) {
//console.log(data)
if(!seldonObjExists(seldonObjectType.MODEL, modelName, namespace)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have to check if the model already exists?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a model already exists here we get an exception. The exception likely happens only when the loaded model has the same CR as an existing one (it is an apply after all).

}

export function unloadModel(modelName, awaitReady=true) {
// console.log("Unloading model "+modelName)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

}

export function loadPipeline(pipelineName, data, awaitReady=true) {
//console.log(data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

@@ -1,3 +1,7 @@
// import { dump as yamlDump } from "../import/js-yaml.mjs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

for (var i = 0; i < 16 * inferBatchSize; i++) {
grpcBytes.push("MQ=="); // base64 of 1
httpBytes.push("97")
}
const payload = {
"http": {"inputs":[{"name":"INPUT0","data":httpBytes,"datatype":"BYTES","shape":shape},{"name":"INPUT1","data":httpBytes,"datatype":"BYTES","shape":shape}]},
"grpc": {"inputs":[{"name":"INPUT0","contents":{"bytes_contents":grpcBytes},"datatype":"BYTES","shape":shape},{"name":"INPUT1","contents":{"bytes_contents":grpcBytes},"datatype":"BYTES","shape":shape}]}
"http": { "inputs": [{ "name": "INPUT0", "data": httpBytes, "datatype": "BYTES", "shape": shape }, { "name": "INPUT1", "data": httpBytes, "datatype": "BYTES", "shape": shape }] },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: how did you reformat this file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the automated formatter from intelliJ (we should try to find a cli formatter and add linting/formatting to the makefile)

"storageUri": uri,
"requirements": modelTemplate.requirements,
"memory": (memoryBytes == null) ? modelTemplate.memoryBytes : memoryBytes,
"minReplicas": 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we need minReplicas, it will trigger I think autoscaling of models which we might want to do.

maybe at least align with the grpc api path.

@@ -0,0 +1,5 @@
export const seldonObjectType = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

lc525 added 2 commits May 7, 2024 09:42
- cleanup commented debug calls
- remove minReplicas from CRs
@lc525 lc525 merged commit 5ea6714 into SeldonIO:v2 May 7, 2024
3 checks passed
@lc525 lc525 deleted the INFRA-949/k6-use-k8s branch May 7, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants