Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing support for Q ports in API300::Synergy::LIGUplinkSet, missing support for multiple Synergy frames #216

Closed
1 task done
vranystepan opened this issue Mar 22, 2017 · 26 comments

Comments

@vranystepan
Copy link
Contributor

vranystepan commented Mar 22, 2017

Scenario/Intent

  • I'm trying to create Uplink Set for Virtual Connect SE 40Gb F8 Module for Synergy, oneview-sdk does not support this kind of uplink ports.
  • Also I would like to add ports from more frames, for example Frame 1, ICM3, Q1 and Frame 2, ICM6, Q1. LIGUplinkSet does not support such feature

Environment Details

  • OneView SDK Version: 4.1.0
  • OneView Appliance Version: 3.0 Synergy edition

Steps to Reproduce

Try to create LIGUplinkSet with following uplink ports:

bay: 2
port: 'Q1'

SDK will raise error since API200::LIGUplinkSet can't handle anything with Q prefix.

    def relative_value_of(port)
        identifier = port.slice!(0)
        offset = case identifier
                 when 'D' then 0
                 when 'X' then 16
                 else raise InvalidResource, "Port not supported: #{identifier} type not found"
                 end
        port.to_i + offset
      end

theoretical analysis for Q ports

I've found that Q ports in Virtual Connect SE 40Gb F8 Module for Synergy have following convention for the relative numbering:

Q1 = 61
	Q1:1 = 62
	Q1:2 = 63
	Q1:3 = 64
	Q1:4 = 65
Q2 = 66
	Q2:1 = 67
	Q2:2 = 68
	Q2:3 = 69
	Q2:4 = 70

This also means that we have to distinguish between QN and QN:M ports within the relative_value_of method. So we have to check whether String port contains . or : delimiter.

  • if positive, M will be used as a addition to the base number which will be the same for variants
  • if negative, addition will equal 0

dirty draft (needs A LOT OF improvements :) ):

          port_delimiter = nil
          port_delimiter = '.' if port.include? '.'
          port_delimiter = ':' if port.include? ':'
          if port_delimiter
            port_desc = port.split(port_delimiter)
            port_addition = port_desc[1]
            port_current = port_desc[0]
          else
            port_addition = 0
            port_current = port
          end
          port_number = port_current.gsub(/[^\d]/, '').to_i
          return (61 + ((port_number - 1) * 5)  + port_addition)

theoretical analysis for multiple synergy frames

I guess that we can edit add_uplink method in API200::LIGUplinkSet this way:

      # Specify one uplink passing the virtual connect bay and the port to be attached.
      # @param [Fixnum] bay number to identify the VC
      # @param [String] port to attach the uplink. Allowed D1..D16 and X1..X10
      def add_uplink(bay, port, enclosure_index = 1)
        entry = {
          'desiredSpeed' => 'Auto',
          'logicalLocation' => {
            'locationEntries' => [
              { 'relativeValue' => bay, 'type' => 'Bay' },
              { 'relativeValue' => enclosure_index, 'type' => 'Enclosure' },
              { 'relativeValue' => relative_value_of(port), 'type' => 'Port' }
            ]
          }
        }
        @data['logicalPortConfigInfos'] << entry
      end

Open topics

  • Investigate: Virtual Connect SE 16Gb FC Module for Synergy module also has Q ports. Do these ports have the same offset?

Update for Virtual Connect SE 16Gb FC Module for Synergy:
Each FC module has 4x QSFP ports (these ports can be used with splitter only, so you can allocate QN:M ports only) and 8 SFP+ ports.

QSFP ports relative mapping:

Q1:1 = 21
Q2:1 = 25
Q3:1 = 29
Q4:1 = 33

SFP+ ports relative mapping:

1 = 13
2 = 14
3 = 15
8 = 20

This means that we also have to distinguish between ICM models; Virtual Connect SE 16Gb FC Module for Synergy and Virtual Connect SE 40Gb F8 Module for Synergy ... So maybe we'll override both relevant methods add_uplink & relative_value_of in API300::Synergy?

