Skip to content
This repository has been archived by the owner on May 7, 2021. It is now read-only.

Flight #940

Merged
merged 17 commits into from
Nov 9, 2018
Merged

Flight #940

merged 17 commits into from
Nov 9, 2018

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Nov 5, 2018

Add Flight object, which serves as a parent for all Clusters in a kola run. Make the Flight responsible for platform infrastructure that shouldn't be recreated per Cluster.

Move the following into Flight:

  • Platform API objects
  • SSH agent and lifecycle of platform SSH keys
  • On qemu: dnsmasq, discovery etcd, and NTP server
  • On qemu: kola mkimage functionality

Fixes #803.

@bgilbert
Copy link
Contributor Author

bgilbert commented Nov 5, 2018

The GitHub UI reordered the commits. The correct order is:

  • 8dd2bdf platform: rename base.go to cluster.go
  • 8b0d050 platform: move logger into platform.go
  • 1442ed7 platform: add Name() to Cluster interface
  • 769d74c platform: add Flight
  • 1f2cfa5 kola: drop rate-limiting hack
  • 66c0840 *: don't hardcode hostport of local Omaha server
  • 967429c platform/local: dynamically select Omaha listen port
  • 058e1e6 platform/local: only create one network segment
  • d05c5c0 platform/local: increase interface limit beyond 8 bits
  • 68e9853 platform/local: use one netns per flight
  • 3cf1b50 platform: add Flight.Name()
  • aee0698 platform: move SSH agent into flight
  • 980170e platform/qemu: move disk-related code into disk.go
  • ccaf811 platform/qemu: unexport Disk methods
  • 8a7cdf9 cmd/kola: drop mkimage subcommand
  • 90faf14 platform/qemu: add helper function to make safe tempfile name
  • bec530c platform/qemu: enable console logging in CL images by default

BaseCluster already implements it.  harness.H does too, and since
cluster.TestCluster also does, we need to disambiguate every reference
to TestCluster.Name().
Add Flight interface and corresponding types as the parent object of a
Cluster.  Move cloud API objects into flight types, allowing them (and
their rate-limiting mechanisms) to be shared across clusters.
Cloud API objects can handle their own rate-limiting now that they're
shared between clusters.
We don't validate that the port isn't already in use, but we're running
in a network namespace, so hopefully there's no contention.
The other segments are apparently unused.
When all clusters are in the same netns, we'll eventually need more than
254 interfaces.  Support 16 bits' worth, but only create 500 for now,
since a large number of dhcp-host directives significantly increases
dnsmasq's startup time.

We don't actually need to retain the entire Interfaces array in memory
after dnsmasq.conf is rendered, but it's a small enough amount of RAM
not to be worth fixing for now.
Share the dnsmasq, discovery etcd, and NTP server between clusters.
Continue to use a per-cluster Omaha server, since tests can configure it.
No longer upload SSH key once per cluster.
It'll now be handled automatically.
Move the old "kola mkimage" functionality into flight and run it by
default on CL.
@dustymabe
Copy link
Member

nice work @bgilbert !

Copy link
Contributor

@arithx arithx left a comment

Choose a reason for hiding this comment

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

Initial Pass

kola/tests/kubernetes/setup.go Show resolved Hide resolved
kola/tests/kubernetes/setup.go Show resolved Hide resolved
// NewFlight will consume the environment variables $AWS_REGION,
// $AWS_ACCESS_KEY_ID, and $AWS_SECRET_ACCESS_KEY to determine the region to
// spawn instances in and the credentials to use to authenticate.
func NewFlight(opts *aws.Options) (platform.Flight, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This might just be because it's the initial implementation, but in the platform specific flights should we be handling the creation of entire test run / global resources (i.e. networking resources in AWS) in the platforms during the NewFlight creation and then providing accessors to the individual Cluster objects?

Copy link
Contributor

Choose a reason for hiding this comment

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

To add onto this comment, I'm fine with having these actions done in separate PRs after the landing of this one and am more than happy to file issues once this lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that would require some refactoring of platform/api/*, e.g. to have a setup function returning a state struct which is then passed to the instance-creation function. (Or instance creation could become a method on the state struct.) Makes sense, but probably for a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't thought of creating a State struct that has a built-in method for instance-creation but I do like the idea. My initial thought was around the caching of global / entire test run variables with simple accessor methods.

Once this PR lands I'll file a couple of issues for the different clouds that have distinct overarching state.

kola/harness.go Show resolved Hide resolved
platform/local/cluster.go Show resolved Hide resolved
platform/machine/qemu/disk.go Show resolved Hide resolved
kola/harness.go Show resolved Hide resolved
platform/local/flight.go Show resolved Hide resolved
platform/flight.go Show resolved Hide resolved
platform/machine/qemu/disk.go Show resolved Hide resolved
Copy link
Contributor

@ajeddeloh ajeddeloh 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, fix it or leave it. LGTM

kola/harness.go Show resolved Hide resolved
Copy link
Contributor

@arithx arithx left a comment

Choose a reason for hiding this comment

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

LGTM

@bgilbert bgilbert merged commit 513c237 into coreos:master Nov 9, 2018
@bgilbert bgilbert deleted the flight branch November 9, 2018 19:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants