Support multiple CIDR addresses for the pod SDN #14558

JacobTanenbaum · 2017-06-09T18:38:53Z

Work in progress PR for multiple CIDR address work. addresses can be defined in the master config file by

networkConfig:
  clusterNetworkCIDR: ""
  clusterNetworkConfig:
  - clusterNetworkCIDR: 10.128.0.0/28
  - clusterNetworkCIDR: 12.128.0.0/28
  externalIPNetworkCIDRs: null

or by passing a comma seporated list of them on the command line using --network-cidr="10.128.0.0/28,12.128.0.0/28"

besides general review - I could use some feedback on how to do a few things

How should I deal with a config file that defined ClusterNetworkCIDR the old way?
In pkg/sdn/plugin/master.go there where checks to see if the cluster cidr was shrunk, I don't think that check is still required. It should be valid to break a large cluster cidr into it's smaller components. Is checking that all objects are allocated in defined places enough? As done by checkclusterobjects() in pkg/sdn/plugin/common.go
I need to change oc get clusternetwork to show a comma separated list
finish the unit testing

review of the work so far and input on the above questions are appreciated
@knobunc @dcbw @danwinship @rajatchopra

danwinship · 2017-06-12T13:52:12Z

addresses can be defined in the master config file by

networkConfig:
  clusterNetworkCIDR: ""
  clusterNetworkConfig:
  - clusterNetworkCIDR: 10.128.0.0/28
  - clusterNetworkCIDR: 12.128.0.0/28

Hm... Is there any reason you did it that way? It seems like it would be nicer to just have

clusterNetworkCIDRs:
- 10.128.0.0/28
- 12.128.0.0/28

aka

clusterNetworkCIDRs: [ "10.128.0.0/28", "12.128.0.0/28" ]

How should I deal with a config file that defined ClusterNetworkCIDR the old way?

Treat it the same as if it was defined the new way, but with only a single CIDR range

In pkg/sdn/plugin/master.go there where checks to see if the cluster cidr was shrunk, I don't think that check is still required. It should be valid to break a large cluster cidr into it's smaller components. Is checking that all objects are allocated in defined places enough? As done by checkclusterobjects() in pkg/sdn/plugin/common.go

Yes, that sounds right. Make sure the nodes do the right thing on restart as well.

knobunc · 2017-06-12T14:33:24Z

@openshift/networking

knobunc · 2017-06-12T14:36:15Z

@danwinship the reason he did it that way is because we will shortly need to accommodate hot-resizing the subnets (or even supporting a way to have different size host subnets per range, perhaps with a way to label the subnets so different nodes can deliberately have different allocations).

pecameron

Just a quick pass through. I am learning about this as I go so not certain if this is at all helpful.

pecameron · 2017-06-12T15:10:07Z

pkg/cmd/cli/describe/printer.go

 	name := formatResourceName(opts.Kind, n.Name, opts.WithKind)
-	_, err := fmt.Fprintf(w, "%s\t%s\t%d\t%s\t%s\n", name, n.Network, n.HostSubnetLength, n.ServiceNetwork, n.PluginName)
+	_, err := fmt.Fprintf(w, "%s\t%s\t%d\t%s\t%s\n", name, strings.Join(cidrList, ","), n.HostSubnetLength, n.ServiceNetwork, n.PluginName)


doing them all on one line will mess up the report. I assume that people that like this feature will have a pretty fragments IP space.

Yes, it will be hard to read when there are several cidrs. May be one row for each cidr?

for _, cidr := range n.ClusterDef { , err := fmt.Fprintf(w, "%s\t%s\t%d\t%s\t%s\n", name, cidr. ...) }

pecameron · 2017-06-12T15:14:27Z

pkg/cmd/server/api/v1/types.go

@@ -497,6 +497,8 @@ type MasterNetworkConfig struct {
 	NetworkPluginName string `json:"networkPluginName"`
 	// ClusterNetworkCIDR is the CIDR string to specify the global overlay network's L3 space
 	ClusterNetworkCIDR string `json:"clusterNetworkCIDR"`
+	// ClusterNetworkConfig is used to specify the global overlay network's L3 space


might mention that this is a list of cidrs that define the global overlay network's L3 space

pecameron · 2017-06-12T15:16:45Z

pkg/cmd/server/api/v1/types.go

@@ -497,6 +497,8 @@ type MasterNetworkConfig struct {
 	NetworkPluginName string `json:"networkPluginName"`
 	// ClusterNetworkCIDR is the CIDR string to specify the global overlay network's L3 space
 	ClusterNetworkCIDR string `json:"clusterNetworkCIDR"`
+	// ClusterNetworkConfig is used to specify the global overlay network's L3 space
+	ClusterNetworkConfig []ClusterNetworkEntry `json:"clusterNetworkConfig"`
 	// HostSubnetLength is the number of bits to allocate to each host's subnet e.g. 8 would mean a /24 network on the host


So this is the minimum value that can be in the cidr list above. Might comment about this above.

pecameron · 2017-06-12T15:34:41Z

pkg/sdn/api/validation/validation.go

@@ -20,38 +20,67 @@ func SetDefaultClusterNetwork(cn sdnapi.ClusterNetwork) {
 	defaultClusterNetwork = &cn
 }

+//intersect tests two CIDR addresses to see if the first contains the second


In addition to contains, should check for first contained in second (unless second can't be larger than first). Should we check overlap with other defined spaces (service, exportIP, ...)?

pecameron · 2017-06-12T15:51:20Z

pkg/sdn/api/validation/validation_test.go

+//	One poorly formated address and one well formatted
+//	Overlapping addresses
+//	Both Cluster.Network & clusterDef is defined
+//


test out of range (> 255) in IP field.
test mask (/xx) > 32, or < min size (at least as big as HostSubnetLength)
test overlap with other well defined spaces (service ip, external ip,...)
I think overlap can be either contained within or contained by.

pecameron · 2017-06-12T15:56:41Z

pkg/sdn/api/validation/validation_test.go

@@ -30,7 +46,7 @@ func TestValidateClusterNetwork(t *testing.T) {
 			name: "Bad network",
 			cn: &api.ClusterNetwork{
 				ObjectMeta:       metav1.ObjectMeta{Name: "any"},
-				Network:          "10.20.0.0.0/16",
+				ClusterDef:       []api.ClusterNetworkEntry{{ClusterNetworkCIDR: "10.20.0.0.0/16"}},


bad network also 10.a.0.0/16, 10.256.0.0 and 0.0.0.0/33.

pecameron · 2017-06-12T16:25:23Z

pkg/sdn/plugin/common.go

@@ -39,20 +39,36 @@ func clusterNetworkToString(n *osapi.ClusterNetwork) string {
 	return fmt.Sprintf("%s (network: %q, hostSubnetBits: %d, serviceNetwork: %q, pluginName: %q)", n.Name, n.Network, n.HostSubnetLength, n.ServiceNetwork, n.PluginName)
 }

+//determine if the first argument contains any of the second


Isn't this contains the second or is contained in the second?

pecameron · 2017-06-12T16:33:31Z

pkg/sdn/plugin/common.go

+func parseNetworkInfo(clusterNetwork []osapi.ClusterNetworkEntry, serviceNetwork string) (*NetworkInfo, error) {
+	var cn []*net.IPNet
+
+	for _, entry := range clusterNetwork {


I think you need to check each cidr with all other cidrs (except for itself). How many cidrs do we support?

Yes, this is needed. Do we want to restrict number of cidrs that we support? If users specify single IPs or sparse cidrs, this could be huge.

What do you mean check each cidr with all other cidrs?

This is to verify there is no overlap between the given cidrs, otherwise pods may end up with same IPs.

pecameron · 2017-06-12T16:37:13Z

pkg/sdn/plugin/common.go

-		} else if !ni.ClusterNetwork.Contains(subnetIP) {
-			errList = append(errList, fmt.Errorf("existing node subnet: %s is not part of cluster network: %s", subnet.Subnet, ni.ClusterNetwork.String()))
+		} else if _, contains := cidrListContains(ni.ClusterNetwork, subnetIP); !contains {
+			errList = append(errList, fmt.Errorf("existing node subnet: %s in not part of any cluster network CIDR", subnet.Subnet))


pecameron · 2017-06-12T16:38:57Z

pkg/sdn/plugin/common.go

@@ -156,7 +174,7 @@ func getNetworkInfo(osClient *osclient.Client) (*NetworkInfo, error) {
 		return nil, err
 	}

-	return parseNetworkInfo(cn.Network, cn.ServiceNetwork)
+	return parseNetworkInfo(cn.ClusterDef, cn.ServiceNetwork)


Do we care about externalIP and load balancer IP here?

danwinship

looks good as a first draft

danwinship · 2017-06-12T18:29:10Z

pkg/cmd/server/api/types.go

-	ServiceNetworkCIDR string
+	NetworkPluginName    string
+	ClusterNetworkCIDR   string
+	ClusterNetworkConfig []ClusterNetworkEntry


I'd probably go with just ClusterNetworks.

It would be nice if we could get rid of the old ClusterNetworkCIDR field, but I'm not sure how backward-compat works with startup config objects. Old master-config.yaml files need to keep working though, so whatever that requires...

danwinship · 2017-06-12T18:33:06Z

pkg/cmd/server/api/types.go

@@ -626,6 +627,10 @@ type MasterNetworkConfig struct {
 	IngressIPNetworkCIDR string
 }

+type ClusterNetworkEntry struct {
+	ClusterNetworkCIDR string


Maybe just CIDR? You've already got ClusterNetwork in the struct name and field name...

Also, if the plan is to eventually have per-network HostSubnetLength values, I think we should add that here now; if we don't support different values yet, you can just add code to make sure every ClusterNetworkEntry has the same value.

(Also, if we're tweaking HostSubnetLength handling, we might want to consider renaming it and flipping the math around; the fact that it counts bits from the end of the IP address rather than the start is confusing. (Eg, HostSubnetLength of 9 means each node gets a /23 subnet (32 - 9 = 23). I think it would be clearer for most users if the config option was "23" rather than "9".

+1 on CIDR.
If you plan to do global/local HostSubnetLength field that I was suggesting, then.

type ClusterNetworkEntry struct { ClusterNetworkCIDR string HostSubnetLength uint32 }

danwinship · 2017-06-12T18:34:41Z

pkg/cmd/server/origin/master_config.go

@@ -635,7 +635,8 @@ func newAdmissionChain(pluginNames []string, admissionConfigFilename string, plu

 		case serviceadmit.RestrictedEndpointsPluginName:
 			// we need to set some customer parameters, so create by hand
-			restrictedNetworks, err := serviceadmit.ParseSimpleCIDRRules([]string{options.NetworkConfig.ClusterNetworkCIDR, options.NetworkConfig.ServiceNetworkCIDR})
+			// PLACEHOLDER HAVE TO FIGURE OUT OUT TO DO
+			restrictedNetworks, err := serviceadmit.ParseSimpleCIDRRules([]string{options.NetworkConfig.ServiceNetworkCIDR})


You need to pass an array containing all of the clusternetwork cidr values and the servicenetwork cidr value

danwinship · 2017-06-12T18:39:21Z

pkg/sdn/api/v1/types.go

@@ -20,6 +20,8 @@ type ClusterNetwork struct {

 	// Network is a CIDR string specifying the global overlay network's L3 space
 	Network string `json:"network" protobuf:"bytes,2,opt,name=network"`
+	// PLACEHOLDER
+	ClusterDef []ClusterNetworkEntry `json:"clusterDef" protobuf:"bytes,6,rep,name=clusterDef"`


https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md has some information on changing kubernetes API objects, and in particular discusses the backward-compatibility snafus that come with trying to turn a single-element field into an array... (The "API Conventions" link near the top of that document is also filled with useful information about how kubernetes APIs work.)

The easiest way to make this work backward-compatibly would be to say that the "default"/first network is still specified by the Network and HostSubnetLength fields, but the 2nd to Nth networks are part of a new array field (which could be called AdditionalNetworks or something like that).

You need a doc comment above the field, and between the doc comment and the actual declaration should be

// +optional

to indicate that it's an optional field

danwinship · 2017-06-12T18:40:24Z

pkg/sdn/api/v1/types.go

@@ -28,6 +30,11 @@ type ClusterNetwork struct {
 	PluginName string `json:"pluginName,omitempty" protobuf:"bytes,5,opt,name=pluginName"`
 }

+// PLACEHOLDER
+type ClusterNetworkEntry struct {
+	ClusterNetworkCIDR string `json:"clusterNetworkCIDR" protobuf:"bytes,1,opt,name=clusterNetworkCIDR"`


same comments as in pkg/cmd/server/api/types.go above

danwinship · 2017-06-12T18:46:25Z

pkg/sdn/api/types.go

@@ -16,11 +16,16 @@ type ClusterNetwork struct {
 	metav1.ObjectMeta

 	Network          string
+	ClusterDef       []ClusterNetworkEntry


The "unversioned" ClusterNetwork type (the one in pkg/sdn/api/types.go, as opposed to pkg/sdn/api/v1/types.go) is an internal-only object, not part of the API, so it is not subject to backward-compatibility rules. So you could get rid of the old Network and HostSubnetLength fields here and only have the new array.

The one catch with this is that if the unversioned type and the v1 type aren't identical, then you'll need to add some hand-written conversion functions, but this isn't difficult. There are no examples in pkg/sdn/api but you can see examples in other API groups (eg, pkg/image/api/v1/conversion.go).

danwinship · 2017-06-12T18:48:03Z

pkg/sdn/api/types.go

 	HostSubnetLength uint32
 	ServiceNetwork   string
 	PluginName       string
 }

+type ClusterNetworkEntry struct {
+	ClusterNetworkCIDR string


same comments as in pkg/cmd/server/api/types.go above

pravisankar · 2017-06-13T16:59:47Z

With multiple cluster network CIDR support, hostsubnet length don't need to be same for each CIDR. We may not add this support immediately but it will be nice if we can make the config compatible with the future change that is more likely to be added.

How about this master config?

clusterNetworkCIDR:   (deprecate this one)
hostSubnetLength: 2   (global)
clusterNetworkCIDRs:
    - CIDR: 10.128.0.0/28
      hostSubnetLength: 2   (local)
    - CIDR: 12.128.0.0/28
      hostSubnetLength: 2

If local hostSubnetLength field is not set, apply the global hostSubnetLength (during default conversions)
If global and any of the local hostSubnetLength fields (if set) are not matching, bail out with an error similar to 'different host subnet lengths for cluster network cidrs are not currently supported'.

How should I deal with a config file that defined ClusterNetworkCIDR the old way?
I have deprecated/renamed fields in master/node config before (retaining backward compatibility),
you can check these PRs for reference:
6557c81
4663c08

pravisankar · 2017-06-13T17:06:02Z

pkg/sdn/plugin/sdn_controller.go

+	//err = ioutil.WriteFile("/run/openshift-sdn/config.env", []byte(config), 0644)
+	//if err != nil {
+	//	return false, err
+	//}


This was discussed in #13726

pravisankar · 2017-06-13T19:04:28Z

pkg/cmd/server/start/master_args.go

+		}
+		clusterNetworkConfig = append(clusterNetworkConfig, clusterNetworkEntry)
+	}
+


My understanding is command line arguments is for simple/essential use cases and complex use cases need to be specified in configuration file. If we treat multiple cidr use case as complex then we don't need to support command line args.
Supporting comma separated list here makes it harder to specify different hostsubnetLength for each cidr later.

pravisankar · 2017-06-13T19:07:15Z

pkg/sdn/api/v1/types.go

@@ -20,6 +20,8 @@ type ClusterNetwork struct {

 	// Network is a CIDR string specifying the global overlay network's L3 space
 	Network string `json:"network" protobuf:"bytes,2,opt,name=network"`
+	// PLACEHOLDER
+	ClusterDef []ClusterNetworkEntry `json:"clusterDef"`


Needs to add protobuf tag

pravisankar · 2017-06-13T19:11:01Z

pkg/sdn/api/validation/validation.go

@@ -20,38 +20,67 @@ func SetDefaultClusterNetwork(cn sdnapi.ClusterNetwork) {
 	defaultClusterNetwork = &cn
 }

+//intersect tests two CIDR addresses to see if the first contains the second
+func intersect(cidr1, cidr2 string) bool {
+	_, net1, _ := net.ParseCIDR(cidr1)


Check the error (similar to cidr2 below)

pravisankar · 2017-06-13T19:20:34Z

pkg/sdn/plugin/master.go

@@ -178,3 +190,20 @@ func clusterNetworkChanged(obj *osapi.ClusterNetwork, old *osapi.ClusterNetwork)

 	return changed, nil
 }
+
+func clusterDefEquals(obj, old []osapi.ClusterNetworkEntry) bool {
+	if obj == nil && old == nil {


Not needed, condition below covers this check.

JacobTanenbaum · 2017-06-14T14:09:08Z

@danwinship @knobunc @pravisankar Should I change the math for hostSubnetLength field? Won't that How could we upgrade a config from the old math to the new math for hostsubnetlength?

pravisankar · 2017-06-15T18:58:51Z

From the stand point of user using multiple cidr, with the current implementation user may not use a cidr because it conflicts with global HostSubnetLength (they don't have enough bits in the cidr to allocate for the pod). I feel cidr and hostSubnetLength go hand in hand. Why don't we work on supporting multiple cidr and hostsubnetLength per cidr? This will remove some of these limitations and will avoid unnecessary migration later on.

@openshift/networking

JacobTanenbaum · 2017-07-07T14:08:02Z

@danwinship @pravisankar is this moving in the right direction? I deprecated the global hostsubnetlength field and added one per cidr. I also changed it so only one could be defined on the cli and others need to be added to the config

knobunc · 2017-09-27T20:00:11Z

/lgtm

pravisankar · 2017-09-27T20:04:21Z

/lgtm

smarterclayton · 2017-09-27T20:19:08Z

pkg/cmd/server/api/v1/types.go

-	HostSubnetLength uint32 `json:"hostSubnetLength"`
+	// DeprecatedClusterNetworkCIDR is the CIDR string to specify the global overlay network's L3 space.  Deprecated, but maintained for backwards compatibility, use ClusterNetworks instead.
+	DeprecatedClusterNetworkCIDR string `json:"clusterNetworkCIDR,omitempty"`
+	// ClusterNetworks is a list of ClusterNetwork objects that defines the global overlay network's L3 space by specifying a set of CIDR and netmasks that the SDN can allocate addressed from.  If this is specified, then DeprecatedClusterNetworkCIDR and DeprecatedHostSubnetLength may not be set.


Use the external field name - a user won't have any idea what "DeprecatedClusterNetworkCIDR" is without looking at the go code. In general, always use external field names in thees docs, no exceptions.

smarterclayton · 2017-09-27T20:42:37Z

pkg/cmd/server/api/v1/types.go

-	HostSubnetLength uint32 `json:"hostSubnetLength"`
+	// ClusterNetworkCIDR is the CIDR string to specify the global overlay network's L3 space.  Deprecated, but maintained for backwards compatibility, use ClusterNetworks instead.
+	DeprecatedClusterNetworkCIDR string `json:"clusterNetworkCIDR,omitempty"`
+	// ClusterNetworks is a list of ClusterNetwork objects that defines the global overlay network's L3 space by specifying a set of CIDR and netmasks that the SDN can allocate addressed from.  If this is specified, then DeprecatedClusterNetworkCIDR and DeprecatedHostSubnetLength may not be set.


My comments got lost AGAIN. Basically you can't use external names here. Use the names as they would appear in JSON. Users only see this from the API and documentation, they never see the go code, so the names in Go are meaningless.

clusterNetworkCIDR and hostSubnetLength are the names they see

Chaning the Network Config section of the the master config to allow multiple CIDR addresses and hostsubnet Lengths for the allocation of nodes' address space

knobunc · 2017-09-27T21:49:17Z

@smarterclayton Ok, good to go?

JacobTanenbaum · 2017-09-27T22:04:24Z

/test cmd

smarterclayton · 2017-09-27T22:52:09Z

API approved

knobunc · 2017-09-27T23:55:57Z

/lgtm

@smarterclayton would you approve this please too? Thanks

eparis · 2017-09-28T14:14:07Z

/approve

openshift-merge-robot · 2017-09-28T14:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, knobunc, pravisankar

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [eparis]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-ci-robot · 2017-09-28T15:17:55Z

@JacobTanenbaum: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/check	`d758c79`	link	`/test check`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

JacobTanenbaum · 2017-09-28T15:23:38Z

flake #16414 /retest

eparis · 2017-09-28T16:17:50Z

/retest

openshift-merge-robot · 2017-09-28T17:37:22Z

Automatic merge from submit-queue (batch tested with PRs 14558, 16544).

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2017

knobunc added the component/networking label Jun 12, 2017

pecameron reviewed Jun 12, 2017

View reviewed changes

danwinship suggested changes Jun 12, 2017

View reviewed changes

pravisankar reviewed Jun 13, 2017

View reviewed changes

JacobTanenbaum force-pushed the CIDR-work branch from 5dfee72 to 5e73345 Compare June 13, 2017 21:01

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2017

JacobTanenbaum force-pushed the CIDR-work branch from 5d6c39a to 28e55c5 Compare June 15, 2017 19:25

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2017

JacobTanenbaum force-pushed the CIDR-work branch 2 times, most recently from 6010d34 to 6cfab3e Compare June 22, 2017 20:40

JacobTanenbaum force-pushed the CIDR-work branch 6 times, most recently from 024dfea to bb7d9b7 Compare July 6, 2017 15:30

JacobTanenbaum force-pushed the CIDR-work branch from 3da8d78 to 366baeb Compare July 7, 2017 14:29

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017

pravisankar approved these changes Sep 27, 2017

View reviewed changes

openshift-ci-robot assigned pravisankar Sep 27, 2017

smarterclayton reviewed Sep 27, 2017

View reviewed changes

JacobTanenbaum force-pushed the CIDR-work branch from 897a0e3 to 5f143b0 Compare September 27, 2017 20:38

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017

smarterclayton reviewed Sep 27, 2017

View reviewed changes

JacobTanenbaum added 2 commits September 27, 2017 16:57

Allowing multiple CIDR addresses for allocation of Nodes

97f2923

Chaning the Network Config section of the the master config to allow multiple CIDR addresses and hostsubnet Lengths for the allocation of nodes' address space

auto generated files

d1e46b9

JacobTanenbaum force-pushed the CIDR-work branch from 5f143b0 to d1e46b9 Compare September 27, 2017 21:21

smarterclayton added api-approved and removed needs-api-review labels Sep 27, 2017

openshift-merge-robot added the needs-api-review label Sep 27, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017

eparis added the kind/bug Categorizes issue or PR as related to a bug. label Sep 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2017

openshift-merge-robot merged commit bb6f842 into openshift:master Sep 28, 2017

danwinship mentioned this pull request Oct 16, 2017

Fix defaulting of legacy ClusterNetwork fields #16897

Merged

Support multiple CIDR addresses for the pod SDN #14558

Support multiple CIDR addresses for the pod SDN #14558

Conversation

JacobTanenbaum commented Jun 9, 2017 • edited Loading

danwinship commented Jun 12, 2017

knobunc commented Jun 12, 2017

knobunc commented Jun 12, 2017

pecameron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobTanenbaum commented Jun 14, 2017

pravisankar commented Jun 15, 2017

JacobTanenbaum commented Jul 7, 2017

knobunc commented Sep 27, 2017

pravisankar commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented Sep 27, 2017

JacobTanenbaum commented Sep 27, 2017

smarterclayton commented Sep 27, 2017

knobunc commented Sep 27, 2017

eparis commented Sep 28, 2017

openshift-merge-robot commented Sep 28, 2017

openshift-ci-robot commented Sep 28, 2017 • edited Loading

JacobTanenbaum commented Sep 28, 2017 • edited Loading

eparis commented Sep 28, 2017

openshift-merge-robot commented Sep 28, 2017

JacobTanenbaum commented Jun 9, 2017 •

edited

Loading

pravisankar Jun 13, 2017 •

edited

Loading

danwinship Jun 12, 2017 •

edited

Loading

pravisankar commented Jun 13, 2017 •

edited

Loading

openshift-ci-robot commented Sep 28, 2017 •

edited

Loading

JacobTanenbaum commented Sep 28, 2017 •

edited

Loading