@vranystepan
Copy link
Contributor Author

I suppose that API300:Synergy::LIGUplinkSet should contain something like:

        def add_uplink(bay, port, icm_model, enclosure_index = 1)
          enclosure_index = -1 if icm_model.include?('Virtual Connect SE 16Gb FC Module for Synergy')
          entry = {
            'desiredSpeed' => 'Auto',
            'logicalLocation' => {
              'locationEntries' => [
                { 'relativeValue' => bay, 'type' => 'Bay' },
                { 'relativeValue' => enclosure_index, 'type' => 'Enclosure' },
                { 'relativeValue' => relative_value_of(port, icm_model), 'type' => 'Port' }
              ]
            }
          }
          @data['logicalPortConfigInfos'] << entry
        end

        def relative_value_of(port, icm_model)
          #  Virtual Connect SE 40Gb F8 Module for Synergy
          # Virtual Connect SE 16Gb FC Module for Synergy
          # insert logic here!
        end

I'm working on some draft. Will send next update soon.

@vranystepan
Copy link
Contributor Author

This might be the solution for both Synergy modules which can have some uplink ports.
API300::Synergy::LIGUplinkSet

I'm not so sure about hard-coded names of Modules ... on the other hand there's probably no other option as we are creating template only.

        def add_uplink(bay, port, icm_model, enclosure_index = 1)
          enclosure_index = -1 if icm_model.include?('Virtual Connect SE 16Gb FC Module for Synergy')
          entry = {
            'desiredSpeed' => 'Auto',
            'logicalLocation' => {
              'locationEntries' => [
                { 'relativeValue' => bay, 'type' => 'Bay' },
                { 'relativeValue' => enclosure_index, 'type' => 'Enclosure' },
                { 'relativeValue' => relative_value_of(port, icm_model), 'type' => 'Port' }
              ]
            }
          }
          @data['logicalPortConfigInfos'] << entry
        end

        def relative_value_of(port, icm_model)
          port_sep = port.match( /[^[:alnum:]]/ ).to_s
          port_cur = port_sep.empty? ? port : port.split(port_sep)[0]
          port_add = port_sep.empty? ? 0 : port.split(port_sep)[1].to_i
          if  icm_model.includes?('Virtual Connect SE 40Gb F8 Module for Synergy')
            port_number = port_cur.gsub(/[^\d]/, '').to_i
            return (61 + ((port_number - 1) * 5)  + port_add)
          end

          if  icm_model.includes?('Virtual Connect SE 16Gb FC Module for Synergy')
            if port_sep.empty?
              return 12 + port_cur.gsub(/[^\d]/, '').to_i
            else
              port_number = port_cur.gsub(/[^\d]/, '').to_i
              return (20 + ((port_number - 1) * 4)  + port_add)
            end
          end
          raise "Unsupported ICM model #{icm_model}"
        end

@jsmartt
Copy link
Collaborator

jsmartt commented Mar 22, 2017

@vranystepan , what about something like this:?

def add_uplink(bay, port, enclosure_index = 1)
  entry = {
    'desiredSpeed' => 'Auto',
    'logicalLocation' => {
      'locationEntries' => [
        { 'relativeValue' => bay, 'type' => 'Bay' },
        { 'relativeValue' => enclosure_index, 'type' => 'Enclosure' },
        { 'relativeValue' => relative_value_of(port, enclosure_index), 'type' => 'Port' }
      ]
    }
  }
  @data['logicalPortConfigInfos'] << entry
end

def relative_value_of(port, enclosure_index = 1)
  match = port.match(/^([A-Za-z])[:\.]?(\d+)[:\.]?(\d*)$/)
  raise(InvalidResource, 'Invalid port format. Should look similar to D1 or Q1.2') unless match
  identifier, offset, offset2 = match.captures
  case identifier.upcase
  when 'D' then offset.to_i
  when 'X' then 16 + offset.to_i
  when 'Q'
    if enclosure_index = -1 # Virtual Connect SE 16Gb FC Module for Synergy
      20 + (offset.to_i - 1) * 4 + offset2.to_i
    else # Virtual Connect SE 40Gb F8 Module for Synergy
      61 + (offset.to_i - 1) * 5 + offset2.to_i
    end
  else raise(InvalidResource, "Port not supported: #{identifier} type not found")
  end
end

It returns:

relative_value_of('X2:1') # 18
relative_value_of('X2:2') # 18
relative_value_of('Q1') # 61
relative_value_of('Q2') # 66
relative_value_of('Q2:2') # 68
relative_value_of('Q1', -1) # 20
relative_value_of('Q1:1', -1) # 21
relative_value_of('Q2:1', -1) # 25
relative_value_of('Q4:1', -1) # 33

I'm not sure if it's enough to just pass the enclosure_index, but for this case, if the 16GB module is the only one that has a -1 enclosure_index value, we can determine with just that whether it's a 40GB or 16GB module, right? I'm not sure if that logic is correct, so please verify.

Also, what are SFP+ ports, and where are you finding the values for all these different port values?

@vranystepan
Copy link
Contributor Author

vranystepan commented Mar 22, 2017

@jsmartt I like your code! Nice and Clean 👍 Yes, you're right, -1 is valid only for FC modules, so this condition will work. I'll test it tomorrow in the actual environment.

All these values come from LIGs created through web interface. So I'm basically fetching them from API.
I was not able to find some mapping resource inside the OneView API so this was the only available method.

I've just realized that I need to double check behaviour of Q ports (40GbVC) when they are configured as FC or FCoE uplinks. Logically they should have the same relative indexes ... but you never know.

  • Test adjusted code in the actual environment
  • Doublecheck relative indexing of Q ports (40GbVC)

@vranystepan
Copy link
Contributor Author

I've done doublecheck. Port numbering is the same even for FC uplinks. So Q1:1 is 62, Q1:2 is 63 ... The only difference is that you cannot use QN ports, FC uplinks can be binded to QN:M only. But this will be handled by OneView.

@jsmartt
Copy link
Collaborator

jsmartt commented Mar 23, 2017

Cool. @vranystepan , were you planning on implementing this feature, or shall I (or someone else)?

@vranystepan
Copy link
Contributor Author

vranystepan commented Mar 23, 2017

@jsmartt I will try to implement it. One question. Is it API200 or API300/Synergy? I suppose we should place it into the Synergy since there are also C7000 modules with Q modules (HPE Virtual Connect FlexFabric-20/40 F8 Module). I guess that these modules have different relative mapping (I will try to get some data, I should have access to some solution with such switches now).

So what about

  • API 200 (update existing method):
def add_uplink(bay, port, enclosure_index = 1)
  entry = {
    'desiredSpeed' => 'Auto',
    'logicalLocation' => {
      'locationEntries' => [
        { 'relativeValue' => bay, 'type' => 'Bay' },
        { 'relativeValue' => enclosure_index, 'type' => 'Enclosure' },
        { 'relativeValue' => relative_value_of(port, enclosure_index), 'type' => 'Port' }
      ]
    }
  }
  @data['logicalPortConfigInfos'] << entry
end
  • API300/Synergy (add a new method):
def relative_value_of(port, enclosure_index = 1)
  match = port.match(/^([A-Za-z])[:\.]?(\d+)[:\.]?(\d*)$/)
  raise(InvalidResource, 'Invalid port format. Should look similar to D1 or Q1.2') unless match
  identifier, offset, offset2 = match.captures
  case identifier.upcase
  when 'D' then offset.to_i
  when 'X' then 16 + offset.to_i
  when 'Q'
    if enclosure_index = -1 # Virtual Connect SE 16Gb FC Module for Synergy
      20 + (offset.to_i - 1) * 4 + offset2.to_i
    else # Virtual Connect SE 40Gb F8 Module for Synergy
      61 + (offset.to_i - 1) * 5 + offset2.to_i
    end
  else raise(InvalidResource, "Port not supported: #{identifier} type not found")
  end
end
  • eventually something similar for API300/C7000 with different offset(s)

@jsmartt
Copy link
Collaborator

jsmartt commented Mar 23, 2017

I don't see why not just add it to API200 only and let the API300 variant classes inherit it. If OneView API v200 doesn't support an offset that the user passes, it will notify the user, so we wouldn't have to worry about that logic. But keeping them all the same will reduce confusion and difficulties with the methods having different parameters. And if the offsets are different in Synergy and C7000 we can set a class constant that can be used in the method(s) so we don't have to re-define them. Does that sound reasonable?

@vranystepan
Copy link
Contributor Author

vranystepan commented Mar 23, 2017

Absolutely. Perhaps I'll try it with something like

self.class.to_s.include? 'C7000'

@vranystepan
Copy link
Contributor Author

vranystepan commented Mar 24, 2017

@jsmartt Couple points about HPE Virtual Connect FlexFabric-20/40 F8 Module, c7000:

  • You can use QN:M ports only
  • QN:M ports starts at relative value 17: Q1:1 = 17, Q2:1 = 21 ...
  • X ports starts at value 33: X1 = 33, X8 = 40 ...

So Q-ports may be handled this way:

            start = self.class.to_s.include?('Synergy') ? 61 : 16
            start + (offset.to_i - 1) * 5 + offset2.to_i

But how to handle different relative positions of X port across various ICM models (for c-class enclosures) ...

@jsmartt
Copy link
Collaborator

jsmartt commented Mar 24, 2017

Can I take a step back here and ask @tmiotto, @fgbulsoni , @ricardogpsf & @aalexmonteiro why exactly we're doing this conversion in the first place? It looks like the API docs accept the value as strings like "Q1.2", so why go through the effort to convert it to an integer? I see it shown as an integer on the GET of a LIG, but the GET for an UplinkSet shows it as a string. The user doesn't care how this gets handled on the back-end, so why do this conversion at all? I know very little about LIGs, so pardon my ignorance if it shows here.

@tmiotto
Copy link
Contributor

tmiotto commented Mar 24, 2017

@jsmartt It is because if you try to handle it within the LIG it must be an integer. This conversion is not for the user, it is used internally by the SDK.

@jsmartt
Copy link
Collaborator

jsmartt commented Mar 24, 2017

😞 bummer

@ChrisLynchHPE
Copy link
Member

The preferred method is to first:

GET /rest/interconnect-types?filter=name={NAME}

Then, look within portInfos for the portName you are trying to get. I would normalize either the string value you are asking the caller for or the portName to a common value (e.g. replace : with . for common naming with the Interconnect Type objects portInfos port object). Once you find the port object, use the portNumber value to build your logical location of the port in the Uplink Set.

This is what I do within the New-HPOVUplinkSet PowerShell Cmdlet when adding Uplink Set objects to LIGs.

@ChrisLynchHPE
Copy link
Member

BTW, you can ask the caller for the Interconnect Module name so you can look up the module object from the API. OR, you ask for the LIG from the user, which then you look for the relativeValue corresponding to the Interconnect module bay ID, then get the permittedInterconnectTypeUri, which will be an Interconnect Type. Then you can use the guidance I gave above looking at the portInfos collection/array for the portName.

@vranystepan
Copy link
Contributor Author

That's brilliant! Thank you.

For example:
GET /rest/interconnect-types?filter=name=%27Virtual%20Connect%20SE%2040Gb%20F8%20Module%20for%20Synergy%27

Returns:

{
  "type": "InterconnectTypeCollectionV300",
  "members": [
    {
      "type": "interconnect-typeV300",
      "partNumber": "794502-B23",
      "minimumFirmwareVersion": "1.0.0",
      "portInfos": [
        {
          "portName": "Q8:1",
          "portNumber": 97,
          "uplinkCapable": false,
          "portCapability": [
            "Ethernet",
            "Stacking"
          ],
...

However we'll need to pass module name to the LIGUplinkSet instance ...

@vranystepan
Copy link
Contributor Author

vranystepan commented May 4, 2017

@jsmartt, @fgbulsoni,
I've done a quick mockup with Synergy frame.

Test file:

# Logical Interconnect Group in the second Bay Set
lig = OneviewSDK::API300::Synergy::LogicalInterconnectGroup.new(c, {
  name: 'LIG-40gb-1Frame-Redundant-01',
  redundancyType: 'Redundant',
  interconnectBaySet: 2,
  enclosureType: "SY12000",
  enclosureIndexes: [1]
})

# One tagged network / VLAN
mgmt_network = OneviewSDK::API300::EthernetNetwork.new(c, { name: 'Management', vlanId:  1000, purpose:  'General', smartLink: false, privateNetwork: false })
mgmt_network.create
lig.add_network(mgmt_network)

# Two Fourty-gig modules
lig.add_interconnect(2, 'Virtual Connect SE 40Gb F8 Module for Synergy', nil, 1)
lig.add_interconnect(5, 'Virtual Connect SE 40Gb F8 Module for Synergy', nil, 1)

# One Uplink Set
uplink_set = OneviewSDK::API300::LIGUplinkSet.new(c, {
  name: 'UplinkSet1',
  networkType: 'Ethernet',
  primaryPort: nil
})

# Two uplink ports 
uplink_set.add_uplink(2,'Q1', 'Virtual Connect SE 40Gb F8 Module for Synergy')
uplink_set.add_uplink(5,'Q1', 'Virtual Connect SE 40Gb F8 Module for Synergy')

# Add Uplink Set to the Logical Interconnect Group
lig.add_uplink_set(uplink_set)

# Create Logical Interconnect Group
lig.create

api200/lig_uplink_set.rb:

...
      # Specify one uplink passing the virtual connect bay and the port to be attached.
      # @param [Fixnum] bay number to identify the VC
      # @param [String] port to attach the uplink. Allowed D1..D16 and X1..X10
      def add_uplink(bay, port, model = nil, enclosure_index = nil)
        enclosure_index ||= model.include?('Virtual Connect SE 16Gb FC Module') ? -1 : 1
        if model
          port = fetch_relative_value_of(port, model)
        else
          port = (port.to_s == port.to_i.to_s) ? port : relative_value_of(port)
        end
        entry = {
          'desiredSpeed' => 'Auto',
          'logicalLocation' => {
            'locationEntries' => [
              { 'relativeValue' => bay, 'type' => 'Bay' },
              { 'relativeValue' => enclosure_index, 'type' => 'Enclosure' },
              { 'relativeValue' => port, 'type' => 'Port' }
            ]
          }
        }
        @data['logicalPortConfigInfos'] << entry
      end

...

      def fetch_relative_value_of(port, model)
        port = port.sub('.',':') # normalize naming if needed
        interconnect_type = OneviewSDK::Interconnect.get_type(@client, model)
        unless interconnect_type
          list = OneviewSDK::Interconnect.get_types(@client).map { |t| t['name'] }
          raise OneviewSDK::NotFound, "Interconnect type #{type} not found! Supported types: #{list}"
        end
        type_port = interconnect_type['portInfos'].find { |p| p['portName'] == port }
        type_port['portNumber']
      end

Unfortunately I don't have OneView 2.0 with c7000 or OneView 3.0 with c7000 to test other scenarios :(
As I stated before, This is a pretty tough way but we probably can't do this differently when we have LIGUplinkSet and LogicalInterconnectGroup separately.

Another possible solution would be to merge these classes to LogicalInterconnectGroup

@jsmartt
Copy link
Collaborator

jsmartt commented May 4, 2017

Nice! That looks promising. If that method needs to be used in 2 separate resources, you may also want to consider making it a class method (with or without an instance method that wraps it).

As for the c7000 HW, can you use the simulator to get what you need?

@vranystepan
Copy link
Contributor Author

vranystepan commented May 4, 2017

I've just realized that that I need just OneView Virtual Machines without hardware. Will do some functionality tests.

At this point I don't think we need to call this method from more resources, I was just thinking about add_uplink method which will require the third parameter with the model name... which can be annoying. If this is not an issue - let's go in this direction.

Also, I forgot to add enclosure_index. Will play with it tomorrow :)

@vranystepan
Copy link
Contributor Author

@jsmartt @fgbulsoni,
Haha, funny. Synergy Q ports use QN:M naming and guess what: c7000 modules (for example 'HP VC FlexFabric-20/40 F8 Module') use QN.M 😄

Anyway these mothods should be able to handle all 3 scenarios:

  1. legacy functionality with X and d ports
  2. "unpredictable" naming of modules with Q ports (especially in new Synergy modules)
  3. Integer naming - port names which do not require any translation ( Method add_uplink for LIG Uplinks Set does not work with Q ports nor integer ports #228 )

add_uplink method:

      def add_uplink(bay, port, model = nil, enclosure_index = nil)
        enclosure_index ||= model.include?('Virtual Connect SE 16Gb FC Module') ? -1 : 1
        port =
          if model
            fetch_relative_value_of(port, model)
          else
            port.to_s == port.to_i.to_s ? port : relative_value_of(port)
          end
        entry = {
          'desiredSpeed' => 'Auto',
          'logicalLocation' => {
            'locationEntries' => [
              { 'relativeValue' => bay, 'type' => 'Bay' },
              { 'relativeValue' => enclosure_index, 'type' => 'Enclosure' },
              { 'relativeValue' => port, 'type' => 'Port' }
            ]
          }
        }
        @data['logicalPortConfigInfos'] << entry
      end

def fetch_relative_value_of method:

      def fetch_relative_value_of(port, model)
        port_formats = [port.sub('.', ':'), port.sub(':', '.')].uniq
        interconnect_type = OneviewSDK::Interconnect.get_type(@client, model)
        unless interconnect_type
          list = OneviewSDK::Interconnect.get_types(@client).map { |t| t['name'] }
          raise OneviewSDK::NotFound, "Interconnect type #{type} not found! Supported types: #{list}"
        end
        type_port = interconnect_type['portInfos'].find { |p| port_formats.include? p['portName'] }
        raise OneviewSDK::NotFound, "Port #{port} not found" unless type_port
        type_port['portNumber']
      end

I've done functionality tests with OneView 3.0 (20/40 F8 modules), OneView 3.0 Synergy edition (40Gb VC and 16Gb FC VC), even the multi-frame scenario (with 40Gb VC) is working fine.

@jsmartt
Copy link
Collaborator

jsmartt commented May 5, 2017

Nice! Can you submit a PR when you feel you're ready @vranystepan ?

@fgbulsoni
Copy link
Contributor

fgbulsoni commented May 5, 2017

Anyway these methods should be able to handle all 3 scenarios:

💯 ✨ Awesome job @vranystepan ! Waiting anxiously for the PR. :octocat:

🎗️ Don't forget the unit tests update and to include issues #216 and #228 as solved in the CHANGELOG

@vranystepan
Copy link
Contributor Author

@fgbulsoni @jsmartt working on it. Do I have enough permissions in this repo?
And secondly, where should I put these Bug fixes & Enhancements in CHANGELOG?

@fgbulsoni
Copy link
Contributor

Do I have enough permissions in this repo?

@vranystepan Can you try the Fork it, branch it, change it, commit it, and pull-request it. way?
Otherwise I can give permissions if need be.

And secondly, where should I put these Bug fixes & Enhancements in CHANGELOG?

Since this is a bugfix/enhancement and I don't think we've added any other resource to be released, something like this should be good for the changelog:

Unreleased Changes

Suggested release: v4.3.1

Bug fixes & Enhancements:

  • #216 Missing support for Q ports in API300::Synergy::LIGUplinkSet, missing support for multiple Synergy frames
  • #228 Method add_uplink for LIG Uplinks Set does not work with Q ports nor integer ports

@vranystepan
Copy link
Contributor Author

@fgbulsoni ahh, now I got your point :) I've raised a PR #230

@vranystepan
Copy link
Contributor Author

merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants