Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Add logic to generate bazel + gcloud container using rules_docker #193

Merged
merged 10 commits into from
Oct 22, 2018

Conversation

smukherj1
Copy link
Contributor

For now only added logic to create the container. I haven't uploaded it to GCR because I don't have permissions to access gcr.io/rules-k8s.

The container built by this logic doesn't fully work because gcloud installation has some post processing steps after the extraction of the downloaded tarball. I tried to use bootstrap_image_macro from base_images_docker but ran into some issues. I commented out the call and I explain my issue in comments above the invocation of bootstrap_image_macro. The installation cleanup commands in bootstrap_image_macro show what's needed to fully install gcloud and kubectl in this container.

If this container looks acceptable, the next steps will be to upload this container to GCR and change the prow job definition to use this container. At the same time, the e2e_test.sh will need to be changed to run the post installation steps before running any tests

@xingao267
Copy link
Member

xingao267 commented Oct 11, 2018

@smukherj1 We have a bazel_docker_gcloud container using rules_docker here https://github.com/bazelbuild/bazel-toolchains/blob/master/container/ubuntu16_04/builders/bazel_docker_gcloud/BUILD

You could reuse or modify (if needed) the layer targets in the bazel_toolchains repo, and then refer to those targets in this repo?

@xingao267 xingao267 self-requested a review October 11, 2018 17:54
@fejta
Copy link
Contributor

fejta commented Oct 11, 2018

This seems kind of ridiculously complicated compared to the Dockerfile...

@nlopezgi
Copy link
Contributor

Hi Erick,
We are aware that currently using rules_docker to build this type of container is indeed more complicated than using a Dockerfile. But Dockerfiles are really not good at all from any perspective other than being easy to write. e.g.,:

  • rebuilding the image every time is makes CI painfully slow for reasons totally unrelated to what actually has to be tested
  • creating an image with a dockerfile and uploading it to gcr would mean that a 2gb image needs to be pulled for each CI run
  • each time you build an image you get a different result
    It also seems a bit odd that we, owners of rules_docker that advocate it can build better images than with Dockerfiles, are not using it to build our own images.

We're working towards making use of rules_docker for this kind of use case provide significant benefits w/o it being so hard. The use case here for Prow CI is perfect for what we want our users to be able to do: make it so that its possible to build an image that composes existing images (in this case bazel+gcloud) using only rules_docker. We're not there yet though (and this PR will likely need to go through lots of changes before we merge it). We can sync up if you want to know more about our plans, but consider this PR work in progress (and obviously please help us with any feedback you consider relevant!)
Thanks again for all your help with this repo!


load("@io_bazel_rules_docker//container:image.bzl", "container_image")
load("@io_bazel_rules_docker//container:layer.bzl", "container_layer")
#load("@base_images_docker//package_managers:bootstrap_image.bzl",
Copy link
Member

Choose a reason for hiding this comment

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

remove code that are commented out

@fejta
Copy link
Contributor

fejta commented Oct 15, 2018

rebuilding the image every time is makes CI painfully slow for reasons totally unrelated to what actually has to be tested

Right now we use the same image for each run. Why rebuild the image?

It also seems a bit odd that we, owners of rules_docker that advocate it can build better images than with Dockerfiles, are not using it to build our own images.

Agree with this generally and am a big proponent of rules_docker generally over in the kubernetes space.

The bazel layer looks like a great change. How about changing the Dockerfile to be FROM launcher.gcr.io/google/bazel instead of ubuntu?

The gcloud part of this change doesn't look so good right now. Instead of the functional image we have today, we'll wind up publishing an image that does not have gcloud installed, requiring the user to run /google/cloud-sdk/install.sh -q && source /google-cloud-sdk/path.bash.inc" && python -m pip install --upgrade pip setuptools wheel && /google-cloud-sdk/bin/gcloud components install --quiet kubectl whenever they start a container with this image.

Why don't we continue to use the Dockerfile to install gcloud until rules_docker and/or bazel can handle installing gcloud more gracefully?

@smukherj1
Copy link
Contributor Author

smukherj1 commented Oct 16, 2018

@fejta @nlopezgi Trying to see if I can use container_run_and_commit from @base_images_docker/utils:run.bzl to produce a container that is actually usable. In addition to that, I like the idea of updating the "FROM" in the Dockerfile to the bazel image as you suggested and remove bazel from the list of packages being installed.

Unfortunately, container_run_and_commit currently has a bug that I'm looking into fixing

@fejta
Copy link
Contributor

fejta commented Oct 16, 2018

Sweet, thanks for all the work here. Despite my comments about what isn't working well yet I love all the work you and your team are doing with this repo ❤️

For the container produced using rules_docker, use run_and_commit to
run the post install steps
@fejta
Copy link
Contributor

fejta commented Oct 17, 2018

Dockerfile changes LGTM. If you want to push that image and send me the new tag I'll update https://github.com/kubernetes/test-infra/blob/master/config/jobs/bazelbuild/rules_k8s/rules_k8s_config.yaml to make sure it still works.

@smukherj1
Copy link
Contributor Author

Thanks for the review @fejta. Unfortunately, I don't have write permissions to the gcr.io/rules_k8s repo. I'm working on creating mdb groups for our team to have access to it. Would you like me to set that up first or should I upload the image to a different repo I do have access to for the tests?

@fejta
Copy link
Contributor

fejta commented Oct 17, 2018

BTW are we familiar with kaniko? Allows creating images without docker but still using a Dockerfile: https://github.com/GoogleContainerTools/kaniko

@smukherj1
Copy link
Contributor Author

@fejta Yes our team is also looking into kaniko and gVisor. As for testing, I just uploaded a new container built using the Dockerfile as gcr.io/rules-k8s/gcloud-bazel:latest. Could you please try it out?

images/gcloud-bazel/BUILD.bazel Outdated Show resolved Hide resolved
images/gcloud-bazel/BUILD.bazel Outdated Show resolved Hide resolved
images/gcloud-bazel/BUILD.bazel Outdated Show resolved Hide resolved
images/gcloud-bazel/BUILD.bazel Outdated Show resolved Hide resolved
images/gcloud-bazel/BUILD.bazel Outdated Show resolved Hide resolved
images/gcloud-bazel/BUILD.bazel Outdated Show resolved Hide resolved
images/gcloud-bazel/Dockerfile Outdated Show resolved Hide resolved
@fejta
Copy link
Contributor

fejta commented Oct 17, 2018

just uploaded a new container built using the Dockerfile as gcr.io/rules-k8s/gcloud-bazel:latest

a) Did you use make push to trigger the GCB run that publishes the image? Please let me know the tagged version, something like gcr.io/rules-k8s/gcloud-bazel:v20180708-d6e1b65. We don't want to use images built on our workstations.
b) Would you want to send the test-infra PR? (I can review) Happy to do it if you'd prefer

@smukherj1
Copy link
Contributor Author

@fejta No I did not use make push. I will address Nick's comments and upload a new commit and use make push to trigger the GCB run as well as submit the other PR

# Pod spec: https://github.com/kubernetes/test-infra/blob/master/config/jobs/bazelbuild/rules_k8s/rules_k8s_config.yaml
container_push(
name="image-push",
image=":image_commit.tar",
name="gcloud-push",
Copy link
Contributor

Choose a reason for hiding this comment

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

gcloud_push for consistency

Copy link
Contributor

@nlopezgi nlopezgi left a comment

Choose a reason for hiding this comment

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

minor nit, but looks good overall

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 17, 2018
@nlopezgi
Copy link
Contributor

You can probably exclude the failing targets from running on mac (//images/gcloud-bazel:gcloud_install, //images/gcloud-bazel:gcloud-push) in the .bazelci/presubmit-yaml file; the travis error does look like an issue you need to solve

@smukherj1
Copy link
Contributor Author

Submitted kubernetes/test-infra#9841 to update the CI pod spec. I think the pull-rules test here is failing because I'm using a new attribute in the http_file rule. Hopefully the new image won't have this issue

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 17, 2018
@smukherj1
Copy link
Contributor Author

@fejta I think the latest prow failure might be due to an updated kubernetes version that deprecates some command. Specifically the error is in the hellogrpc e2e tests
error: there is no need to specify a resource type as a separate argument when passing arguments in resource/name form (e.g. 'kubectl get resource/<resource_name>' instead of 'kubectl get resource resource/<resource_name>'

Not sure where exactly this kubectl command is located

@fejta
Copy link
Contributor

fejta commented Oct 18, 2018

I believe kubectl command comes from your workstation. It is called from scripts like this: https://github.com/bazelbuild/rules_k8s/blob/master/k8s/apply.sh.tpl

@fejta
Copy link
Contributor

fejta commented Oct 18, 2018

Installing gcloud should also install kubectl, so that's probably where we're getting it.

@fejta
Copy link
Contributor

fejta commented Oct 18, 2018

Maybe the issue is here?

get_lb_ip() {
kubectl --namespace=${USER} get service hello-grpc-staging \
-o jsonpath='{.status.loadBalancer.ingress[0].ip}'
}

Aka should be get service/hello-grpc-staging rather than get service hello-grpc-staging

@fejta
Copy link
Contributor

fejta commented Oct 18, 2018

Let me work on that, will send you PR

@ixdy
Copy link

ixdy commented Oct 18, 2018

Not sure where exactly this kubectl command is located
I believe kubectl command comes from your workstation.

arguably kubectl should probably be declared in a toolchain, since that would let you explicitly pin to versions, but that's an issue for another PR.

@nlopezgi
Copy link
Contributor

@ixdy a toolchain rule for k8s is coming soon

@smukherj1
Copy link
Contributor Author

/retest

1 similar comment
@smukherj1
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, nlopezgi, xingao267

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:
  • OWNERS [fejta,nlopezgi,xingao267]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nlopezgi nlopezgi merged commit db7f731 into bazelbuild:master Oct 22, 2018
@smukherj1 smukherj1 deleted the prow_bazel_container branch October 23, 2018 12:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants