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

Enable CI #22

Merged
merged 1 commit into from
Feb 21, 2017
Merged

Enable CI #22

merged 1 commit into from
Feb 21, 2017

Conversation

kirs
Copy link
Contributor

@kirs kirs commented Feb 18, 2017

This is the scrappy version of the CI running for master and branches. Surprisingly, it took me less than an hour to set it up (big thanks to @sander-m for help with BK and VMs) 🎉

Important things:

  • Only Shopify employees can access the CI page, however the CI status is reported back to GH and visible for everyone
  • I didn't enable it for the PRs on purpose because it's a source of vulnerabilities. This is a public project and anyone could submit a PR with malicious code in bin/ci that would be executed on Shopify VMs. Technically the VMs are isolated but still I wouldn't like to give this opportunity to people outside of the company.

How it works:
Mobile CI at Shopify provides disposable Ubuntu VMs with enabled KVM. We have them integrated with BuildKite, and these computing resources are available to run CI for this project. In bin/ci script we install kubectl, minikube, docker kvm driver, and ruby dependencies. Them we simply run rake test.

It takes 2m40s to start minikube, and the complete test suite runs in 20 seconds. This sounds good enough for me, but in future we can speed it up by pre-building the VM template with minikube and all dependencies installed.

Preview:

screen shot 2017-02-17 at 20 16 56

review @sirupsen @KnVerey @ibawt @wfarr

@ibawt
Copy link
Contributor

ibawt commented Feb 18, 2017

:O

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

😻

  • Is it named rubernetes-deploy gem instead of kubernetes-deploy gem on purpose? :trollface:
  • Can we include the pipeline.yml for this in the repo?
  • I'm ok with doing it a bit later, but I think prebuilding the VM template will definitely be worthwhile. The suite takes 20 seconds now, but that's with a single integration test. On my branch that adds ~10 more, we're already up to 70 seconds.

bin/ci Outdated
bundle install --jobs 4

echo "--- Starting minikube"
minikube start
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple of things I'd like to confirm about the defaults we're using here:

  • --cpus is 2 and --memory is 2048. Seems reasonable to me but ¯\_(ツ)_/¯
  • --disk-size is 20g. Seems a bit excessive? MinimumDiskSizeMB = 2000 (from minikube code), and I've been using that much locally.
  • --kubernetes-version is available. Perhaps we should link this to the version we download above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean you'd like to not rely on defaults and run it with minikube start --cpus 2 --memory 2048 --disk-size=2gb --kubernetes-version=x.x.x?

Choose a reason for hiding this comment

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

The idea of specifying this explicitly with values that the Dev Acceleration team is comfortable with (cc @sander-m) sounds good to me. 👍 Any opinions on values Sander?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • --cpu 2 👍 because these VM instances have 2 cores.
  • --memory 2048 The VM has 8GB RAM available, so this is fine but you could bump it if you want
  • --disk-size=2gb You can expect roughly 20GB of free space on the VM, but we can increase this for the custom template if you need more.

bin/ci Outdated

echo "--- Starting minikube"
minikube start
minikube status
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we wait for this to be "Running" before trying to start the tests? Looks like it was immediately in your test runs, but perhaps that wouldn't always be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

minikube start waits until the cluster is running and then connects to it to verify things. So the check shouldn't be required.

If you think the check is necessary, we can put test -n "$(minikube status | grep 'Running')" into the bash script. It will fail if it couldn't grep the status for "Running".

Choose a reason for hiding this comment

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

Why have minikube status at all then? In case it fails? What's the timeout on minikube start in case it fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no timeout in minikube start that I could find. It may be worth adding it - I'll discuss it with the mikube maintainer.

I agree that taking the facts that I mentioned above, minikube status isn't necessary. I'll remove it.

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's also worth mentioning that if in the worst case minikube start gets crazy and starts to take forever, we have the global Buildkite timeout.

@sirupsen
Copy link

I am assuming Shopify employees can trigger a build manually? Can you put that in the README as well to be required? If we ship the gem with ShipIt already, this shouldn't be an issue. I'm really concerned about this test suite getting rusty... complicated VM test suites that rely on multiple versions of things to work together are very prone to rust.

Copy link

@sirupsen sirupsen left a comment

Choose a reason for hiding this comment

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

This is pretty awesome Kir... I'd +1 to all of Katrina's comments. Great work. Thanks @sander-m!

bin/ci Outdated
# KVM support for minikube
curl -Lo docker-machine-driver-kvm https://github.com/dhiltgen/docker-machine-kvm/releases/download/v0.7.0/docker-machine-driver-kvm
chmod +x docker-machine-driver-kvm
sudo mv docker-machine-driver-kvm /usr/local/bin/

Choose a reason for hiding this comment

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

Can you extract all these versions to environment variables? I'd be a little concerned that these things get out of date. How about writing a test that asserts the versions used in this file are the same as we use in development?

Just a sketch in code (haven't run any of this so don't know if it works, but it illustrates an idea):

test "check versions with CI" do
  versions = File.read("omg.sh").scan(/([\w_]+)_VERSION="([\d\.]+)"/)
  versions.each do |(util, version)|
    case util
    when "KUBECTL"
      dev_version =`kubectl version | grep -oP "(?<=v)[0-9]+\.[0-9]+\.[0-9]+"`.strip
      assert_equal dev_version, version
    when "DOCKER_MACHINE"
      # compare somehow
    when "MINIKUBE"
       # compare somehow
    else
       flunk "#{util} was not defined to compare with development..."
    end
  end
end

Alternatively we could enforce this somehow else, e.g. booting test/test_helper.rb to make sure both dev and CI runs against the same versions.

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'm concerned about minikube and kubectl versions too. The problem is that in future they will be installed in the VM template that doesn't have the context of the repo.

I'm going to move versions to environmental variables and in the next PR I'll introduce the version check in test helper.

@kirs
Copy link
Contributor Author

kirs commented Feb 21, 2017

I am assuming Shopify employees can trigger a build manually? Can you put that in the README as well to be required? If we ship the gem with ShipIt already, this shouldn't be an issue. I'm really concerned about this test suite getting rusty... complicated VM test suites that rely on multiple versions of things to work together are very prone to rust.

Shopify employees push code to branches, and the branch builds are automatically triggered. AFAIK it also means that PRs from Shopify employees are also built automatically. It will also build git tags (== gem releases).

Copy link
Contributor

@wfarr wfarr left a comment

Choose a reason for hiding this comment

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

Shopify employees push code to branches, and the branch builds are automatically triggered. AFAIK it also means that PRs from Shopify employees are also built automatically. It will also build git tags (== gem releases).

This was the only thing I was waiting to hear back on. This all looks good to me!

@kirs
Copy link
Contributor Author

kirs commented Feb 21, 2017

Can we include the pipeline.yml for this in the repo?

I had exactly the same suggestion, but then we discussed it with @sander-m and came to the conclusion that since CI is a private thing, in this case we can keep the config in BK and don't expose it to the public.

I'm ok with doing it a bit later, but I think prebuilding the VM template will definitely be worthwhile. The suite takes 20 seconds now, but that's with a single integration test. On my branch that adds ~10 more, we're already up to 70 seconds.

Sure, working on the VM template is one of the next things in my list.

@sirupsen
Copy link

Shopify employees push code to branches, and the branch builds are automatically triggered. AFAIK it also means that PRs from Shopify employees are also built automatically. It will also build git tags (== gem releases).
This was the only thing I was waiting to hear back on. This all looks good to me!

That's... magic. ❇️

@sander-m
Copy link
Contributor

LGTM. I can help out with the custom template.

@kirs
Copy link
Contributor Author

kirs commented Feb 21, 2017

I've addressed all concerns that were mentioned. Please let me know if there's any other feedback.

I have another updated about Buildkite:

BK will build branches as you're working on them, but as soon as you create a PR it will stop building new commits from that branch because it's a PR and we disabled PR builds. The workaround for that is to trigger the build manually, which is very easy:

screen shot 2017-02-21 at 10 55 33

@ibawt
Copy link
Contributor

ibawt commented Feb 21, 2017

fyi @stefanmb

Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

BK will build branches as you're working on them, but as soon as you create a PR it will stop building new commits from that branch because it's a PR and we disabled PR builds. The workaround for that is to trigger the build manually, which is very easy:

Can we add this to the readme in a "maintainers" section or something? Otherwise LGTM.

README.md Outdated
@@ -1,5 +1,7 @@
# Kubernetes::Deploy

[![Build status](https://badge.buildkite.com/0f2d4956d49fbc795f9c17b0a741a6aa9ea532738e5f872ac8.svg)](https://buildkite.com/shopify/rubernetes-deploy-gem)
Copy link
Contributor

Choose a reason for hiding this comment

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

kubernetes-deploy-gem (I see you fixed it in buildkite)

@kirs
Copy link
Contributor Author

kirs commented Feb 21, 2017

🎉🎉🎉

@KnVerey great suggestions. I've fixed both things.

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.

6 participants