From a61b0a8371be5e968dee0fd5af8afd697b0de174 Mon Sep 17 00:00:00 2001 From: Mayank Tiwari Date: Mon, 29 Oct 2018 18:27:35 -0400 Subject: [PATCH 1/7] Symmetric host probing In addition, do not probe for host that is no longer in the host store. Change-Id: I8a750239b9dbd8a913cd8b12debe318ed396fca9 --- .../segmentrouting/HostHandler.java | 39 ++++++++++++++----- .../segmentrouting/SegmentRoutingManager.java | 11 ++++++ 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java index ffc0e4dd45..ae35f7baf1 100644 --- a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java +++ b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java @@ -448,18 +448,37 @@ public class HostHandler { * @param pairRemotePort pair remote port */ private void probe(Host host, ConnectPoint location, DeviceId pairDeviceId, PortNumber pairRemotePort) { + //Check if the host still exists in the host store + if (hostService.getHost(host.id()) == null) { + log.debug("Host entry for host {} no more present. Aborting hostprobe discover for this host", host.id()); + return; + } + VlanId vlanToProbe = host.vlan().equals(VlanId.NONE) ? srManager.getInternalVlanId(location) : host.vlan(); - srManager.interfaceService.getInterfaces().stream() - .filter(i -> i.vlanTagged().contains(vlanToProbe) || - i.vlanUntagged().equals(vlanToProbe) || - i.vlanNative().equals(vlanToProbe)) - .filter(i -> i.connectPoint().deviceId().equals(pairDeviceId)) - .filter(i -> !i.connectPoint().port().equals(pairRemotePort)) - .forEach(i -> { - log.debug("Probing host {} on pair device {}", host.id(), i.connectPoint()); - srManager.probingService.probeHost(host, i.connectPoint(), ProbeMode.DISCOVER); - }); + if (srManager.symmetricProbing) { + srManager.interfaceService.getInterfaces().stream() + .filter(i -> i.vlanTagged().contains(vlanToProbe) || + i.vlanUntagged().equals(vlanToProbe) || + i.vlanNative().equals(vlanToProbe)) + .filter(i -> i.connectPoint().deviceId().equals(pairDeviceId)) + .filter(i -> i.connectPoint().port().equals(location.port())) + .forEach(i -> { + log.debug("Probing host {} on pair device {}", host.id(), i.connectPoint()); + srManager.probingService.probeHost(host, i.connectPoint(), ProbeMode.DISCOVER); + }); + } else { + srManager.interfaceService.getInterfaces().stream() + .filter(i -> i.vlanTagged().contains(vlanToProbe) || + i.vlanUntagged().equals(vlanToProbe) || + i.vlanNative().equals(vlanToProbe)) + .filter(i -> i.connectPoint().deviceId().equals(pairDeviceId)) + .filter(i -> !i.connectPoint().port().equals(pairRemotePort)) + .forEach(i -> { + log.debug("Probing host {} on pair device {}", host.id(), i.connectPoint()); + srManager.probingService.probeHost(host, i.connectPoint(), ProbeMode.DISCOVER); + }); + } } /** diff --git a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java index a41c58da25..3c37d8057c 100644 --- a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java +++ b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java @@ -239,6 +239,10 @@ public class SegmentRoutingManager implements SegmentRoutingService { label = "Enable active probing to discover dual-homed hosts.") boolean activeProbing = true; + @Property(name = "symmetricProbing", boolValue = false, + label = "Enable only send probe on the same port number of the pair device") + boolean symmetricProbing = false; + @Property(name = "singleHomedDown", boolValue = false, label = "Enable administratively taking down single-homed hosts " + "when all uplinks are gone") @@ -616,6 +620,13 @@ public class SegmentRoutingManager implements SegmentRoutingService { log.info("{} active probing", activeProbing ? "Enabling" : "Disabling"); } + String strSymmetricProving = Tools.get(properties, "symmetricProbing"); + boolean expectSymmetricProbing = Boolean.parseBoolean(strSymmetricProving); + if (expectSymmetricProbing != symmetricProbing) { + symmetricProbing = expectSymmetricProbing; + log.info("{} symmetric probing", symmetricProbing ? "Enabling" : "Disabling"); + } + String strSingleHomedDown = Tools.get(properties, "singleHomedDown"); boolean expectSingleHomedDown = Boolean.parseBoolean(strSingleHomedDown); if (expectSingleHomedDown != singleHomedDown) { From e671fc96246dcaed4dd11dce38eaefa694fbd32f Mon Sep 17 00:00:00 2001 From: Yi Tseng Date: Wed, 31 Oct 2018 15:34:30 -0700 Subject: [PATCH 2/7] Update gNMI version and build script Change-Id: I0f2d3c84a7a13111dc5da966b21836338d327929 --- WORKSPACE | 4 + drivers/gnmi/BUILD | 3 +- modules.defs | 2 +- protocols/gnmi/BUILD | 3 +- protocols/gnmi/stub/BUILD | 49 +- protocols/gnmi/stub/src/main/proto/COMMIT_ID | 2 - protocols/gnmi/stub/src/main/proto/gnmi.proto | 423 ------------------ tools/build/bazel/gnmi_BUILD | 17 + tools/build/bazel/gnmi_workspace.bzl | 16 + 9 files changed, 50 insertions(+), 469 deletions(-) delete mode 100644 protocols/gnmi/stub/src/main/proto/COMMIT_ID delete mode 100644 protocols/gnmi/stub/src/main/proto/gnmi.proto create mode 100644 tools/build/bazel/gnmi_BUILD create mode 100644 tools/build/bazel/gnmi_workspace.bzl diff --git a/WORKSPACE b/WORKSPACE index 723f0ac1fa..1e83aaadff 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -68,6 +68,10 @@ load("//tools/build/bazel:p4lang_workspace.bzl", "generate_p4lang") generate_p4lang() +load("//tools/build/bazel:gnmi_workspace.bzl", "generate_gnmi") + +generate_gnmi() + git_repository( name = "build_bazel_rules_nodejs", remote = "https://github.com/bazelbuild/rules_nodejs.git", diff --git a/drivers/gnmi/BUILD b/drivers/gnmi/BUILD index 50d8b5e739..63b11c6962 100644 --- a/drivers/gnmi/BUILD +++ b/drivers/gnmi/BUILD @@ -4,8 +4,7 @@ COMPILE_DEPS = CORE_DEPS + KRYO + [ "@io_grpc_grpc_java//netty", "@io_grpc_grpc_java//stub", "//core/store/serializers:onos-core-serializers", - "//protocols/gnmi/stub:gnmi_java_grpc", - "//protocols/gnmi/stub:gnmi_java_proto", + "//protocols/gnmi/stub:onos-protocols-gnmi-stub", "//protocols/grpc/api:onos-protocols-grpc-api", "//protocols/grpc/proto:onos-protocols-grpc-proto", ] diff --git a/modules.defs b/modules.defs index 158b07c5f7..b4097c70da 100644 --- a/modules.defs +++ b/modules.defs @@ -266,7 +266,7 @@ ONOS_APPS = [ PROTOCOL_APPS = [ # '//protocols/grpc:onos-protocols-grpc-oar', # '//protocols/p4runtime:onos-protocols-p4runtime-oar', - '//protocols/gnmi:onos-protocols-gnmi-oar', + #'//protocols/gnmi:onos-protocols-gnmi-oar', '//protocols/xmpp/core:onos-protocols-xmpp-core-oar', '//protocols/xmpp/pubsub:onos-protocols-xmpp-pubsub-oar', ] diff --git a/protocols/gnmi/BUILD b/protocols/gnmi/BUILD index 30599ae05f..8a3510d054 100644 --- a/protocols/gnmi/BUILD +++ b/protocols/gnmi/BUILD @@ -1,6 +1,5 @@ BUNDLES = [ - "//protocols/gnmi/stub:gnmi_java_grpc", - "//protocols/gnmi/stub:gnmi_java_proto", + "//protocols/gnmi/stub:onos-protocols-gnmi-stub", ] onos_app( diff --git a/protocols/gnmi/stub/BUILD b/protocols/gnmi/stub/BUILD index 3b1a2dc0c1..cf7926594b 100644 --- a/protocols/gnmi/stub/BUILD +++ b/protocols/gnmi/stub/BUILD @@ -1,43 +1,14 @@ -load("//tools/build/bazel:osgi_java_library.bzl", "wrapped_osgi_jar") -load("@io_grpc_grpc_java//:java_grpc_library.bzl", "java_grpc_library") +load("//tools/build/bazel:osgi_java_library.bzl", "osgi_proto_jar") -wrapped_osgi_jar( - name = "gnmi_java_grpc", - jar = ":gnmi_java_grpc_native", - visibility = ["//visibility:public"], +PROTOS = [ + "@com_github_openconfig_gnmi//:gnmi_proto", + "@com_github_openconfig_gnmi//:gnmi_ext_proto", +] + +osgi_proto_jar( + grpc_proto_lib = "@com_github_openconfig_gnmi//:gnmi_proto", + proto_libs = PROTOS, deps = [ - "@io_grpc_grpc_java//core", - "@io_grpc_grpc_java//protobuf", - "@io_grpc_grpc_java//stub", - ], -) - -wrapped_osgi_jar( - name = "gnmi_java_proto", - jar = ":gnmi_java_proto_native", - visibility = ["//visibility:public"], - deps = [ - "@com_google_protobuf//:protobuf_java", - ], -) - -java_proto_library( - name = "gnmi_java_proto_native", - visibility = ["//visibility:public"], - deps = [":gnmi_proto"], -) - -java_grpc_library( - name = "gnmi_java_grpc_native", - srcs = [":gnmi_proto"], - deps = [":gnmi_java_proto_native"], -) - -proto_library( - name = "gnmi_proto", - srcs = ["src/main/proto/gnmi.proto"], - deps = [ - "@com_google_protobuf//:any_proto", - "@com_google_protobuf//:descriptor_proto", + "@com_google_api_grpc_proto_google_common_protos//jar", ], ) diff --git a/protocols/gnmi/stub/src/main/proto/COMMIT_ID b/protocols/gnmi/stub/src/main/proto/COMMIT_ID deleted file mode 100644 index acf773de9c..0000000000 --- a/protocols/gnmi/stub/src/main/proto/COMMIT_ID +++ /dev/null @@ -1,2 +0,0 @@ -https://github.com/openconfig/gnmi/blob/master/proto/gnmi/gnmi.proto -9c8d9e965b3e854107ea02c12ab11b70717456f2 diff --git a/protocols/gnmi/stub/src/main/proto/gnmi.proto b/protocols/gnmi/stub/src/main/proto/gnmi.proto deleted file mode 100644 index 1f3bb7cd05..0000000000 --- a/protocols/gnmi/stub/src/main/proto/gnmi.proto +++ /dev/null @@ -1,423 +0,0 @@ -// -// Copyright 2016 Google Inc. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// -syntax = "proto3"; - -import "google/protobuf/any.proto"; -import "google/protobuf/descriptor.proto"; - -// Package gNMI defines a service specification for the gRPC Network Management -// Interface. This interface is defined to be a standard interface via which -// a network management system ("client") can subscribe to state values, -// retrieve snapshots of state information, and manipulate the state of a data -// tree supported by a device ("target"). -// -// This document references the gNMI Specification which can be found at -// http://github.com/openconfig/reference/blob/master/rpc/gnmi -package gnmi; - -// Define a protobuf FileOption that defines the gNMI service version. -extend google.protobuf.FileOptions { - // The gNMI service semantic version. - string gnmi_service = 1001; -} - -// gNMI_service is the current version of the gNMI service, returned through -// the Capabilities RPC. -option (gnmi_service) = "0.5.0"; - -service gNMI { - // Capabilities allows the client to retrieve the set of capabilities that - // is supported by the target. This allows the target to validate the - // service version that is implemented and retrieve the set of models that - // the target supports. The models can then be specified in subsequent RPCs - // to restrict the set of data that is utilized. - // Reference: gNMI Specification Section 3.2 - rpc Capabilities(CapabilityRequest) returns (CapabilityResponse); - // Retrieve a snapshot of data from the target. A Get RPC requests that the - // target snapshots a subset of the data tree as specified by the paths - // included in the message and serializes this to be returned to the - // client using the specified encoding. - // Reference: gNMI Specification Section 3.3 - rpc Get(GetRequest) returns (GetResponse); - // Set allows the client to modify the state of data on the target. The - // paths to modified along with the new values that the client wishes - // to set the value to. - // Reference: gNMI Specification Section 3.4 - rpc Set(SetRequest) returns (SetResponse); - // Subscribe allows a client to request the target to send it values - // of particular paths within the data tree. These values may be streamed - // at a particular cadence (STREAM), sent one off on a long-lived channel - // (POLL), or sent as a one-off retrieval (ONCE). - // Reference: gNMI Specification Section 3.5 - rpc Subscribe(stream SubscribeRequest) returns (stream SubscribeResponse); -} - -// Notification is a re-usable message that is used to encode data from the -// target to the client. A Notification carries two types of changes to the data -// tree: -// - Deleted values (delete) - a set of paths that have been removed from the -// data tree. -// - Updated values (update) - a set of path-value pairs indicating the path -// whose value has changed in the data tree. -// Reference: gNMI Specification Section 2.1 -message Notification { - int64 timestamp = 1; // Timestamp in nanoseconds since Epoch. - Path prefix = 2; // Prefix used for paths in the message. - // An alias for the path specified in the prefix field. - // Reference: gNMI Specification Section 2.4.2 - string alias = 3; - repeated Update update = 4; // Data elements that have changed values. - repeated Path delete = 5; // Data elements that have been deleted. -} - -// Update is a re-usable message that is used to store a particular Path, -// Value pair. -// Reference: gNMI Specification Section 2.1 -message Update { - Path path = 1; // The path (key) for the update. - Value value = 2 [deprecated=true]; // The value (value) for the update. - TypedValue val = 3; // The explicitly typed update value. - uint32 duplicates = 4; // Number of coalesced duplicates. -} - -// TypedValue is used to encode a value being sent between the client and -// target (originated by either entity). -message TypedValue { - // One of the fields within the val oneof is populated with the value - // of the update. The type of the value being included in the Update - // determines which field should be populated. In the case that the - // encoding is a particular form of the base protobuf type, a specific - // field is used to store the value (e.g., json_val). - oneof value { - string string_val = 1; // String value. - int64 int_val = 2; // Integer value. - uint64 uint_val = 3; // Unsigned integer value. - bool bool_val = 4; // Bool value. - bytes bytes_val = 5; // Arbitrary byte sequence value. - float float_val = 6; // Floating point value. - Decimal64 decimal_val = 7; // Decimal64 encoded value. - ScalarArray leaflist_val = 8; // Mixed type scalar array value. - google.protobuf.Any any_val = 9; // protobuf.Any encoded bytes. - bytes json_val = 10; // JSON-encoded text. - bytes json_ietf_val = 11; // JSON-encoded text per RFC7951. - string ascii_val = 12; // Arbitrary ASCII text. - } -} - -// Path encodes a data tree path as a series of repeated strings, with -// each element of the path representing a data tree node name and the -// associated attributes. -// Reference: gNMI Specification Section 2.2.2. -message Path { - // Elements of the path are no longer encoded as a string, but rather within - // the elem field as a PathElem message. - repeated string element = 1 [deprecated=true]; - string origin = 2; // Label to disambiguate path. - repeated PathElem elem = 3; // Elements of the path. - string target = 4; // The name of the target - // (Sec. 2.2.2.1) -} - -// PathElem encodes an element of a gNMI path, along ith any attributes (keys) -// that may be associated with it. -// Reference: gNMI Specification Section 2.2.2. -message PathElem { - string name = 1; // The name of the element in the path. - map key = 2; // Map of key (attribute) name to value. -} - -// Value encodes a data tree node's value - along with the way in which -// the value is encoded. This message is deprecated by gNMI 0.3.0. -// Reference: gNMI Specification Section 2.2.3. -message Value { - option deprecated = true; - bytes value = 1; // Value of the variable being transmitted. - Encoding type = 2; // Encoding used for the value field. -} - -// Encoding defines the value encoding formats that are supported by the gNMI -// protocol. These encodings are used by both the client (when sending Set -// messages to modify the state of the target) and the target when serializing -// data to be returned to the client (in both Subscribe and Get RPCs). -// Reference: gNMI Specification Section 2.3 -enum Encoding { - JSON = 0; // JSON encoded text. - BYTES = 1; // Arbitrarily encoded bytes. - PROTO = 2; // Encoded according to out-of-band agreed Protobuf. - ASCII = 3; // ASCII text of an out-of-band agreed format. - JSON_IETF = 4; // JSON encoded text as per RFC7951. -} - -// Error message previously utilised to return errors to the client. Deprecated -// in favour of using the google.golang.org/genproto/googleapis/rpc/status -// message in the RPC response. -// Reference: gNMI Specification Section 2.5 -message Error { - option deprecated = true; - uint32 code = 1; // Canonical gRPC error code. - string message = 2; // Human readable error. - google.protobuf.Any data = 3; // Optional additional information. -} - -// Decimal64 is used to encode a fixed precision decimal number. The value -// is expressed as a set of digits with the precision specifying the -// number of digits following the decimal point in the digit set. -message Decimal64 { - int64 digits = 1; // Set of digits. - uint32 precision = 2; // Number of digits following the decimal point. -} - -// ScalarArray is used to encode a mixed-type array of values. -message ScalarArray { - // The set of elements within the array. Each TypedValue message should - // specify only elements that have a field identifier of 1-7 (i.e., the - // values are scalar values). - repeated TypedValue element = 1; -} - -// SubscribeRequest is the message sent by the client to the target when -// initiating a subscription to a set of paths within the data tree. The -// request field must be populated and the initial message must specify a -// SubscriptionList to initiate a subscription. The message is subsequently -// used to define aliases or trigger polled data to be sent by the target. -// Reference: gNMI Specification Section 3.5.1.1 -message SubscribeRequest { - oneof request { - SubscriptionList subscribe = 1; // Specify the paths within a subscription. - Poll poll = 3; // Trigger a polled update. - AliasList aliases = 4; // Aliases to be created. - } -} - -// Poll is sent within a SubscribeRequest to trigger the device to -// send telemetry updates for the paths that are associated with the -// subscription. -// Reference: gNMI Specification Section Section 3.5.1.4 -message Poll { -} - -// SubscribeResponse is the message used by the target within a Subscribe RPC. -// The target includes a Notification message which is used to transmit values -// of the path(s) that are associated with the subscription. The same message -// is to indicate that the target has sent all data values once (is -// synchronized). -// Reference: gNMI Specification Section 3.5.1.4 -message SubscribeResponse { - oneof response { - Notification update = 1; // Changed or sampled value for a path. - // Indicate target has sent all values associated with the subscription - // at least once. - bool sync_response = 3; - // Deprecated in favour of google.golang.org/genproto/googleapis/rpc/status - Error error = 4 [deprecated=true]; - } -} - -// SubscriptionList is used within a Subscribe message to specify the list of -// paths that the client wishes to subscribe to. The message consists of a -// list of (possibly prefixed) paths, and options that relate to the -// subscription. -// Reference: gNMI Specification Section 3.5.1.2 -message SubscriptionList { - Path prefix = 1; // Prefix used for paths. - repeated Subscription subscription = 2; // Set of subscriptions to create. - // Whether target defined aliases are allowed within the subscription. - bool use_aliases = 3; - QOSMarking qos = 4; // DSCP marking to be used. - // Mode of the subscription. - enum Mode { - STREAM = 0; // Values streamed by the target (Sec. 3.5.1.5.2). - ONCE = 1; // Values sent once-off by the target (Sec. 3.5.1.5.1). - POLL = 2; // Values sent in response to a poll request (Sec. 3.5.1.5.3). - } - Mode mode = 5; - // Whether elements of the schema that are marked as eligible for aggregation - // should be aggregated or not. - bool allow_aggregation = 6; - // The set of schemas that define the elements of the data tree that should - // be sent by the target. - repeated ModelData use_models = 7; - // The encoding that the target should use within the Notifications generated - // corresponding to the SubscriptionList. - Encoding encoding = 8; - // An optional field to specify that only updates to current state should be - // sent to a client. If set, the initial state is not sent to the client but - // rather only the sync message followed by any subsequent updates to the - // current state. For ONCE and POLL modes, this causes the server to send only - // the sync message (Sec. 3.5.2.3). - bool updates_only = 9; -} - -// Subscription is a single request within a SubscriptionList. The path -// specified is interpreted (along with the prefix) as the elements of the data -// tree that the client is subscribing to. The mode determines how the target -// should trigger updates to be sent. -// Reference: gNMI Specification Section 3.5.1.3 -message Subscription { - Path path = 1; // The data tree path. - SubscriptionMode mode = 2; // Subscription mode to be used. - uint64 sample_interval = 3; // ns between samples in SAMPLE mode. - // Indicates whether values that not changed should be sent in a SAMPLE - // subscription. - bool suppress_redundant = 4; - // Specifies the maximum allowable silent period in nanoseconds when - // suppress_redundant is in use. The target should send a value at least once - // in the period specified. - uint64 heartbeat_interval = 5; -} - -// SubscriptionMode is the mode of the subscription, specifying how the -// target must return values in a subscription. -// Reference: gNMI Specification Section 3.5.1.3 -enum SubscriptionMode { - TARGET_DEFINED = 0; // The target selects the relevant mode for each element. - ON_CHANGE = 1; // The target sends an update on element value change. - SAMPLE = 2; // The target samples values according to the interval. -} - -// QOSMarking specifies the DSCP value to be set on transmitted telemetry -// updates from the target. -// Reference: gNMI Specification Section 3.5.1.2 -message QOSMarking { - uint32 marking = 1; -} - -// Alias specifies a data tree path, and an associated string which defines an -// alias which is to be used for this path in the context of the RPC. The alias -// is specified as a string which is prefixed with "#" to disambiguate it from -// data tree element paths. -// Reference: gNMI Specification Section 2.4.2 -message Alias { - Path path = 1; // The path to be aliased. - string alias = 2; // The alias value, a string prefixed by "#". -} - -// AliasList specifies a list of aliases. It is used in a SubscribeRequest for -// a client to create a set of aliases that the target is to utilize. -// Reference: gNMI Specification Section 3.5.1.6 -message AliasList { - repeated Alias alias = 1; // The set of aliases to be created. -} - -// SetRequest is sent from a client to the target to update values in the data -// tree. Paths are either deleted by the client, or modified by means of being -// updated, or replaced. Where a replace is used, unspecified values are -// considered to be replaced, whereas when update is used the changes are -// considered to be incremental. The set of changes that are specified within -// a single SetRequest are considered to be a transaction. -// Reference: gNMI Specification Section 3.4.1 -message SetRequest { - Path prefix = 1; // Prefix used for paths in the message. - repeated Path delete = 2; // Paths to be deleted from the data tree. - repeated Update replace = 3; // Updates specifying elements to be replaced. - repeated Update update = 4; // Updates specifying elements to updated. -} - -// SetResponse is the response to a SetRequest, sent from the target to the -// client. It reports the result of the modifications to the data tree that were -// specified by the client. Errors for this RPC should be reported using the -// https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto -// message in the RPC return. The gnmi.Error message can be used to add additional -// details where required. -// Reference: gNMI Specification Section 3.4.2 -message SetResponse { - Path prefix = 1; // Prefix used for paths. - // A set of responses specifying the result of the operations specified in - // the SetRequest. - repeated UpdateResult response = 2; - Error message = 3 [deprecated=true]; // The overall status of the transaction. - int64 timestamp = 4; // Timestamp of transaction (ns since epoch). -} - -// UpdateResult is used within the SetResponse message to communicate the -// result of an operation specified within a SetRequest message. -// Reference: gNMI Specification Section 3.4.2 -message UpdateResult { - // The operation that was associated with the Path specified. - enum Operation { - INVALID = 0; - DELETE = 1; // The result relates to a delete of Path. - REPLACE = 2; // The result relates to a replace of Path. - UPDATE = 3; // The result relates to an update of Path. - } - // Deprecated timestamp for the UpdateResult, this field has been - // replaced by the timestamp within the SetResponse message, since - // all mutations effected by a set should be applied as a single - // transaction. - int64 timestamp = 1 [deprecated=true]; - Path path = 2; // Path associated with the update. - Error message = 3 [deprecated=true]; // Status of the update operation. - Operation op = 4; // Update operation type. -} - -// GetRequest is sent when a client initiates a Get RPC. It is used to specify -// the set of data elements for which the target should return a snapshot of -// data. The use_models field specifies the set of schema modules that are to -// be used by the target - where use_models is not specified then the target -// must use all schema models that it has. -// Reference: gNMI Specification Section 3.3.1 -message GetRequest { - Path prefix = 1; // Prefix used for paths. - repeated Path path = 2; // Paths requested by the client. - // Type of elements within the data tree. - enum DataType { - ALL = 0; // All data elements. - CONFIG = 1; // Config (rw) only elements. - STATE = 2; // State (ro) only elements. - // Data elements marked in the schema as operational. This refers to data - // elements whose value relates to the state of processes or interactions - // running on the device. - OPERATIONAL = 3; - } - DataType type = 3; // The type of data being requested. - Encoding encoding = 5; // Encoding to be used. - repeated ModelData use_models = 6; // The schema models to be used. -} - -// GetResponse is used by the target to respond to a GetRequest from a client. -// The set of Notifications corresponds to the data values that are requested -// by the client in the GetRequest. -// Reference: gNMI Specification Section 3.3.2 -message GetResponse { - repeated Notification notification = 1; // Data values. - Error error = 2 [deprecated=true]; // Errors that occurred in the Get. -} - -// CapabilityRequest is sent by the client in the Capabilities RPC to request -// that the target reports its capabilities. -// Reference: gNMI Specification Section 3.2.1 -message CapabilityRequest { -} - -// CapabilityResponse is used by the target to report its capabilities to the -// client within the Capabilities RPC. -// Reference: gNMI Specification Section 3.2.2 -message CapabilityResponse { - repeated ModelData supported_models = 1; // Supported schema models. - repeated Encoding supported_encodings = 2; // Supported encodings. - string gNMI_version = 3; // Supported gNMI version. -} - -// ModelData is used to describe a set of schema modules. It can be used in a -// CapabilityResponse where a target reports the set of modules that it -// supports, and within the SubscribeRequest and GetRequest messages to specify -// the set of models from which data tree elements should be reported. -// Reference: gNMI Specification Section 3.2.3 -message ModelData { - string name = 1; // Name of the model. - string organization = 2; // Organization publishing the model. - string version = 3; // Semantic version of the model. -} diff --git a/tools/build/bazel/gnmi_BUILD b/tools/build/bazel/gnmi_BUILD new file mode 100644 index 0000000000..43cb693cba --- /dev/null +++ b/tools/build/bazel/gnmi_BUILD @@ -0,0 +1,17 @@ +proto_library( + name = "gnmi_proto", + srcs = ["gnmi/gnmi.proto"], + deps = [ + ":gnmi_ext_proto", + "@com_google_protobuf//:descriptor_proto", + "@com_google_protobuf//:any_proto", + ], + visibility = ["//visibility:public"], +) + + +proto_library( + name = "gnmi_ext_proto", + srcs = ["gnmi_ext/gnmi_ext.proto"], + visibility = ["//visibility:public"], +) diff --git a/tools/build/bazel/gnmi_workspace.bzl b/tools/build/bazel/gnmi_workspace.bzl new file mode 100644 index 0000000000..6b0d4db9ec --- /dev/null +++ b/tools/build/bazel/gnmi_workspace.bzl @@ -0,0 +1,16 @@ +load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") + +# FIXME: Currently gNMI proto file uses incorrect path to import "gnmi_ext.proto" +# Temporary use patch from ONF before gNMI team fix it. + +GNMI_COMMIT = "onos" +GNMI_SHA = "0c4d5f168cb142f8135171204dac3ff8840a147f51fa361079f42fa585bec2ce" + +def generate_gnmi(): + http_archive( + name = "com_github_openconfig_gnmi", + urls = ["https://github.com/opennetworkinglab/gnmi/archive/%s.zip" % GNMI_COMMIT], + sha256 = GNMI_SHA, + strip_prefix = "gnmi-%s/proto" % GNMI_COMMIT, + build_file = "//tools/build/bazel:gnmi_BUILD", + ) From 7828d01ca571190c1c3ba526dd24ef6e7399c79c Mon Sep 17 00:00:00 2001 From: snehaprem Date: Thu, 1 Nov 2018 01:02:21 -0400 Subject: [PATCH 3/7] route-remove cli command with source type addition Change-Id: I420d7d0273c3b0c883c69c9f64fbe854fa1b6c65 --- .../routeservice/cli/RouteRemoveCommand.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/apps/route-service/app/src/main/java/org/onosproject/routeservice/cli/RouteRemoveCommand.java b/apps/route-service/app/src/main/java/org/onosproject/routeservice/cli/RouteRemoveCommand.java index 9fef2ec14d..24ecc71d50 100644 --- a/apps/route-service/app/src/main/java/org/onosproject/routeservice/cli/RouteRemoveCommand.java +++ b/apps/route-service/app/src/main/java/org/onosproject/routeservice/cli/RouteRemoveCommand.java @@ -41,6 +41,10 @@ public class RouteRemoveCommand extends AbstractShellCommand { required = true) String nextHopString = null; + @Argument(index = 2, name = "source", description = "Source type of the route", + required = false) + String source = null; + @Override protected void execute() { RouteAdminService service = AbstractShellCommand.get(RouteAdminService.class); @@ -48,7 +52,14 @@ public class RouteRemoveCommand extends AbstractShellCommand { IpPrefix prefix = IpPrefix.valueOf(prefixString); IpAddress nextHop = IpAddress.valueOf(nextHopString); - service.withdraw(Collections.singleton(new Route(Route.Source.STATIC, prefix, nextHop))); + // Routes through cli without mentioning source then it is created as STATIC, + // otherwise routes are created with corresponding source. + + Route route = source == null ? + new Route(Route.Source.STATIC, prefix, nextHop) : + new Route(Route.Source.valueOf(source), prefix, nextHop); + + service.withdraw(Collections.singleton(route)); } } From 801c6b00d82878993c7348c572ba9d94f6131a19 Mon Sep 17 00:00:00 2001 From: Ai Hamano Date: Thu, 1 Nov 2018 09:29:03 +0900 Subject: [PATCH 4/7] [ODTN]Update create-connectivity.json According to the definition of tapi-connectivity@2018-10-16.yang, "conn-constraint" and "topo-constraint" have been changed to "connectivity-constraint" and "topology-constraint". Change-Id: I08a59f18db8bff6b76e37ebccd4c5fdd9fac2897 --- apps/odtn/service/src/test/resources/create-connectivity.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/odtn/service/src/test/resources/create-connectivity.json b/apps/odtn/service/src/test/resources/create-connectivity.json index 35798f4ec8..47fd6dc76b 100644 --- a/apps/odtn/service/src/test/resources/create-connectivity.json +++ b/apps/odtn/service/src/test/resources/create-connectivity.json @@ -26,8 +26,8 @@ "protection-role" : "WORK" } ], - "conn-constraint" : {}, - "topo-constraint" : {}, + "connectivity-constraint" : {}, + "topology-constraint" : {}, "resilience-constraint" : [ ] } } From b696950be8291d74485d4f54aa1aeae69f036518 Mon Sep 17 00:00:00 2001 From: Jian Li Date: Tue, 30 Oct 2018 20:38:07 +0900 Subject: [PATCH 5/7] Fix: resolve a NPE occurs when getArpMode() returns null value 1. Add missing SG flow rules at receiving of node COMPLETE event 2. Fix typos in routing handler Change-Id: Id1e7d6217f55f2ef134873c98d4bc763a21fcfb5 --- .../impl/OpenstackRoutingArpHandler.java | 5 +++ .../impl/OpenstackRoutingHandler.java | 10 ++--- .../impl/OpenstackSecurityGroupHandler.java | 38 ++++++------------- 3 files changed, 21 insertions(+), 32 deletions(-) diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java index c711dede4b..304258fa37 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java @@ -871,6 +871,11 @@ public class OpenstackRoutingArpHandler { } private void setDefaultArpRule(OpenstackNode osNode, boolean install) { + + if (getArpMode() == null) { + return; + } + switch (getArpMode()) { case ARP_PROXY_MODE: setDefaultArpRuleForProxyMode(osNode, install); diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java index 16a3e28c8b..a831216b3b 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java @@ -478,15 +478,15 @@ public class OpenstackRoutingHandler { private void setInternalRoutes(Router osRouter, Subnet updatedSubnet, boolean install) { Network updatedNetwork = osNetworkAdminService.network(updatedSubnet.getNetworkId()); Set routableSubnets = routableSubnets(osRouter, updatedSubnet.getId()); - String updatedSegmendId = getSegmentId(updatedSubnet); + String updatedSegmentId = getSegmentId(updatedSubnet); // installs rule from/to my subnet intentionally to fix ICMP failure // to my subnet gateway if no external gateway added to the router osNodeService.completeNodes(COMPUTE).forEach(cNode -> { setInternalRouterRules( cNode.intgBridge(), - updatedSegmendId, - updatedSegmendId, + updatedSegmentId, + updatedSegmentId, IpPrefix.valueOf(updatedSubnet.getCidr()), IpPrefix.valueOf(updatedSubnet.getCidr()), updatedNetwork.getNetworkType(), @@ -496,7 +496,7 @@ public class OpenstackRoutingHandler { routableSubnets.forEach(subnet -> { setInternalRouterRules( cNode.intgBridge(), - updatedSegmendId, + updatedSegmentId, getSegmentId(subnet), IpPrefix.valueOf(updatedSubnet.getCidr()), IpPrefix.valueOf(subnet.getCidr()), @@ -506,7 +506,7 @@ public class OpenstackRoutingHandler { setInternalRouterRules( cNode.intgBridge(), getSegmentId(subnet), - updatedSegmendId, + updatedSegmentId, IpPrefix.valueOf(subnet.getCidr()), IpPrefix.valueOf(updatedSubnet.getCidr()), updatedNetwork.getNetworkType(), diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java index 11b22fd326..672ce8baca 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java @@ -61,7 +61,6 @@ import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupEvent; import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupListener; import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupService; import org.onosproject.openstacknetworking.util.RulePopulatorUtil; -import org.onosproject.openstacknode.api.OpenstackNode; import org.onosproject.openstacknode.api.OpenstackNodeEvent; import org.onosproject.openstacknode.api.OpenstackNodeListener; import org.onosproject.openstacknode.api.OpenstackNodeService; @@ -611,23 +610,21 @@ public class OpenstackSecurityGroupHandler { private void resetSecurityGroupRules() { if (useSecurityGroup) { - osNodeService.completeNodes(OpenstackNode.NodeType.COMPUTE) - .forEach(node -> osFlowRuleService - .setUpTableMissEntry(node.intgBridge(), ACL_TABLE)); + osNodeService.completeNodes(COMPUTE).forEach(node -> { + osFlowRuleService.setUpTableMissEntry(node.intgBridge(), ACL_TABLE); + initializeConnTrackTable(node.intgBridge(), true); + }); + securityGroupService.securityGroups().forEach(securityGroup -> securityGroup.getRules().forEach(this::securityGroupRuleAdded)); - osNodeService.nodes().stream() - .filter(node -> node.type().equals(OpenstackNode.NodeType.COMPUTE)) - .forEach(node -> initializeConnTrackTable(node .intgBridge(), true)); } else { - osNodeService.completeNodes(OpenstackNode.NodeType.COMPUTE) - .forEach(node -> osFlowRuleService - .connectTables(node.intgBridge(), ACL_TABLE, JUMP_TABLE)); + osNodeService.completeNodes(COMPUTE).forEach(node -> { + osFlowRuleService.connectTables(node.intgBridge(), ACL_TABLE, JUMP_TABLE); + initializeConnTrackTable(node.intgBridge(), false); + }); + securityGroupService.securityGroups().forEach(securityGroup -> securityGroup.getRules().forEach(this::securityGroupRuleRemoved)); - osNodeService.nodes().stream() - .filter(node -> node.type().equals(OpenstackNode.NodeType.COMPUTE)) - .forEach(node -> initializeConnTrackTable(node.intgBridge(), false)); } log.info("Reset security group info " + @@ -933,22 +930,9 @@ public class OpenstackSecurityGroupHandler { @Override public void event(OpenstackNodeEvent event) { - OpenstackNode osNode = event.subject(); - switch (event.type()) { case OPENSTACK_NODE_COMPLETE: - eventExecutor.execute(() -> { - try { - if (useSecurityGroup) { - initializeConnTrackTable(osNode.intgBridge(), true); - log.info("SG table initialization : {} is done", - osNode.intgBridge()); - } - } catch (IllegalArgumentException e) { - log.error("ACL table initialization error : {}", - e.getMessage()); - } - }); + resetSecurityGroupRules(); break; case OPENSTACK_NODE_CREATED: case OPENSTACK_NODE_REMOVED: From 28ec77fe80e717c2f90008792083d611cc754c58 Mon Sep 17 00:00:00 2001 From: Jian Li Date: Wed, 31 Oct 2018 07:07:25 +0900 Subject: [PATCH 6/7] Fix: start commit the conntrack only if the flow in the whitelist 1. Tag VNI and VID for ICMP reply packet initiated from exGW 2. Do not remove ICMP reply match rules when reset reactive SNAT rules 3. Fix incorrect SNAT IP retrieval methods for external gateway Change-Id: I9649161e9661636ea93f04d71159949d9281f4ae --- .../impl/OpenstackRoutingArpHandler.java | 33 +++++++---- .../impl/OpenstackRoutingHandler.java | 8 ++- .../impl/OpenstackRoutingIcmpHandler.java | 27 +++++++-- .../impl/OpenstackSecurityGroupHandler.java | 59 +++++++++++-------- .../impl/OpenstackSwitchingHandler.java | 20 +++---- 5 files changed, 92 insertions(+), 55 deletions(-) diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java index 304258fa37..6ae7100893 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingArpHandler.java @@ -76,6 +76,7 @@ import org.openstack4j.model.network.NetFloatingIP; import org.openstack4j.model.network.Network; import org.openstack4j.model.network.Port; import org.openstack4j.model.network.Router; +import org.openstack4j.model.network.RouterInterface; import org.osgi.service.component.ComponentContext; import org.slf4j.Logger; @@ -515,27 +516,33 @@ public class OpenstackRoutingArpHandler { } private void setFakeGatewayArpRuleByRouter(Router router, boolean install) { - setFakeGatewayArpRuleByGateway(router.getExternalGatewayInfo(), install); + setFakeGatewayArpRuleByGateway(router.getId(), router.getExternalGatewayInfo(), install); } - private Set getExternalGatewaySnatIps(ExternalGateway extGw) { - return osNetworkAdminService.ports().stream() - .filter(port -> - Objects.equals(port.getNetworkId(), extGw.getNetworkId())) - .filter(port -> - Objects.equals(port.getDeviceOwner(), DEVICE_OWNER_ROUTER_GW)) - .flatMap(port -> port.getFixedIps().stream()) + private Set getExternalGatewaySnatIps(String routerId, ExternalGateway extGw) { + if (routerId == null) { + return ImmutableSet.of(); + } + + Set portIds = osRouterAdminService.routerInterfaces(routerId).stream() + .map(RouterInterface::getPortId) + .collect(Collectors.toSet()); + + return portIds.stream() + .map(pid -> osNetworkAdminService.port(pid)) + .filter(p -> Objects.equals(p.getDeviceOwner(), DEVICE_OWNER_ROUTER_GW)) + .flatMap(p -> p.getFixedIps().stream()) .collect(Collectors.toSet()); } - private void setFakeGatewayArpRuleByGateway(ExternalGateway extGw, boolean install) { + private void setFakeGatewayArpRuleByGateway(String routerId, ExternalGateway extGw, boolean install) { if (ARP_BROADCAST_MODE.equals(getArpMode())) { if (extGw == null) { return; } - setFakeGatewayArpRuleByIps(getExternalGatewaySnatIps(extGw), install); + setFakeGatewayArpRuleByIps(getExternalGatewaySnatIps(routerId, extGw), install); } } @@ -649,13 +656,15 @@ public class OpenstackRoutingArpHandler { case OPENSTACK_ROUTER_GATEWAY_ADDED: eventExecutor.execute(() -> // add a gateway manually after adding a router - setFakeGatewayArpRuleByGateway(event.externalGateway(), true) + setFakeGatewayArpRuleByGateway(event.subject().getId(), + event.externalGateway(), true) ); break; case OPENSTACK_ROUTER_GATEWAY_REMOVED: eventExecutor.execute(() -> // remove a gateway from an existing router - setFakeGatewayArpRuleByGateway(event.externalGateway(), false) + setFakeGatewayArpRuleByGateway(event.subject().getId(), + event.externalGateway(), false) ); break; case OPENSTACK_FLOATING_IP_ASSOCIATED: diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java index a831216b3b..89b50dfda5 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingHandler.java @@ -850,7 +850,6 @@ public class OpenstackRoutingHandler { .matchIPSrc(srcSubnet) .matchEthDst(Constants.DEFAULT_GATEWAY_MAC); - switch (networkType) { case VXLAN: sBuilder.matchTunnelId(Long.parseLong(segmentId)); @@ -882,7 +881,14 @@ public class OpenstackRoutingHandler { GW_COMMON_TABLE, install); + // TODO: we do not remove the IcmpReplyMatchRules with false installation flag + // need to find a better way to remove this rule + if (install) { + setIcmpReplyRules(deviceId, install); + } + } + private void setIcmpReplyRules(DeviceId deviceId, boolean install) { // Sends ICMP response to controller for SNATing ingress traffic TrafficSelector selector = DefaultTrafficSelector.builder() .matchEthType(Ethernet.TYPE_IPV4) diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java index d1ab9d70a1..91970823ba 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackRoutingIcmpHandler.java @@ -93,6 +93,9 @@ public class OpenstackRoutingIcmpHandler { private static final String ERR_REQ = "Failed to handle ICMP request: "; private static final String ERR_DUPLICATE = " already exists"; + private static final String VXLAN = "VXLAN"; + private static final String VLAN = "VLAN"; + @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) protected CoreService coreService; @@ -407,13 +410,26 @@ public class OpenstackRoutingIcmpHandler { } private void sendReply(Ethernet icmpReply, InstancePort instPort) { - TrafficTreatment treatment = DefaultTrafficTreatment.builder() - .setOutput(instPort.portNumber()) - .build(); + TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder() + .setOutput(instPort.portNumber()); + + String netId = instPort.networkId(); + String segId = osNetworkService.segmentId(netId); + + switch (osNetworkService.networkType(netId)) { + case VXLAN: + tBuilder.setTunnelId(Long.valueOf(segId)); + break; + case VLAN: + tBuilder.setVlanId(VlanId.vlanId(segId)); + break; + default: + break; + } OutboundPacket packet = new DefaultOutboundPacket( instPort.deviceId(), - treatment, + tBuilder.build(), ByteBuffer.wrap(icmpReply.serialize())); packetService.emit(packet); @@ -435,7 +451,8 @@ public class OpenstackRoutingIcmpHandler { return; } - if (!gateways.isEmpty() && !gateways.contains(context.inPacket().receivedFrom().deviceId())) { + if (!gateways.isEmpty() && + !gateways.contains(context.inPacket().receivedFrom().deviceId())) { return; } diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java index 672ce8baca..7fe5fce949 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSecurityGroupHandler.java @@ -49,6 +49,7 @@ import org.onosproject.net.flow.DefaultTrafficTreatment; import org.onosproject.net.flow.TrafficSelector; import org.onosproject.net.flow.TrafficTreatment; import org.onosproject.net.flow.criteria.ExtensionSelector; +import org.onosproject.net.flow.instructions.ExtensionTreatment; import org.onosproject.openstacknetworking.api.InstancePort; import org.onosproject.openstacknetworking.api.InstancePortAdminService; import org.onosproject.openstacknetworking.api.InstancePortEvent; @@ -172,13 +173,13 @@ public class OpenstackSecurityGroupHandler { .build(); private final InstancePortListener instancePortListener = - new InternalInstancePortListener(); + new InternalInstancePortListener(); private final OpenstackNetworkListener osNetworkListener = - new InternalOpenstackNetworkListener(); + new InternalOpenstackNetworkListener(); private final OpenstackNetworkListener osPortListener = - new InternalOpenstackPortListener(); + new InternalOpenstackPortListener(); private final OpenstackSecurityGroupListener securityGroupListener = - new InternalSecurityGroupListener(); + new InternalSecurityGroupListener(); private final OpenstackNodeListener osNodeListener = new InternalNodeListener(); private ConsistentMap removedOsPortStore; @@ -331,10 +332,10 @@ public class OpenstackSecurityGroupHandler { SecurityGroupRule rSgRule = new NeutronSecurityGroupRule .SecurityGroupRuleConcreteBuilder() - .from(sgRule) - .direction(sgRule.getDirection().toUpperCase() - .equals(EGRESS) ? INGRESS : EGRESS) - .build(); + .from(sgRule) + .direction(sgRule.getDirection().toUpperCase() + .equals(EGRESS) ? INGRESS : EGRESS) + .build(); populateSecurityGroupRule(rSgRule, instPort, port, rInstPort.ipAddress().toIpPrefix(), install); populateSecurityGroupRule(rSgRule, rInstPort, port, @@ -343,7 +344,7 @@ public class OpenstackSecurityGroupHandler { } else { populateSecurityGroupRule(sgRule, instPort, port, sgRule.getRemoteIpPrefix() == null ? IP_PREFIX_ANY : - IpPrefix.valueOf(sgRule.getRemoteIpPrefix()), install); + IpPrefix.valueOf(sgRule.getRemoteIpPrefix()), install); } } @@ -358,14 +359,25 @@ public class OpenstackSecurityGroupHandler { return; } + // XXX All egress traffic needs to go through connection tracking module, + // which might hurt its performance. + ExtensionTreatment ctTreatment = + niciraConnTrackTreatmentBuilder(driverService, instPort.deviceId()) + .commit(true) + .build(); + + TrafficTreatment treatment = DefaultTrafficTreatment.builder() + .extension(ctTreatment, instPort.deviceId()) + .transition(JUMP_TABLE) + .build(); + selectors.forEach(selector -> osFlowRuleService.setRule(appId, - instPort.deviceId(), - selector, - DefaultTrafficTreatment.builder().transition(JUMP_TABLE).build(), - PRIORITY_ACL_RULE, - ACL_TABLE, - install)); + instPort.deviceId(), + selector, treatment, + PRIORITY_ACL_RULE, + ACL_TABLE, + install)); } /** @@ -486,7 +498,7 @@ public class OpenstackSecurityGroupHandler { sgRule.getPortRangeMin() < sgRule.getPortRangeMax()) { Map portRangeMatchMap = buildPortRangeMatches(sgRule.getPortRangeMin(), - sgRule.getPortRangeMax()); + sgRule.getPortRangeMax()); portRangeMatchMap.forEach((key, value) -> { if (sgRule.getProtocol().toUpperCase().equals(PROTO_TCP)) { @@ -523,9 +535,6 @@ public class OpenstackSecurityGroupHandler { sgRule.getPortRangeMin() == null ? 0 : sgRule.getPortRangeMin(), sgRule.getPortRangeMax() == null ? 0 : sgRule.getPortRangeMax()); buildMatchRemoteIp(sBuilder, remoteIp, sgRule.getDirection()); - if (sgRule.getRemoteGroupId() != null && sgRule.getRemoteGroupId().isEmpty()) { - buildMatchRemoteIp(sBuilder, remoteIp, sgRule.getDirection()); - } } private void buildTunnelId(TrafficSelector.Builder sBuilder, Port port) { @@ -536,6 +545,8 @@ public class OpenstackSecurityGroupHandler { sBuilder.matchVlanId(VlanId.vlanId(segId)); } else if (VXLAN.equals(netType)) { sBuilder.matchTunnelId(Long.valueOf(segId)); + } else { + log.warn("Cannot tag the VID due to lack of support of virtual network type {}", netType); } } @@ -628,13 +639,13 @@ public class OpenstackSecurityGroupHandler { } log.info("Reset security group info " + - (useSecurityGroup ? " with " : " without") + " Security Group"); + (useSecurityGroup ? " with " : " without") + " Security Group"); } private void securityGroupRuleAdded(SecurityGroupRule sgRule) { osNetService.ports().stream() .filter(port -> port.getSecurityGroups() - .contains(sgRule.getSecurityGroupId())) + .contains(sgRule.getSecurityGroupId())) .forEach(port -> { updateSecurityGroupRule( instancePortService.instancePort(port.getId()), @@ -649,7 +660,7 @@ public class OpenstackSecurityGroupHandler { Sets.union(osNetService.ports(), removedPorts).stream() .filter(port -> port.getSecurityGroups() - .contains(sgRule.getSecurityGroupId())) + .contains(sgRule.getSecurityGroupId())) .forEach(port -> { updateSecurityGroupRule( instancePortService.instancePort(port.getId()), @@ -819,7 +830,7 @@ public class OpenstackSecurityGroupHandler { } private class InternalOpenstackNetworkListener - implements OpenstackNetworkListener { + implements OpenstackNetworkListener { @Override public boolean isRelevant(OpenstackNetworkEvent event) { @@ -875,7 +886,7 @@ public class OpenstackSecurityGroupHandler { } private class InternalSecurityGroupListener - implements OpenstackSecurityGroupListener { + implements OpenstackSecurityGroupListener { @Override public boolean isRelevant(OpenstackSecurityGroupEvent event) { diff --git a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingHandler.java b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingHandler.java index 1d74e9af31..e63444f035 100644 --- a/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingHandler.java +++ b/apps/openstacknetworking/app/src/main/java/org/onosproject/openstacknetworking/impl/OpenstackSwitchingHandler.java @@ -39,7 +39,6 @@ import org.onosproject.net.flow.DefaultTrafficSelector; import org.onosproject.net.flow.DefaultTrafficTreatment; import org.onosproject.net.flow.TrafficSelector; import org.onosproject.net.flow.TrafficTreatment; -import org.onosproject.net.flow.instructions.ExtensionTreatment; import org.onosproject.openstacknetworking.api.InstancePort; import org.onosproject.openstacknetworking.api.InstancePortEvent; import org.onosproject.openstacknetworking.api.InstancePortListener; @@ -49,7 +48,6 @@ import org.onosproject.openstacknetworking.api.OpenstackNetworkEvent; import org.onosproject.openstacknetworking.api.OpenstackNetworkListener; import org.onosproject.openstacknetworking.api.OpenstackNetworkService; import org.onosproject.openstacknetworking.api.OpenstackSecurityGroupService; -import org.onosproject.openstacknetworking.util.RulePopulatorUtil; import org.onosproject.openstacknode.api.OpenstackNode; import org.onosproject.openstacknode.api.OpenstackNodeService; import org.openstack4j.model.network.Network; @@ -500,25 +498,21 @@ public final class OpenstackSwitchingHandler { .matchInPort(instPort.portNumber()) .build(); - // XXX All egress traffic needs to go through connection tracking module, - // which might hurt its performance. - ExtensionTreatment ctTreatment = - RulePopulatorUtil.niciraConnTrackTreatmentBuilder(driverService, instPort.deviceId()) - .commit(true).build(); + TrafficTreatment.Builder tBuilder = DefaultTrafficTreatment.builder() + .setTunnelId(getVni(instPort)); - TrafficTreatment.Builder tb = DefaultTrafficTreatment.builder() - .setTunnelId(getVni(instPort)) - .transition(ARP_TABLE); - if (securityGroupService.isSecurityGroupEnabled() && ethType == Ethernet.TYPE_IPV4) { - tb.extension(ctTreatment, instPort.deviceId()); + if (ethType == Ethernet.TYPE_ARP) { + tBuilder.transition(ARP_TABLE); + } else if (ethType == Ethernet.TYPE_IPV4) { + tBuilder.transition(ACL_TABLE); } osFlowRuleService.setRule( appId, instPort.deviceId(), selector, - tb.build(), + tBuilder.build(), PRIORITY_TUNNEL_TAG_RULE, VTAG_TABLE, install); From c88ebaa1e42778d85cbf0677be192f921fad0075 Mon Sep 17 00:00:00 2001 From: Charles Chan Date: Thu, 1 Nov 2018 20:08:47 -0700 Subject: [PATCH 7/7] Save developers from wasting hours on wrong bucket type Enforce bucket type to be consistent with group type Update unit tests accordingly Change-Id: Ia5f56ca4b5445268ebee09e321c34ed3f3f39827 (cherry picked from commit 5c00be8b143eb73d42b98820641da5c20e458579) --- .../net/group/DefaultGroupDescription.java | 3 +- .../group/DefaultGroupDescriptionTest.java | 31 ++++--- .../net/group/DefaultGroupTest.java | 37 ++++---- .../codec/impl/GroupCodecTest.java | 15 ++-- .../onosproject/utils/ComparatorsTest.java | 3 +- .../group/impl/DistributedGroupStoreTest.java | 87 ++++++++----------- 6 files changed, 83 insertions(+), 93 deletions(-) diff --git a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java index e047062831..4322482ac3 100644 --- a/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java +++ b/core/api/src/main/java/org/onosproject/net/group/DefaultGroupDescription.java @@ -65,7 +65,8 @@ public class DefaultGroupDescription implements GroupDescription { if (this.type == GroupDescription.Type.INDIRECT) { checkArgument(buckets.buckets().size() == 1, "Indirect group " + "should have only one action bucket"); - } + } + checkArgument(buckets.buckets().stream().allMatch(b -> b.type() == type), "Inconsistent bucket type"); this.appCookie = appCookie; this.givenGroupId = groupId; this.appId = appId; diff --git a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java index 0a0a093aa3..3d2d3687bc 100644 --- a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java +++ b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupDescriptionTest.java @@ -16,6 +16,8 @@ package org.onosproject.net.group; import org.junit.Test; +import org.onosproject.core.GroupId; +import org.onosproject.net.PortNumber; import org.onosproject.net.flow.DefaultTrafficTreatment; import org.onosproject.net.flow.TrafficTreatment; @@ -32,28 +34,31 @@ import static org.onosproject.net.NetTestTools.did; * Default group description unit tests. */ public class DefaultGroupDescriptionTest { - byte[] keyData = "abcdefg".getBytes(); + private final byte[] keyData = "abcdefg".getBytes(); private final GroupKey key = new DefaultGroupKey(keyData); - private final TrafficTreatment treatment = - DefaultTrafficTreatment.emptyTreatment(); - private final GroupBucket bucket = - DefaultGroupBucket.createSelectGroupBucket(treatment); - private final GroupBuckets groupBuckets = - new GroupBuckets(ImmutableList.of(bucket)); + private final GroupId groupId1 = new GroupId(1); + private final TrafficTreatment treatment = DefaultTrafficTreatment.emptyTreatment(); + + private final GroupBucket failoverGroupBucket = + DefaultGroupBucket.createFailoverGroupBucket(treatment, PortNumber.IN_PORT, groupId1); + private final GroupBuckets failoverGroupBuckets = new GroupBuckets(ImmutableList.of(failoverGroupBucket)); + private final GroupBucket indirectGroupBucket = + DefaultGroupBucket.createIndirectGroupBucket(treatment); + private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectGroupBucket)); + private final DefaultGroupDescription d1 = new DefaultGroupDescription(did("2"), GroupDescription.Type.FAILOVER, - groupBuckets); - private final DefaultGroupDescription sameAsD1 = - new DefaultGroupDescription(d1); + failoverGroupBuckets); + private final DefaultGroupDescription sameAsD1 = new DefaultGroupDescription(d1); private final DefaultGroupDescription d2 = new DefaultGroupDescription(did("2"), GroupDescription.Type.INDIRECT, - groupBuckets); + indirectGroupBuckets); private final DefaultGroupDescription d3 = new DefaultGroupDescription(did("3"), GroupDescription.Type.FAILOVER, - groupBuckets, + failoverGroupBuckets, key, 711, APP_ID); @@ -86,7 +91,7 @@ public class DefaultGroupDescriptionTest { public void testConstruction() { assertThat(d3.deviceId(), is(did("3"))); assertThat(d3.type(), is(GroupDescription.Type.FAILOVER)); - assertThat(d3.buckets(), is(groupBuckets)); + assertThat(d3.buckets(), is(failoverGroupBuckets)); assertThat(d3.appId(), is(APP_ID)); assertThat(d3.givenGroupId(), is(711)); assertThat(key.key(), is(keyData)); diff --git a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java index e8fb7b3bf3..c84ac876f8 100644 --- a/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java +++ b/core/api/src/test/java/org/onosproject/net/group/DefaultGroupTest.java @@ -18,6 +18,7 @@ package org.onosproject.net.group; import org.junit.Test; import org.onosproject.core.GroupId; import org.onosproject.net.NetTestTools; +import org.onosproject.net.PortNumber; import org.onosproject.net.flow.DefaultTrafficTreatment; import com.google.common.collect.ImmutableList; @@ -35,24 +36,20 @@ public class DefaultGroupTest { private final GroupId id2 = new GroupId(7); private final GroupId id3 = new GroupId(1234); - private final GroupBucket bucket = - DefaultGroupBucket.createSelectGroupBucket( - DefaultTrafficTreatment.emptyTreatment()); - private final GroupBuckets groupBuckets = - new GroupBuckets(ImmutableList.of(bucket)); - private final GroupDescription groupDesc1 = - new DefaultGroupDescription(did("1"), - GroupDescription.Type.FAILOVER, - groupBuckets); - private final GroupDescription groupDesc2 = - new DefaultGroupDescription(did("2"), - GroupDescription.Type.FAILOVER, - groupBuckets); + private final GroupBucket failoverBucket = DefaultGroupBucket.createFailoverGroupBucket( + DefaultTrafficTreatment.emptyTreatment(), PortNumber.IN_PORT, id1); + private final GroupBuckets failoverGroupBuckets = new GroupBuckets(ImmutableList.of(failoverBucket)); - private final GroupDescription groupDesc3 = - new DefaultGroupDescription(did("3"), - GroupDescription.Type.INDIRECT, - groupBuckets); + private final GroupBucket indirectBucket = + DefaultGroupBucket.createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment()); + private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectBucket)); + + private final GroupDescription groupDesc1 = + new DefaultGroupDescription(did("1"), GroupDescription.Type.FAILOVER, failoverGroupBuckets); + private final GroupDescription groupDesc2 = + new DefaultGroupDescription(did("2"), GroupDescription.Type.FAILOVER, failoverGroupBuckets); + private final GroupDescription groupDesc3 = + new DefaultGroupDescription(did("3"), GroupDescription.Type.INDIRECT, indirectGroupBuckets); DefaultGroup group1 = new DefaultGroup(id1, groupDesc1); DefaultGroup sameAsGroup1 = new DefaultGroup(id1, groupDesc1); @@ -83,7 +80,7 @@ public class DefaultGroupTest { assertThat(group1.life(), is(0L)); assertThat(group1.packets(), is(0L)); assertThat(group1.referenceCount(), is(0L)); - assertThat(group1.buckets(), is(groupBuckets)); + assertThat(group1.buckets(), is(failoverGroupBuckets)); assertThat(group1.state(), is(Group.GroupState.PENDING_ADD)); assertThat(group1.failedRetryCount(), is(0)); } @@ -94,14 +91,14 @@ public class DefaultGroupTest { @Test public void checkConstructionWithDid() { DefaultGroup group = new DefaultGroup(id2, NetTestTools.did("1"), - GroupDescription.Type.ALL, groupBuckets); + GroupDescription.Type.FAILOVER, failoverGroupBuckets); assertThat(group.id(), is(id2)); assertThat(group.bytes(), is(0L)); assertThat(group.life(), is(0L)); assertThat(group.packets(), is(0L)); assertThat(group.referenceCount(), is(0L)); assertThat(group.deviceId(), is(NetTestTools.did("1"))); - assertThat(group.buckets(), is(groupBuckets)); + assertThat(group.buckets(), is(failoverGroupBuckets)); assertThat(group.failedRetryCount(), is(0)); } diff --git a/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java b/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java index 0867213bdc..19c9671ed9 100644 --- a/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java +++ b/core/common/src/test/java/org/onosproject/codec/impl/GroupCodecTest.java @@ -76,23 +76,22 @@ public class GroupCodecTest { @Test public void codecEncodeTest() { - GroupBucket bucket1 = DefaultGroupBucket - .createSelectGroupBucket(DefaultTrafficTreatment.emptyTreatment()); - GroupBucket bucket2 = DefaultGroupBucket - .createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment()); - GroupBuckets buckets = new GroupBuckets(ImmutableList.of(bucket1, bucket2)); - GroupBuckets bucketsIndirect = new GroupBuckets(ImmutableList.of(bucket2)); + GroupBucket bucket1 = DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment()); + GroupBucket bucket2 = DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment()); + GroupBucket bucket3 = DefaultGroupBucket.createIndirectGroupBucket(DefaultTrafficTreatment.emptyTreatment()); + GroupBuckets allBuckets = new GroupBuckets(ImmutableList.of(bucket1, bucket2)); + GroupBuckets indirectBuckets = new GroupBuckets(ImmutableList.of(bucket3)); DefaultGroup group = new DefaultGroup( new GroupId(1), NetTestTools.did("d1"), ALL, - buckets); + allBuckets); DefaultGroup group1 = new DefaultGroup( new GroupId(2), NetTestTools.did("d2"), INDIRECT, - bucketsIndirect); + indirectBuckets); MockCodecContext context = new MockCodecContext(); GroupCodec codec = new GroupCodec(); diff --git a/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java b/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java index 6047719bb3..d616e71c36 100644 --- a/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java +++ b/core/common/src/test/java/org/onosproject/utils/ComparatorsTest.java @@ -129,8 +129,7 @@ public class ComparatorsTest { private final ConnectPoint cp = new ConnectPoint(DeviceId.deviceId("of:00001"), PortNumber.portNumber(100)); private final GroupBucket testBucket = - DefaultGroupBucket.createSelectGroupBucket( - DefaultTrafficTreatment.emptyTreatment()); + DefaultGroupBucket.createAllGroupBucket(DefaultTrafficTreatment.emptyTreatment()); private final GroupBuckets groupBuckets = new GroupBuckets(ImmutableList.of(testBucket)); private final GroupDescription groupDesc1 = diff --git a/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java index 0a44359184..087bd808ad 100644 --- a/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java +++ b/core/store/dist/src/test/java/org/onosproject/store/group/impl/DistributedGroupStoreTest.java @@ -61,11 +61,11 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.onosproject.net.NetTestTools.APP_ID; import static org.onosproject.net.NetTestTools.did; import static org.onosproject.net.group.GroupDescription.Type.ALL; import static org.onosproject.net.group.GroupDescription.Type.INDIRECT; -import static org.onosproject.net.group.GroupDescription.Type.SELECT; import static org.onosproject.net.group.GroupStore.UpdateType.ADD; import static org.onosproject.net.group.GroupStore.UpdateType.SET; /** @@ -73,49 +73,49 @@ import static org.onosproject.net.group.GroupStore.UpdateType.SET; */ public class DistributedGroupStoreTest { - DeviceId deviceId1 = did("dev1"); - DeviceId deviceId2 = did("dev2"); - GroupId groupId1 = new GroupId(1); - GroupId groupId2 = new GroupId(2); - GroupId groupId3 = new GroupId(3); - GroupKey groupKey1 = new DefaultGroupKey("abc".getBytes()); - GroupKey groupKey2 = new DefaultGroupKey("def".getBytes()); - GroupKey groupKey3 = new DefaultGroupKey("ghi".getBytes()); + private final DeviceId deviceId1 = did("dev1"); + private final DeviceId deviceId2 = did("dev2"); + private final GroupId groupId1 = new GroupId(1); + private final GroupId groupId2 = new GroupId(2); + private final GroupId groupId3 = new GroupId(3); + private final GroupKey groupKey1 = new DefaultGroupKey("abc".getBytes()); + private final GroupKey groupKey2 = new DefaultGroupKey("def".getBytes()); + private final GroupKey groupKey3 = new DefaultGroupKey("ghi".getBytes()); - TrafficTreatment treatment = - DefaultTrafficTreatment.emptyTreatment(); - GroupBucket selectGroupBucket = - DefaultGroupBucket.createSelectGroupBucket(treatment); - GroupBucket failoverGroupBucket = - DefaultGroupBucket.createFailoverGroupBucket(treatment, - PortNumber.IN_PORT, groupId1); + private final TrafficTreatment treatment = DefaultTrafficTreatment.emptyTreatment(); + private final TrafficTreatment treatment2 = DefaultTrafficTreatment.builder() + .setOutput(PortNumber.portNumber(2)).build(); + private final GroupBucket allGroupBucket = DefaultGroupBucket.createAllGroupBucket(treatment); + private final GroupBucket allGroupBucket2 = DefaultGroupBucket.createAllGroupBucket(treatment2); + private final GroupBuckets allGroupBuckets = new GroupBuckets(ImmutableList.of(allGroupBucket)); + private final GroupBucket indirectGroupBucket = DefaultGroupBucket.createIndirectGroupBucket(treatment); + private final GroupBuckets indirectGroupBuckets = new GroupBuckets(ImmutableList.of(indirectGroupBucket)); - GroupBuckets buckets = new GroupBuckets(ImmutableList.of(selectGroupBucket)); - GroupDescription groupDescription1 = new DefaultGroupDescription( + private final GroupDescription groupDescription1 = new DefaultGroupDescription( deviceId1, ALL, - buckets, + allGroupBuckets, groupKey1, groupId1.id(), APP_ID); - GroupDescription groupDescription2 = new DefaultGroupDescription( + private final GroupDescription groupDescription2 = new DefaultGroupDescription( deviceId2, INDIRECT, - buckets, + indirectGroupBuckets, groupKey2, groupId2.id(), APP_ID); - GroupDescription groupDescription3 = new DefaultGroupDescription( + private final GroupDescription groupDescription3 = new DefaultGroupDescription( deviceId2, INDIRECT, - buckets, + indirectGroupBuckets, groupKey3, groupId3.id(), APP_ID); - DistributedGroupStore groupStoreImpl; - GroupStore groupStore; - ConsistentMap auditPendingReqQueue; + private DistributedGroupStore groupStoreImpl; + private GroupStore groupStore; + private ConsistentMap auditPendingReqQueue; static class MasterOfAll extends MastershipServiceAdapter { @Override @@ -285,8 +285,8 @@ public class DistributedGroupStoreTest { GroupDescription groupDescription3 = new DefaultGroupDescription( deviceId1, - SELECT, - buckets, + ALL, + allGroupBuckets, new DefaultGroupKey("aaa".getBytes()), null, APP_ID); @@ -347,7 +347,7 @@ public class DistributedGroupStoreTest { GroupOperation opAdd = GroupOperation.createAddGroupOperation(groupId1, INDIRECT, - buckets); + indirectGroupBuckets); groupStore.groupOperationFailed(deviceId1, opAdd); List eventsAfterAddFailed = delegate.eventsSeen(); @@ -361,7 +361,7 @@ public class DistributedGroupStoreTest { GroupOperation opModify = GroupOperation.createModifyGroupOperation(groupId2, INDIRECT, - buckets); + indirectGroupBuckets); groupStore.groupOperationFailed(deviceId2, opModify); List eventsAfterModifyFailed = delegate.eventsSeen(); assertThat(eventsAfterModifyFailed, hasSize(1)); @@ -400,7 +400,7 @@ public class DistributedGroupStoreTest { // test group exists GroupOperation opAdd = GroupOperation - .createAddGroupOperation(groupId1, INDIRECT, buckets); + .createAddGroupOperation(groupId1, ALL, allGroupBuckets); GroupOperation addFailedExists = GroupOperation .createFailedGroupOperation(opAdd, GroupMsgErrorCode.GROUP_EXISTS); groupStore.groupOperationFailed(deviceId1, addFailedExists); @@ -420,7 +420,7 @@ public class DistributedGroupStoreTest { assertEquals(0, g2.failedRetryCount()); assertEquals(GroupState.PENDING_ADD, g2.state()); GroupOperation opAdd1 = GroupOperation - .createAddGroupOperation(groupId2, INDIRECT, buckets); + .createAddGroupOperation(groupId2, INDIRECT, indirectGroupBuckets); GroupOperation addFailedInvalid = GroupOperation .createFailedGroupOperation(opAdd1, GroupMsgErrorCode.INVALID_GROUP); @@ -487,9 +487,7 @@ public class DistributedGroupStoreTest { */ @Test public void testUpdateGroupDescription() { - - GroupBuckets buckets = - new GroupBuckets(ImmutableList.of(failoverGroupBucket, selectGroupBucket)); + GroupBuckets buckets = new GroupBuckets(ImmutableList.of(allGroupBucket2)); groupStore.deviceInitialAuditCompleted(deviceId1, true); groupStore.storeGroupDescription(groupDescription1); @@ -504,40 +502,31 @@ public class DistributedGroupStoreTest { assertThat(group1.appCookie(), is(newKey)); assertThat(group1.buckets().buckets(), hasSize(2)); - short weight = 5; - GroupBucket selectGroupBucketWithWeight = - DefaultGroupBucket.createSelectGroupBucket(treatment, weight); - buckets = new GroupBuckets(ImmutableList.of(failoverGroupBucket, - selectGroupBucketWithWeight)); - + buckets = new GroupBuckets(ImmutableList.of(allGroupBucket, allGroupBucket2)); groupStore.updateGroupDescription(deviceId1, newKey, ADD, buckets, newKey); - group1 = groupStore.getGroup(deviceId1, groupId1); assertThat(group1.appCookie(), is(newKey)); assertThat(group1.buckets().buckets(), hasSize(2)); for (GroupBucket bucket : group1.buckets().buckets()) { - if (bucket.type() == SELECT) { - assertEquals(weight, bucket.weight()); - } + assertTrue(bucket.treatment().equals(treatment) || + bucket.treatment().equals(treatment2)); } - buckets = new GroupBuckets(ImmutableList.of(selectGroupBucketWithWeight)); - + buckets = new GroupBuckets(ImmutableList.of(allGroupBucket2)); groupStore.updateGroupDescription(deviceId1, newKey, SET, buckets, newKey); - group1 = groupStore.getGroup(deviceId1, groupId1); assertThat(group1.appCookie(), is(newKey)); assertThat(group1.buckets().buckets(), hasSize(1)); GroupBucket onlyBucket = group1.buckets().buckets().iterator().next(); - assertEquals(weight, onlyBucket.weight()); + assertEquals(treatment2, onlyBucket.treatment()); } @Test