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

Use /var/tmp instead of /tmp for sonic_installer and generate_dump #985

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

olivier-singla
Copy link
Contributor

sonic_installer and generate_dump utilities use currently /tmp directory
to store temporary files. Since the amount of data stored can be rather
large (especially in the case of the sonic_installer), we are switching to
use /var/tmp instead.

We are doing these changes in anticipation that at some point /tmp will being
mounted over TMPFS

/tmp is typically mounted on TMPFS, which means it's fast and does not produce
wear on SSD or Hard-Drive. /var/tmp is mounted on the rootfs partition,
therefore not in the TMPFS partition.

We also noticed that both sonic_installer and generate_dump produce temporary
files which are not deleted once thse programes are done. We are then making
changes to peform a clean-up and remove the temporary files not needed anymore.

Please enter the commit message for your changes. Lines starting

with '#' will be ignored, and an empty message aborts the commit.

Date: Sat Jul 11 19:58:59 2020 -0400

On branch var_tmp

Changes to be committed:

modified: scripts/generate_dump

modified: sonic_installer/bootloader/aboot.py

modified: sonic_installer/bootloader/grub.py

modified: sonic_installer/bootloader/onie.py

modified: sonic_installer/bootloader/uboot.py

modified: sonic_installer/common.py

modified: sonic_installer/main.py

- What I did

- How I did it

- How to verify it

- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)

sonic_installer and generate_dump utilities use currently /tmp directory
to store temporary files. Since the amount of data stored can be rather
large (especially in the case of the sonic_installer), we are switching to
use /var/tmp  instead.

We are doing these changes in anticipation that at some point /tmp will being
mounted over TMPFS

/tmp is typically mounted on TMPFS, which means it's fast and does not produce
wear on SSD or Hard-Drive. /var/tmp is mounted on the rootfs partition,
therefore not in the TMPFS partition.

We also noticed that both sonic_installer and generate_dump produce temporary
files which are not deleted once thse programes are done. We are then making
changes to peform a clean-up and remove the temporary files not needed anymore.

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date:      Sat Jul 11 19:58:59 2020 -0400
#
# On branch var_tmp
# Changes to be committed:
#	modified:   scripts/generate_dump
#	modified:   sonic_installer/bootloader/aboot.py
#	modified:   sonic_installer/bootloader/grub.py
#	modified:   sonic_installer/bootloader/onie.py
#	modified:   sonic_installer/bootloader/uboot.py
#	modified:   sonic_installer/common.py
#	modified:   sonic_installer/main.py
#
@lgtm-com
Copy link

lgtm-com bot commented Jul 12, 2020

This pull request introduces 2 alerts when merging 3909818 into fad6ad5 - view on LGTM.com

new alerts:

  • 2 for Unused import

@jleveque
Copy link
Contributor

jleveque commented Jul 12, 2020

We are doing these changes in anticipation that at some point /tmp will being
mounted over TMPFS

/tmp is typically mounted on TMPFS, which means it's fast and does not produce
wear on SSD or Hard-Drive. /var/tmp is mounted on the rootfs partition,
therefore not in the TMPFS partition.

I'm not clear on what the benefit of this PR is. I can see benefit in cleaning up the temporary files once the applications are done with them, but why change the location to /var/tmp? As you mentioned if /tmp is eventually mounted in tmpfs, there will be less wear and tear on the hard disk, so I believe tmpfs is actually the better solution here. Plus, on devices with small hard disks, it will be beneficial, if not critical, to keep temporary files off the hard disk, thus keeping the location as /tmp and mounting /tmp as tmpfs should be the ultimate goal.

@olivier-singla
Copy link
Contributor Author

We are doing these changes in anticipation that at some point /tmp will being
mounted over TMPFS

/tmp is typically mounted on TMPFS, which means it's fast and does not produce
wear on SSD or Hard-Drive. /var/tmp is mounted on the rootfs partition,
therefore not in the TMPFS partition.

I'm not clear on what the benefit of this PR is. I can see benefit in cleaning up the temporary files once the applications are done with them, but why change the location to /var/tmp? As you mentioned if /tmp is eventually mounted in tmpfs, there will be less wear and tear on the hard disk, so I believe tmpfs is actually the better solution here. Plus, on devices with small hard disks, it will be beneficial, if not critical, to keep temporary files off the hard disk, thus keeping the location as /tmp and mounting /tmp as tmpfs should be the ultimate goal.

