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

upgrade to ksonnet 0.12.0, jsonnet v0.11.2 in the Docker image for our test workers #1540

Closed
kkasravi opened this issue Sep 16, 2018 · 20 comments · Fixed by kubeflow/testing#201
Assignees

Comments

@kkasravi
Copy link
Contributor

issue #1534 requires a version of workflows/k8s.libsonnet that includes apiextensions
This would be in {ksonnet 0.12.0, jsonnet v0.11.2}

Are we able to upgrade to ksonnet 0.12.0?
I wasn't able figure out where to upgrade ksonnet other that what was already done
for the web backend #1508

@kkasravi
Copy link
Contributor Author

/assign @pdmack

@kkasravi
Copy link
Contributor Author

/assign @kunmingg

@kkasravi
Copy link
Contributor Author

/cc @wbuchwalter

@kkasravi
Copy link
Contributor Author

/cc @jlewi

@jlewi
Copy link
Contributor

jlewi commented Sep 17, 2018

The ksonnet version is installed in our test image.
https://github.com/kubeflow/testing/blob/master/images/Dockerfile#L92

So we'd need to update that and then build and push a new version of that image.

I'm unclear though why #1534 requires a ksonnet update.

Where is k8s.libsonnet being used? And where are apiextensions being used?

@jlewi
Copy link
Contributor

jlewi commented Sep 17, 2018

Ok I see; it looks like #1534 is refactoring TFJob to make heavy use of k.libsonnet and that requires apiextensions for defining the CRD.

I know we were discussing whether or not to use k.libsonnet in other issues; where did we land on this?

@jlewi
Copy link
Contributor

jlewi commented Sep 17, 2018

I think this is one PR where we were discussing this
#1437 (comment)

@kkasravi
Copy link
Contributor Author

You had said that for some of the libraries which aren't normally user-facing, the use of k.libsonnet would be ok. Others that have exposure to non-ksonnet experts we shouldn't go overboard on k.libsonnet, k8s.libsonnet.

@kkasravi
Copy link
Contributor Author

I can update the Dockerfile and submit it as a PR to this issue

@kkasravi
Copy link
Contributor Author

see kubeflow/testing#201

@jlewi
Copy link
Contributor

jlewi commented Sep 17, 2018

You had said that for some of the libraries which aren't normally user-facing, the use of k.libsonnet would be ok. Others that have exposure to non-ksonnet experts we shouldn't go overboard on k.libsonnet, k8s.libsonnet.

You are correct. I still am uncertain about what the best practice/style for using k.libsonnet is. I apologize if I wasn't clear about this.

Your PR #1535 was helpful to spark some initial thoughts. See comment:
#1535 (comment)

@jlewi jlewi changed the title upgrade to ksonnet 0.12.0, jsonnet v0.11.2 upgrade to ksonnet 0.12.0, jsonnet v0.11.2 in the Docker image for our test workers Sep 17, 2018
@jlewi
Copy link
Contributor

jlewi commented Sep 17, 2018

kubeflow/testing#201 is merged I'll build a new Docker image with the changes.

@jlewi
Copy link
Contributor

jlewi commented Sep 17, 2018

I built new docker images
gcr.io/kubeflow-ci/test-worker:v20180917-b094114-e3b0c4
gcr.io/kubeflow-releasing/worker:v20180917-b094114-e3b0c4

These are now tagged latest as well.

@jlewi jlewi closed this as completed Sep 17, 2018
@jlewi
Copy link
Contributor

jlewi commented Sep 18, 2018

Looks like this caused a bunch of presubmit failures; error message is

INFO|2018-09-18T00:51:05|/src/kubeflow/testing/py/kubeflow/testing/util.py|62| time="2018-09-18T00:51:05Z" level=warning msg="Versioned packages stored in unversioned paths - please run `ks upgrade` to correct."

@jlewi jlewi reopened this Sep 18, 2018
@jlewi
Copy link
Contributor

jlewi commented Sep 18, 2018

Looks like the previous images were
gcr.io/kubeflow-ci/test-worker:v20180822-da7cefc-e3b0c4
gcr.io/kubeflow-releasing/worker:v20180822-da7cefc-e3b0c4

Adding the latest tag back to those images.

see: kubeflow/testing#196

@jlewi
Copy link
Contributor

jlewi commented Sep 18, 2018

It looks like if we want to upgrade to ksonnet 0.12 we will need to upgrade to the ksonnet app used for our tests.

I think the way to handle this is we can upgrade individual tests to change the test-worker image to use the image
gcr.io/kubeflow-ci/test-worker:v20180917-b094114-e3b0c4

rather than the "latest" tag. This way the presubmit will verify the tests pass with the new image.

Ideally, the test worker image should be a ksonnet parameter that we set it in the prow_config.yaml
https://github.com/kubeflow/kubeflow/blob/master/prow_config.yaml.

The reason for setting it in prow_config.yaml and not the jsonnet file is that makes it easier to see its consistently set across all E2E tests. It looks like a lot of our E2E tests though don't expose it as a parameter.

An example is the tf-operator E2E tests
https://github.com/kubeflow/tf-operator/blob/master/prow_config.yaml#L17

@kunmingg
Copy link
Contributor

I feel that ideally our test workflows shouldn't have dependency on ks versions.
How about turn test workflows into prototypes and e2e test will generate from them on the fly?

@jlewi
Copy link
Contributor

jlewi commented Sep 28, 2018

@kunmingg isn't dynamically creating ksonnet apps going to be a lot more difficult?

run_e2e_workflow.py can figure out which ksonnet version to use just by looking at app.yaml in the app dir specified in prow_config.yaml.We can install multiple versions of ksonnet in the same Docker image e.g
ks-11
ks-12

This seems like a fairly tractable way to deal with ksonnet's app versioning.

@kkasravi
Copy link
Contributor Author

@jlewi @kunmingg @ankushagarwal seems like kfctl.sh also needs this logic

function check_install() {
  if ! which "${1}" &>/dev/null; then
    echo "You don't have ${1} installed. Please install ${1}."
    exit 1
  fi
}
# TODO(ankushagarwal): verify ks version is higher than 0.11.0
check_install ks

and should ask the user to install the latest ks version.

@jlewi
Copy link
Contributor

jlewi commented Oct 8, 2018

@kkasravi can you open up a separate issue for that?

For our E2E tests; this should be fixed by kubeflow/testing#228

ksonnet 0.11 and ksonnet 0.12 should now be installed in our test image and run_e2e_workflow.py will automatically select the correct version based on the ksonnet app.

@jlewi jlewi closed this as completed Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants