Skip to content

Enhance qos tests to support single-asic, multi-asic, and multi-dut testing#6946

Closed
sanmalho-git wants to merge 5 commits intosonic-net:masterfrom
sanmalho-git:qos_new
Closed

Enhance qos tests to support single-asic, multi-asic, and multi-dut testing#6946
sanmalho-git wants to merge 5 commits intosonic-net:masterfrom
sanmalho-git:qos_new

Conversation

@sanmalho-git
Copy link
Contributor

@sanmalho-git sanmalho-git commented Dec 2, 2022

Description of PR

Summary:
Fixes # (issue)
The existing QoS (test_qos_sai.py) is written to accomodata a single asic on a single Dut. But, we require the same tests to be executed against a T2 chassis (with single/multi-asic linecards) and multi-asic pizza boxes.

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 201911
  • 202012
  • 202205

Approach

What is the motivation for this PR?

All the test cases create a list of src and dst ports. For the different modes, here is the distribution of the src and dst ports:

  • single_asic: The src and dst ports are on the same asic on the same linecard.
  • single_dut_multi_asic: On a multi-asic DUT/linecard, the src port is on an asic, while the dst ports are on another asic on the same DUT/linecard
  • multi_dut: The src port is on an asic on one of the DUT/linecards, and the dst port is on another asic on another DUT/linecard. This is currently only required for T2 topology

How did you do it?

Approach to accomplish this is the following:

  • All the tests have to parameterized for the 3 modes defined above.

    • This is done using the 'select_src_and_dst_dut_and_asic' fixture that is parameterized for 'single_asic', 'single_dut_multi_asic', 'multi_dut' - Based on the mode, it sets the src_dut_index, dst_dut_index, src_asic_index and dst_asic_index
    • Added fixture 'get_src_dst_asic_and_duts' that returns dictionary of the src_dut_index, dst_dut_index, src_asic_index, and dst_asic_index, and the src_dut and dst_dut (instances of MultiAsicSonicHost), src_asic and dst_asic (instances of Asic), and also a list of all DUTs and all Asics
  • dutConfig is modified such that testPortIds and testPortIps are collecting from all the duts and asics involved and stored in a dictionary with key being the dutIndex and value being a dictionary per asic index.

    • __buildTestPorts then sets the src and dst ports based on the src_dut_index, dst_dut_index, src_asic_index and dst_asic_index
  • All the other fixtures and tests, we use 'get_src_dst_asci_and_duts' fixture instead of enum_rand_one_frontend_hostname and enum_frontend_index.

    • The code instead the fixtures and tests is modified to the actions on the correct src/dst dut or asic. For example: - swap_syncd fixture would swap syncd docker on all DUT's (both src and dst) instead of just one DUT as before. - stopServices - do it all_duts (src and dst duts)
  • Similarly, changes to saitests involved dealing with multiple DUTs (and thus multiple sai clients) and modifying other data structure like 'interface_to_front_mapping' in sai_base_test.py and port_list, sai_port_list, front_port_list in switch.py to deal with multiple duts (modified to be dictionary with keys being 'src' and 'dst')

    • default_interface_to_front_map.ini file in ptf modified to have syntax: ptf_port@dut_port:dut_index - Done by using new fixture 'ptf_portmap_file_all_frontend_nodes' in ptfhost_utils.py
    • tests in sai_qos_tests.py pass src_dut_index and dst_dut_index in the testParams.
      • The saitests classes then use this to do the actions on the right client and ports.
  • Assumptions:

    • For multi-dut, we are assuming that hwsku for all the cards are same.

How did you verify/test it?

Ran the tests against T2 J2C+ chassis.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@sanmalho-git sanmalho-git marked this pull request as draft December 2, 2022 04:32
@azure-pipelines
Copy link

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/qos/files/qos.yml
Fixing tests/qos/qos_sai_base.py
Fixing tests/qos/qos_helpers.py
Fixing tests/conftest.py
Fixing tests/qos/test_qos_sai.py
Fixing tests/saitests/py3/sai_qos_tests.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/test_pretest.py
Fixing tests/saitests/py3/sai_qos_tests.py
...
[truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2022

This pull request introduces 7 alerts and fixes 1 when merging fc5697ac3869dc23335489501662dfcd2c5a3fc1 into 44880ce - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 2 for Except block handles 'BaseException'
  • 2 for Unused import
  • 1 for Variable defined multiple times

fixed alerts:

  • 1 for Unused import

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@wenyiz2021
Copy link
Contributor

can you please add test result in PR description? thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we keeping ecn/wrr params for 400G and not for 100G ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in the latest qos.yml file committed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still seeing no ecn params for 100g but available for 400g. Is this intentional ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this code here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will it not impact existing test case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in latest commit

Copy link
Contributor

Choose a reason for hiding this comment

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

Please enable both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in latest commit

Copy link
Contributor

Choose a reason for hiding this comment

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

please re-check if there is duplicate code.

@sanmalho-git sanmalho-git marked this pull request as ready for review December 22, 2022 17:31
Copy link
Contributor

@vmittal-msft vmittal-msft left a comment

Choose a reason for hiding this comment

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

Can you please separate out changes for single asic from multi asic/dut enhancements and raise a different PR ? It has been very challenging to verify this one big PR for T0, T1, T2-single asic, T2-multi asic enviroments ?

@rlhui
Copy link

rlhui commented Feb 1, 2023

what's pending for this PR? Thanks.

@sanmalho-git
Copy link
Contributor Author

Here is the status on this PR:

  • T2 and T1 testing is done and looks good.
  • Vineet is testing out T0 and dualTor topologies.
  • need confirmation from BRCM to push the cint scripts to community.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I give "run_tests.sh -n TB_Name -d DUT_2_name -c "qos/test_qos_sai.py" The TB has 3 DUTs, and I have given the second DUT.

This fails:
# PTF port is mapped to single DUT
target_dut_index = int(list(dut_intf_map.keys())[0])
target_dut_port = int(list(dut_intf_map.values())[0])

          router_mac = router_macs[target_dut_index]

E IndexError: list index out of range

a_dut_port = 'Ethernet64'
a_dut_port_index = 8
active_active_ports_mux_status = {}
active_dut_map = {}
asic_idx = 0
disabled_ptf_ports = set([])
dut_intf_map = {'1': 27}
dut_port = 'Ethernet64'
duthost =
duthosts = []
duts_minigraph_facts = {'sfd-vt2-lc1': [{'deployment_id': None, 'dhcp_servers': [], 'dhcpv6_servers': [], 'forced_mgmt_routes': [], ...}, {'d...t_routes': [], ...}, {'deployment_id': None, 'dhcp_servers': [], 'dhcpv6_servers': [], 'forced_mgmt_routes': [], ...}]}
duts_running_config_facts = {'sfd-vt2-lc1': [{'ACL_TABLE': {'DATAACL': {'policy_desc': u'DATAACL', 'ports': [u'PortChannel101', u'PortChannel103',..._limit_interval': u'600', 'state': u'enabled'}, ...}, 'BGP_DEVICE_GLOBAL': {'STATE': {'tsa_enabled': u'false'}}, ...}]}
idx = 0
mg_facts = {'deployment_id': None, 'dhcp_servers': [], 'dhcpv6_servers': [], 'forced_mgmt_routes': [], ...}
mux_server_url = ''
ports_map = {'0': {'asic_idx': 0, 'dut_port': 'Ethernet0', 'target_dest_mac': 'e8:d3:22:30:22:1a', 'target_dut': [0], ...}, '20': ... '22': {'asic_idx': 1, 'dut_port': 'Ethernet176', 'target_dest_mac': 'e8:d3:22:30:22:1b', 'target_dut': [0], ...}, ...}
ptf_port = '59'
ptfhost = <tests.common.devices.ptf.PTFHost object at 0x7f7ae9fee3d0>
router_mac = 'e8:d3:22:30:22:1a'
router_macs = ['e8:d3:22:30:22:18']
target_dut_index = 1
target_dut_port = 27
target_hostname = 'sfd-vt2-lc1'
tbinfo = {'auto_recover': 'True', 'comment': 'Tests SFD T2 - Vanguard setup', 'conf-name': 'sfd-vt2', 'duts': ['sfd-vt2-lc0', 'sfd-vt2-lc1', 'sfd-vt2-lc2', 'sfd-vt2-sup'], ...}

Copy link
Contributor

Choose a reason for hiding this comment

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

when there is not 'tc_to_dscp_map' config on dut, json.loads will rasise a exception because of loading none/empty .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in latest commit

…esting (sandeep: PR#6946)

Cleanup for QoS

Minor fixes for Qos tests

Changes to support QoS multi-asic and ixes to QoS tests for single-asic

 - Ignoring qos/test_buffer.py for T2 and allowing to run test_qos_sai.py on our chassis
 - Changes to support QoS multi-asic

Run only single-asic QoS tests

Integrating cint calls into qos tests

Fixes for QoS multi-asic support

qos masic - docker services should be updated on both src and dst asics

it was being done only on the src asic

More fixes for QoS tests

Adding missing import of 'socket' to sai_qos_base.py file

More fixes to QoS tests based on PR creation

Final fixes for Qos tests with rebase

Fixes for multi-dut and multi-asic in ReleaseAllPorts

Adding missing texttable.py file needed for QoS tests

Fixing typo in QoS tests

Embedding 'show counter' calls in PgSharedWatermark qos test

Adding missing QoS file

Fixing select fixture for QoS test

Fixing the path used for docker-sync-rpc image for QoS tests

qos tuning for LossyQueue parameter

Fixes for PFCXon test

QoS changes after rebase

double commit PR sonic-net#7109 sonic-net#7119 sonic-net#7140 sonic-net#7154 (sonic-net#7173)

Changes to DscpToPgmapping and PgSharedWatermarkTest for qos

Updating cint scripts and validating successfully executed

Adding sleep before checking stats for QoS tests

Change PgSharedWatermark_test assert stmt

More fixes for QoS tests

Fixes for QoS QSharedWatermarkTest test

Need to ignore one of the asserts as SAI_INGRESS_PRIORITY_GROUP_STAT_XOFF_ROOM_WATERMARK_BYTES is not supported on DNX

Enabling multi-asic/multi-dut and single-asic mode for QoS tests

All the fixes are in libsai that comes with 202205 - so we can run all
the tests in our weekend pipeline

QoS rebase fixes

Removing ptf_dut_ip - internal Nokia related code

Fixing issue created via rebase

Latest fixes
@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/qos/qos_helpers.py
Fixing tests/saitests/py3/sai_qos_tests.py
Fixing tests/qos/test_qos_sai.py
Fixing tests/qos/files/mellanox/qos_param_generator.py
Fixing tests/test_pretest.py
Fixing tests/qos/qos_sai_base.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/saitests/py3/sai_qos_tests.py

check yaml...........................................(no files to check)Skipped
...
[truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/qos/qos_helpers.py
Fixing tests/saitests/py3/sai_qos_tests.py
Fixing tests/qos/test_qos_sai.py
Fixing tests/qos/files/mellanox/qos_param_generator.py
Fixing tests/test_pretest.py
Fixing tests/qos/qos_sai_base.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/saitests/py3/sai_qos_tests.py

check yaml...........................................(no files to check)Skipped
...
[truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/qos/qos_helpers.py
Fixing tests/saitests/py3/sai_qos_tests.py
Fixing tests/qos/test_qos_sai.py
Fixing tests/qos/files/mellanox/qos_param_generator.py
Fixing tests/test_pretest.py
Fixing tests/qos/qos_sai_base.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/saitests/py3/sai_qos_tests.py

check yaml...........................................(no files to check)Skipped
...
[truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The detected issues may be old or new. For new issues, please try to fix them.

For old issues, it is not mandatory to fix them because they were not caused by this change. It is unfair to blame
author of this pull request. But if you can take extra effort to fix the old issues as well, that would be great!

Detailed pre-commit check results:
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing tests/qos/qos_helpers.py
Fixing tests/saitests/py3/sai_qos_tests.py
Fixing tests/qos/test_qos_sai.py
Fixing tests/qos/files/mellanox/qos_param_generator.py
Fixing tests/test_pretest.py
Fixing tests/qos/qos_sai_base.py

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/saitests/py3/sai_qos_tests.py

check yaml...........................................(no files to check)Skipped
...
[truncated extra lines, please run pre-commit locally to view full check results]

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

src_ports = dutConfig['testPortIds'][src_dut_index][src_asic_index]
if get_src_dst_asic_and_duts['src_asic'] == get_src_dst_asic_and_duts['dst_asic']:
# Src and dst are the same asics, leave one for dst port and the rest for src ports
qosConfig["hdrm_pool_size"]["src_port_ids"] = src_ports[:-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

This change causes failure on mellanox platform.
It expands port numbers of "qosConfig["hdrm_pool_size"]["src_port_ids"]".
But "qosConfig["hdrm_pool_size"].["pkts_num_trig_pfc_shp"] didn't expand accordingly.
Finally, caused "index out of range" error in ptf, as below:

"======================================================================",
"ERROR: sai_qos_tests.HdrmPoolSizeTest",
"----------------------------------------------------------------------",
"Traceback (most recent call last):",
"  File \"saitests/py3/sai_qos_tests.py\", line 2160, in runTest",
"    pkts_num_trig_pfc = self.pkts_num_trig_pfc_shp[i]",
"IndexError: list index out of range",
"",
"----------------------------------------------------------------------",
"Ran 1 test in 145.581s",
"",
"FAILED (errors=1)"]

For better understand error, share the my qos param for running mellanox qos test:

relevant qos param value before this change:

pkts_num_trig_pfc_shp=[50182, 25192, 12697, 6450, 3326, 1764, 983, 593, 398, 300, 251, 227, 215, 209]
src_port_ids=[2, 3, 4, 5, 6, 7, 8]

relevant qos parame value after apply this change:

pkts_num_trig_pfc_shp=[50182, 25192, 12697, 6450, 3326, 1764, 983, 593, 398, 300, 251, 227, 215, 209];
src_port_ids=[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23];

recv_counters, queue_counters = sai_thrift_read_port_counters(
self.src_client, self.asic_type, port_list['src'][self.src_port_id])
xmit_counters, queue_counters = sai_thrift_read_port_counters(
self.dst_client, self.asic-type, port_list['dst'][self.dst_port_id])
Copy link
Contributor

Choose a reason for hiding this comment

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

typo issue: "self.asic-type", should correct to "self.asic_type"

caused test failure as below:

"======================================================================",
"ERROR: sai_qos_tests.PtfReleaseBuffer",
"----------------------------------------------------------------------",
"Traceback (most recent call last):",
"  File \"saitests/py3/sai_qos_tests.py\", line 1542, in runTest",
"    self.dst_client, self.asic-type, port_list['dst'][self.dst_port_id])",
"AttributeError: 'PtfReleaseBuffer' object has no attribute 'asic'",
"",

@XuChen-MSFT XuChen-MSFT requested a review from stephenxs April 8, 2023 03:11
@XuChen-MSFT
Copy link
Contributor

Hi @stephenxs

Partial of changes in this PR are related to qos sai test on mellanox platform, could you please to take a look as well.?

@vmittal-msft
Copy link
Contributor

Ported changes from this PR to #8149
Will close this once 8148 gets merged.

@mlok-nokia
Copy link
Contributor

mlok-nokia commented May 31, 2023

This PR is no longer needed

@mlok-nokia
Copy link
Contributor

This PR has been replaced by multiple PRs. This one should be closed or ignored.

@vmittal-msft
Copy link
Contributor

Closing this PR as changes are merged to master branch.

wangxin pushed a commit that referenced this pull request Aug 29, 2023
…le_dut_multi_asic and multi_dut (#8222) (#9703)

This PR is in continuation of PR# #8149
which was originally part of PR# #6946

The existing QoS (test_qos_sai.py) is written to accommodate a single asic on a single Dut. But, we require the same tests to be executed against a T2 chassis (with single/multi-asic linecards) and multi-asic pizza boxes.

What is the motivation for this PR?
1.Qos test cases failed with intermittent errors

How did you do it?
Two issues are addressed here :

1.The dscp queue mapping for LossyQueue Test changed in config file to map to queue 1 of traffic-class instead of  0  since disabling the tx and filling up the queue 0  prevents the lacp packets going out and port channel goes down

2.During Qos test on transmission disable and enable, sometimes on test failure the port dangles in a transmission disable state and did not recover. Switching the step to enable the transmission port before the BCMSAI credit-watchdog enable , eradicate the test failure due to bad port state.

How did you verify/test it?
Executed qos testcases on for single_asic ,single_dut_multi_asic & multi_dut

Co-authored-by: ansrajpu-git <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.