The idea to use /var/tmp was to minimize the impact of moving /tmp to tmpfs, and have the two utilities, sonic_installer and generate_dump, to perform as currently (/tmp over SSD or HD). This said, I can revert to use /tmp instead of using /var/tmp

@rajendra-dendukuri
Copy link
Contributor

We are doing these changes in anticipation that at some point /tmp will being
mounted over TMPFS

/tmp is typically mounted on TMPFS, which means it's fast and does not produce
wear on SSD or Hard-Drive. /var/tmp is mounted on the rootfs partition,
therefore not in the TMPFS partition.

I'm not clear on what the benefit of this PR is. I can see benefit in cleaning up the temporary files once the applications are done with them, but why change the location to /var/tmp? As you mentioned if /tmp is eventually mounted in tmpfs, there will be less wear and tear on the hard disk, so I believe tmpfs is actually the better solution here. Plus, on devices with small hard disks, it will be beneficial, if not critical, to keep temporary files off the hard disk, thus keeping the location as /tmp and mounting /tmp as tmpfs should be the ultimate goal.

It is typical to have systems with larger hard disks relative to the RAM they have. For certain applications like sonic_installer, we need to store the temporary file in persistent storage while we extract it. If we keep the temporary file in RAM and then extract it, we will need more free space in RAM. So it is ideal that large temporary files be stored on disk and manageable sized files can go in /tmp which is proposed to be changed to a tmpfs. That is the reason for changing sonic_installer to /var/tmp.

@jleveque jleveque requested a review from lguohan July 12, 2020 18:41
@jleveque
Copy link
Contributor

It is typical to have systems with larger hard disks relative to the RAM they have.

Typically, yes. However, there are a handful of SONiC-supported platforms where the RAM is 4 GB and the SDD is also 4 GB or even smaller (e.g., 2 GB). These platforms would actually be harmed by this change rather than helped.

I'd like @lguohan's to provide his opinion here, as well.

@rajendra-dendukuri
Copy link
Contributor

It is typical to have systems with larger hard disks relative to the RAM they have.

Typically, yes. However, there are a handful of SONiC-supported platforms where the RAM is 4 GB and the SDD is also 4 GB or even smaller (e.g., 2 GB). These platforms would actually be harmed by this change rather than helped.

No new harm will be done due this PR. The switches were already using /tmp which is currently mounted on disk. We are just moving it to /var/tmp which is also on disk. Essentially it is a no-op.

xumia and others added 3 commits July 13, 2020 14:19
* Support to verify reboot for secure boot

* Support to verify reboot for secure boot

* Fix type issue

* Change the verification command to verify-next-image

* Fix python function error

* Fix Host_PATH case issue

* Fix some messages issue

* Remove not neccessary message

* not to import no use module

* Change command to use hyphens

* Simplify the command verify-next-image

* Add fast-reboot exit code

* Improve the verification message
…d boot) as part of db migration (sonic-net#986)

* Fix for command. show interface transceiver eeprom -d Ethernet
Make sure key is present in dom_info_dict and then only parse
else skip.

* Fix the None Type Exception when Interface Table does not exist (cold boot) as part of db migration
…mmands (sonic-net#983)

Replace underscores with hyphens in all sonic-installer subcommands as well as the root command itself.

- Install two entry points for sonic-installer: `sonic-installer` and `sonic_installer`. This allows for backward compatibility and time for users to transition from using the underscore versions to the hyphenated versions.
- Replace underscores with hyphens in the following subcommands:
    ```
    set-default
    set-next-boot
    binary-version
    upgrade-docker
    rollback-docker
    ```
    - Also add aliases from the underscore versions of these subcommnds to the new hyphenated subcommands to allow for backward compatibility and time for users to transition from using the underscore versions to the hyphenated versions.

- Print deprecation warnings to stderr if issued command line contains any deprecated versions with underscores
- Bonus: Addition of the AliasedGroup base class also adds support for abbreviated subcommands
- Fixed some style issues which did not align with PEP8 standards
- Eliminate duplicate code by adding DOCKER_CONTAINER_LIST; Remove unknown 'amon' container from the list
@olivier-singla
Copy link
Contributor Author

I'd like @lguohan's to provide his opinion here, as well.

I was wondering, did you had a chance to review this change?

shlomibitton and others added 5 commits July 16, 2020 10:26
- What I did
Add support for QSFP-DD cables on 'sfpshow' script
Adapt sfp_test to new eeprom output.

- How I did it
Added a new label "application_advertisement" to print from DB.
Exclude "specification compliance" print from QSFP-DD cables.

Signed-off-by: Shlomi Bitton <shlomibi@mellanox.com>
- Support protocol number == 0
- Emit warning if --table_name not found in Config DB

Signed-off-by: Danny Allen <daall@microsoft.com>
sonic-net#995)

Root group name was changed from `cli` to `sonic_installer` in sonic-net#983. However, `verify-next-image` subcommand was being added in an already-open PR sonic-net#979 at the time. It did not get updated before merge. This aligns it with the new group name.
Add telemetry service to the list of services to stop/reset-failed/restart during config load/reload operations.
Added breakout subcommand in config command

Signed-off-by: Sangita Maity <sangitamaity0211@gmail.com>
@rajendra-dendukuri
Copy link
Contributor

@lguohan can you please review this change.

samaity and others added 12 commits July 29, 2020 23:51
…et#1002)

Adding a PortChannel to a Vlan group, will change the 'Vlan' tag to 'trunk'.
Signed-off-by: Shlomi Bitton <shlomibi@mellanox.com>
…ng() (sonic-net#1012)

Fix typo and enable test for tablesWithOutYang()

Signed-off-by: Praveen Chaudhary pchaudhary@linkedin.com
Fix Azure/sonic-buildimage#5019

Fix "the input device is not a TTY" error reported by 'show lldp' commands when executed via pytest scripts in sonic-mgmt.
a few tests in drops_group_test fails on second run.
The reason is that /tmp/dropstat is not cleaned, the first
run leave some state in the folder which cause the subsequent
run to fail.

The fix is the always clean up the folder.

Signed-off-by: Guohan Lu <lguohan@gmail.com>
)

merge container feature cli into feature cli

config command:
```
admin@vlab-01:~$ sudo config feature
Usage: config feature [OPTIONS] COMMAND [ARGS]...

  Modify configuration of features

Options:
  -?, -h, --help  Show this message and exit.

Commands:
  autorestart  Configure autorestart status for a feature
  state        Configure status of feature
```

show command:
```
admin@vlab-01:~$ show feature
Usage: show feature [OPTIONS] COMMAND [ARGS]...

  Show feature status

Options:
  -?, -h, --help  Show this message and exit.

Commands:
  autorestart  Show auto-restart status for a feature
  status       Show feature status
```

output:

```
admin@vlab-01:~$ show feature status
Feature     Status    AutoRestart
----------  --------  -------------
lldp        enabled   enabled
pmon        enabled   enabled
sflow       disabled  enabled
database    enabled   disabled
restapi     disabled  enabled
telemetry   enabled   enabled
snmp        enabled   enabled
bgp         enabled   enabled
radv        enabled   enabled
dhcp_relay  enabled   enabled
nat         enabled   enabled
teamd       enabled   enabled
syncd       enabled   enabled
swss        enabled   enabled
admin@vlab-01:~$ show feature autorestart
Feature     AutoRestart
----------  -------------
lldp        enabled
pmon        enabled
sflow       enabled
database    disabled
restapi     enabled
telemetry   enabled
snmp        enabled
bgp         enabled
radv        enabled
dhcp_relay  enabled
nat         enabled
teamd       enabled
syncd       enabled
swss        enabled
```

Signed-off-by: Guohan Lu <lguohan@gmail.com>
Signed-off-by: Guohan Lu <lguohan@gmail.com>
…nic-net#1028)

also introduce fixtures to setup the cmd as well as asic context

Signed-off-by: Guohan Lu <lguohan@gmail.com>
)

Create environment files for SONiC immutable attribute. The file
will reside in the incoming image dir under sonic-config dir.
Incoming image will then move the file to /etc/sonic. The file
will be used to avoid calls into cfggen for those attributes.

signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
Instead of having multiple implementation of preparing a SWI image for
secureboot, fast-reboot now reuses boot0.
SWI images booting in regular mode will keep using the old behavior.
Add unit tests for 'show platform ...' commands
FuzailBrcm and others added 28 commits August 6, 2020 11:09
…ith pddf_2.0 (sonic-net#940)

Added PDDF utilities changes to make them compatible with 2.0 platform APIs. Since PDDF 2.0 is supporting platform 2.0 APIs, we needed to move the utilities to support the same.

Signed-off-by: Fuzail Khan <fuzail.khan@broadcom.com>
it is better not let every command to have its
own connector and connect to db.

it is also not good to have a global db variable.

Here, the idea is to have a single db connector
and pass the connector as a click context

Signed-off-by: Guohan Lu <lguohan@gmail.com>
…sonic-net#978)

* QoS and Buffer config genration for multi ASIC platforms. 

In case of multi ASIC platforms each ASIC has its own buffer and QoS 
configuration template that is used to populate corresponding ASIC's config DB. 
Made changes so that QoS config commands generates QoS and buffer configs for
all ASICs. Also the 'config qos clear' acts on all ASIC instances.
…ge (sonic-net#1008)

As part of consolidating all common Python-based functionality into the new sonic-py-common package, this pull request redirects all Python applications/scripts in sonic-utilities repo to point to sonic-py-common and removes multiple duplicate definitions of common functions in the process. This is the next step toward resolving sonic-net/sonic-buildimage#4999.

- Also reorganize imports for consistency
- Also align some style
…1034)

create new feature.py for both config and show, move the code into feature.py
move common code into sonic-utiltities.common.cli.py

Signed-off-by: Guohan Lu <lguohan@gmail.com>
…et#1039)

generate xml, html and term report

Signed-off-by: Guohan Lu <lguohan@gmail.com>
Signed-off-by: Guohan Lu <lguohan@gmail.com>
- add vlan.py in config and show
- add extensive unit tests for vlan

Signed-off-by: Guohan Lu <lguohan@gmail.com>
)

`sonic_installer` has been renamed `sonic-installer`. Update the application name everywhere it is used.
As part of the migration from Python 2 to Python 3, this PR ensures that all Python files in sonic-utilities are Python 3-compliant, and work with both Python 2 and Python 3.

Once this is merged, we can begin building a Python 3-based version of the sonic-utilities package.
…1044)

add extensive test coverage for show interfaces commands.

Signed-off-by: Guohan Lu <lguohan@gmail.com>
To support the `file=` argument in `print()` function in Python 2.7.
…1049)

previously, teamshow needs to read data from teamdctl command
output. As teamdmon daemon has been developed to put data into
state db. Now, teamshow can get data from state db.

Also remove teamshow command and incoporate as part of show
command.

Also add extensive unit tests

Signed-off-by: Guohan Lu <lguohan@gmail.com>
Port breakout-related data should be gathered on-demand, not every time `config` is executed. If the ConfigDB is not ready, the call to get hwsku will hang indefinitely, which will occur when loading config for the first time. Also, if it fails to retrieve the port breakout globals, it will abort, even if the executed command was unrelated to port breakout. This is not desirable behavior.

This PR eliminates port breakout-related global variables, and encapsulates them in functions to be called on-demand, only when commands which require the data are executed. It is currently only accessed in two places. If we feel the need to cache it in the future for efficiency, we can look into adding it to the Click context.

Also rename `_get_option()` to `_get_breakout_cfg_file_name()` to add more detail.
…onic-net#1018)

- What I did
Fixed issue with pfcwd show stats command during SONiC init.
Traceback was returned if DB was not yet initialized which is incorrect. Correct behavior would to rather return empty line if something is not yet ready to be pulled and displayed.

- How I did it
Returned empty dictionary instead of None if data are still not present in DB. It is because in source code data expected to be as dictionary type but if there is no expected attributes in DB None was returned. Wich is incorrect and caused following exception: 'NoneType' object has no attribute 'keys'

Signed-off-by: Volodymyr Samotiy <volodymyrs@mellanox.com>
use VLAN_MEMBER table to get vlan configuration

Signed-off-by: Guohan Lu <lguohan@gmail.com>
…on for all Mellanox switches (sonic-net#993)

* [db_migrator] Support migrate to single ingress buffer pool mode
db_migrator supports migrating old configuration who has 2 ingress pools into the new configuration who has 1 ingress pool, including BUFFER_POOL, BUFFER_PROFILE and BUFFER_PORT_INGRESS_PROFILE_LIST

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* Update according to MSFT comments

1. Don't need to migrate for single buffer pool mode
2. Move buffer setting migration to another file
3. Remove unnecessary code/upgrading flows

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* Fix LGTM warning

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* Fix an error

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* mellanox_db_migrator => mellanox_buffer_migrator

* Fix issue: after migration the lossless profiles are lost

This issue can fail warm reboot because after warm reboot the buffermgr
doesn't have time to generate lossless profiles and the following
orchagent bake can fail due to this

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* Update buffer configuration for version 1.0.4

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* Update the buffer setting for version 1.0.4

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* [mellanox_buffer_migrator] log identifier updated from 'db_migrator' to 'mellanox_buffer_identifier'

Signed-off-by: Stephen Sun <stephens@mellanox.com>

* [db_migrator] Adjust db_migrator according to the latest master change

Signed-off-by: Stephen Sun <stephens@mellanox.com>

Co-authored-by: Stephen Sun <stephens@mellanox.com>
Common changes will be used to support SONiC CLIs for multi ASIC
- New MultiAsic class  to support not displaying of internal object 
- Common CLI options which needed for multi ASIC platforms
- a new decorator to execute a function on all namespaces

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
…net#1036)

* Change fast-reboot script to use swss and radv service script for stopping services
Changes:
-- Display ipv4 address with left adjust of 25 width and with space before UDP.
-- IPv6 address will be displayed as is.
-- Add test data to appl_db.json and config_db.json
-- add test file sflow_test.py
-- use pass_db decorator for sflow_interface.
-- since sflow needs ctx, create use Db() in function.

Signed-off-by: Praveen Chaudhary pchaudhary@linkedin.com
Code coverage requires that python code be run with the same process.
Current test code was invoking filter fdb via shell which launches
new process and so coverage is not available. This PR calls
the main method from within test code.

signed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>
* [config] Reduce Calls to SONiC Cfggen

Calls to sonic-cfggen is CPU expensive. This PR reduces calls to
sonic-cfggen during config-setup when configuring buffer/qos.

singed-off-by: Tamer Ahmed <tamer.ahmed@microsoft.com>

* review comment, apply both buffer and qos configs or none
* Fix the fast-reboot-dump.py error when it try to use inet_aton to translate ipv6 address
* Add send_ndp as TODO in fast-reboot-dump.py to ipv6 target
The import re line was removed from the file in sonic-net#953. However, it is still referenced in the update_sonic_environment() function. Adding it back.
)

Changes to support the show interface status/description for multi ASIC
- Add argparse intfutil script
- Change the IntfDescription and IntfStatus classes to get the information from all namespaces.
- Add changes to filter out the internal ports from display
- Add support for -n and -d click options
- Add unit test to test mulit asic commands

Signed-off-by: Arvindsrinivasan Lakshmi Narasimhan <arlakshm@microsoft.com>
sonic_installer and generate_dump utilities use currently /tmp directory
to store temporary files. Since the amount of data stored can be rather
large (especially in the case of the sonic_installer), we are switching to
use /var/tmp  instead.

We are doing these changes in anticipation that at some point /tmp will being
mounted over TMPFS

/tmp is typically mounted on TMPFS, which means it's fast and does not produce
wear on SSD or Hard-Drive. /var/tmp is mounted on the rootfs partition,
therefore not in the TMPFS partition.

We also noticed that both sonic_installer and generate_dump produce temporary
files which are not deleted once thse programes are done. We are then making
changes to peform a clean-up and remove the temporary files not needed anymore.
@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2020

This pull request introduces 3 alerts and fixes 2 when merging d5ef3ba into 2c0ff92 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Syntax error

fixed alerts:

  • 2 for Unused local variable

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

Successfully merging this pull request may close these issues.