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

Run E2E tests on GCE #302

Merged

Conversation

kosyfrances
Copy link
Contributor

@kosyfrances kosyfrances commented Jun 20, 2018

This PR does the following:

  • Downloads gcloud
  • Authenticates using secrets
  • Creates cluster on GKE
  • Pushes docker image to GCR
  • Runs e2e test on the GKE cluster, using the image from GCR

Fixes #282

To be done: Document steps for users

@kosyfrances kosyfrances self-assigned this Jun 20, 2018
@kosyfrances kosyfrances changed the title Run E2E tests on GCE WIP: Run E2E tests on GCE Jun 20, 2018
@kosyfrances kosyfrances force-pushed the asymmetric/circle-deploy branch 2 times, most recently from a180260 to 7008477 Compare June 21, 2018 11:30
@kosyfrances kosyfrances changed the title WIP: Run E2E tests on GCE Run E2E tests on GCE Jun 21, 2018

# setup SDK
gcloud auth activate-service-account --key-file=${HOME}/gcloud-service-key.json
gcloud config set project $GCLOUD_PROJECT_ID
Copy link
Contributor

Choose a reason for hiding this comment

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

The GCLOUD_PROJECT_ID variable probably also is set in the web UI, just like GCLOUD_SERVICE_KEY, right? If so, add a comment about, please.

- run:
name: install google cloud sdk
environment:
GCLOUD_COMPUTE_ZONE: europe-west1-b
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be used only in gcloud config set compute/zone GCLOUD_COMPUTE_ZONE so maybe just drop the variable?

You can keep it though, if this helps in you in writing a documentation.

- run:
name: Push the docker image
environment:
GCLOUD_COMPUTE_ZONE: europe-west1-b
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this env var here necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still to be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it has already been addressed ... 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, in a separate place, where you dropped this env var in favor of just calling gcloud config set compute/zone europe-west1-b directly.

What I mean here is that this env var is not used in this step. The step is about pushing a docker image to some google docker registry and here the env var is not needed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the env var sets the default compute zone for the project. I just saw that I was setting it again where I was pushing the docker image so I will be taking that out instead and keeping gcloud config set compute/zone europe-west1-b

@@ -3,6 +3,8 @@ jobs:
build:
machine: true
working_directory: ~/.go_workspace/src/github.com/habitat-sh/habitat-operator
environment:
IMAGE_NAME: habitat/habitat-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Would setting IMAGE_NAME to eu.gcr.io/$GCLOUD_PROJECT_ID/$IMAGE_NAME-$CIRCLE_BUILD_NUM be better? I can see it could at least avoid creating a separate tag for the generated image and likely save you from repeating the image name for gcr in some places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did set it initially to eu.gcr.io/$GCLOUD_PROJECT_ID/habitat/habitat-operator-$CIRCLE_BUILD_NUM but the $CIRCLE_BUILD_NUM variable was not interpolated and I am not really sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would that thing work? https://circleci.com/docs/2.0/env-vars/#setting-an-environment-variable-in-a-shell-command In our case we could have something like the following as the very first step:

- run:
  name: Setup environment
  command: |
    echo 'export IMAGE_NAME=eu.gcr.io/$GCLOUD_PROJECT_ID/habitat/habitat-operator-$CIRCLE_BUILD_NUM' >>"${BASH_ENV}"
    echo 'export IMAGE_TAG=testing' >>"${BASH_ENV}"
    echo 'export IMAGE_FULL_NAME="${IMAGE_NAME}:${IMAGE_TAG}"' >>"${BASH_ENV}"

If that works, that it is a great way to avoid some repetitions (like repeating this long-winded docker image name or the "testing" tag in some places), and we could use it to tell make image to generate a docker image that contains the operator directly with the name we really want instead of creating the habitat-sh/habitat-operator image and then tagging it under something else. Also, you could then drop this part:

environment:
  IMAGE_NAME: habitat/habitat-operator

name: boot cluster on gke
environment:
K8S_VERSION: 1.10.4-gke.2
command: gcloud container clusters create --cluster-version=$K8S_VERSION --disk-size=20 operator-test-$CIRCLE_BUILD_NUM
- run:
name: create image
command: make TAG=testing image
Copy link
Contributor

Choose a reason for hiding this comment

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

You could call make IMAGE=${IMAGE_NAME} TAG=testing image if you follow my suggestion for the IMAGE_NAME env var.

name: Push the docker image
environment:
GCLOUD_COMPUTE_ZONE: europe-west1-b
command: docker push eu.gcr.io/$GCLOUD_PROJECT_ID/$IMAGE_NAME-$CIRCLE_BUILD_NUM:testing
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use ${IMAGE_NAME}:testing if you follow my suggestion about the IMAGE_NAME env var.

name: Configure docker to use gcloud to authenticate requests to Container Registry
command: gcloud auth configure-docker
- run:
name: Tag the docker image
Copy link
Contributor

Choose a reason for hiding this comment

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

This step could be dropped if you follow my suggestion about the IMAGE_NAME env var.

- run:
name: boot cluster on gke
environment:
K8S_VERSION: 1.10.4-gke.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename it to GKE_K8S_VERSION? I was a tiny bit confused if it doesn't clash with the K8S_VERSION env var from setup step.

- run:
name: e2e tests
command: make TESTIMAGE=habitat/habitat-operator:testing e2e
command: make TESTIMAGE=eu.gcr.io/$GCLOUD_PROJECT_ID/$IMAGE_NAME-$CIRCLE_BUILD_NUM:testing e2e
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use IMAGE_NAME if you followed yadda yadda.

# - when: on_fail
- run:
name: Delete image from Container Registry
command: gcloud container images delete eu.gcr.io/$GCLOUD_PROJECT_ID/$IMAGE_NAME-$CIRCLE_BUILD_NUM:testing --force-delete-tags --quiet
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use ${IMAGE_NAME}:testing if you followed my suggestion about the IMAGE_NAME env var.

@kosyfrances kosyfrances force-pushed the asymmetric/circle-deploy branch 2 times, most recently from 657bef0 to b4bf3eb Compare June 28, 2018 16:27
Copy link
Contributor

@krnowak krnowak left a comment

Choose a reason for hiding this comment

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

Got another idea for reducing the image name and tag repetitions, hopefully it will work. Also, go code could be simpler I think.

@@ -3,6 +3,8 @@ jobs:
build:
machine: true
working_directory: ~/.go_workspace/src/github.com/habitat-sh/habitat-operator
environment:
IMAGE_NAME: habitat/habitat-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Would that thing work? https://circleci.com/docs/2.0/env-vars/#setting-an-environment-variable-in-a-shell-command In our case we could have something like the following as the very first step:

- run:
  name: Setup environment
  command: |
    echo 'export IMAGE_NAME=eu.gcr.io/$GCLOUD_PROJECT_ID/habitat/habitat-operator-$CIRCLE_BUILD_NUM' >>"${BASH_ENV}"
    echo 'export IMAGE_TAG=testing' >>"${BASH_ENV}"
    echo 'export IMAGE_FULL_NAME="${IMAGE_NAME}:${IMAGE_TAG}"' >>"${BASH_ENV}"

If that works, that it is a great way to avoid some repetitions (like repeating this long-winded docker image name or the "testing" tag in some places), and we could use it to tell make image to generate a docker image that contains the operator directly with the name we really want instead of creating the habitat-sh/habitat-operator image and then tagging it under something else. Also, you could then drop this part:

environment:
  IMAGE_NAME: habitat/habitat-operator

name: boot cluster on gke
environment:
GKE_K8S_VERSION: 1.10.4-gke.2
command: gcloud container clusters create --cluster-version=$K8S_VERSION --disk-size=20 operator-test-$CIRCLE_BUILD_NUM
Copy link
Contributor

Choose a reason for hiding this comment

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

Should --cluster-version receive ${GKE_K8S_VERSION} instead of $K8S_VERSION?

- run:
name: Push the docker image
environment:
GCLOUD_COMPUTE_ZONE: europe-west1-b
Copy link
Contributor

Choose a reason for hiding this comment

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

Still to be addressed.

}

// WaitForLoadBalancerIP waits for Load Balancer IP to be available
func (f *Framework) WaitForLoadBalancerIP(serviceName string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you just drop the GetServiceIP function and change the WaitForLoadBalancerIP function to return (string, error)? You could do it with something like:

address := ""
err := wait.Poll(…, func() (bool, error) {
    …
    if len(…) == 0 {
        return false, nil
    }

    address = service.Status.LoadBalancer.Ingress[0].IP
    return true, nil
}
return address, err

Maybe you will want to return service.Spec.ClusterIP if wait.Poll returns an error, but not sure how useful it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can drop service.Spec.ClusterIP because the test will still fail if it does not have the LoadBalancerIP itself

@@ -100,10 +100,19 @@ func TestBind(t *testing.T) {
t.Fatal(err)
}

// Wait until Load Balancer IP is ready
if err := framework.WaitForLoadBalancerIP(svc.ObjectMeta.Name); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could move this call to happen after time.Sleep(serviceStartupWaitTime), because right now we wait for the load balancer some time and then we unconditionally sleep for a fixed amount of time for the service to be ready. But the service is probably already ready by the time we get the load balancer IP. With the change, we can make the TestBind test a wee bit faster, because we will sleep for the service to be ready and maybe the load balancer will get the address at the same time and make the polling to last shorter.

@kosyfrances kosyfrances force-pushed the asymmetric/circle-deploy branch 5 times, most recently from 7e56e55 to 4bc2cdb Compare July 2, 2018 16:56
@kosyfrances
Copy link
Contributor Author

kosyfrances commented Jul 2, 2018

@krnowak it is ready for another review 😅

Copy link
Contributor

@krnowak krnowak left a comment

Choose a reason for hiding this comment

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

Looks good, just two small nitpicks and we can merge it.


# setup SDK
gcloud auth activate-service-account --key-file=${HOME}/gcloud-service-key.json
gcloud config set project $GCLOUD_PROJECT_ID # GCLOUD_PROJECT_ID is set in Circle CI web UI
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this # GCLOUD_PROJECT_ID is set … comment to a separate line? Ideally above this one.

- run:
name: create image
command: make TAG=testing image
command: make IMAGE=$IMAGE_NAME TAG=testing image
Copy link
Contributor

Choose a reason for hiding this comment

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

TAG=$IMAGE_TAG please.

To get e2e tests to run on GCE, we do the following:
download gcloud, authenticate using secrets,
create cluster on GKE, push docker image to GCR,
and finally run the e2e test on the GKE cluster, using the image from GCR

Co-authored-by: Lorenzo Manacorda <lorenzo@kinvolk.io>

Signed-off-by: Kosy Anyanwu <kosy@kinvolk.io>
Signed-off-by: Kosy Anyanwu <kosy@kinvolk.io>
@krnowak
Copy link
Contributor

krnowak commented Jul 3, 2018

LFAD.

@krnowak krnowak merged commit 3eab922 into habitat-sh:master Jul 3, 2018
@krnowak krnowak deleted the asymmetric/circle-deploy branch July 3, 2018 13:59
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 this pull request may close these issues.

2 participants