Skip to content

Use controlled execution environment, to avoid failure if PATH is unset (LP: #1959570)#336

Merged
slyon merged 3 commits into
canonical:mainfrom
slyon:no-path-env
Apr 3, 2023
Merged

Use controlled execution environment, to avoid failure if PATH is unset (LP: #1959570)#336
slyon merged 3 commits into
canonical:mainfrom
slyon:no-path-env

Conversation

@slyon

@slyon slyon commented Mar 23, 2023

Copy link
Copy Markdown
Contributor

Description

Use a controlled execution environment.
This is to allow execution in constrained environments, like early boot or
Ubuntu Core 20/22 where the PATH environment variable might not be set

Also, handle nmcli's "NetworkManager not running" error gracefully.
Considering /usr/bin/nmcli for normal NetworkManager and /snap/bin/nmcli -> /snap/bin/network-manager.nmcli for NetworkManager snap as valid implementations.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad. LP#1959570

@slyon

slyon commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

This fix seems to be working generally. There's still a brief interruption of the network connectivity, while restarting the "snap.network-manager.networkmanager.service" daemon (obviously), but the SSH connection is recovered after a few seconds.

I've built a core22 base snap, using https://launchpad.net/~slyon/+archive/ubuntu/lp1959570 and a UC22 image out of this to run this test (all executed over an SSH connection):

slyon@ubuntu:~$ sudo snap set system system.network.netplan.network.bridges.br54.dhcp4=true
slyon@ubuntu:~$ sudo netplan get
network:
  version: 2
  renderer: NetworkManager
  ethernets:
    enp0s2:
      dhcp4: true
  bridges:
    br54:
      dhcp4: true
slyon@ubuntu:~$ sudo snap set system system.network.netplan.network.bridges.br54.dhcp4=false
slyon@ubuntu:~$ sudo netplan get
network:
  version: 2
  renderer: NetworkManager
  ethernets:
    enp0s2:
      dhcp4: true
  bridges:
    br54:
      dhcp4: false

@slyon slyon requested review from daniloegea and ogayot March 23, 2023 13:53

@daniloegea daniloegea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be enough settings the environment variables only once via os.environ in the entry point of the python code?

Comment thread netplan/cli/utils.py Outdated
Comment thread Makefile
Comment thread netplan/cli/commands/status.py
Comment thread netplan/cli/commands/apply.py Outdated
Comment thread netplan/cli/commands/apply.py Outdated
slyon added 3 commits April 3, 2023 14:27
This is to allow execution in constrained environments, like early boot or
Ubuntu Core 20/22 where the PATH environment variable might not be set.

E.g. for the 'nmcli' binary:
It could be /usr/bin/nmcli for normal NetworkManager or
/snap/bin/nmcli -> /snap/bin/network-manager.nmcli for NM snap
Also, consider /usr/bin/nmcli and
/snap/bin/nmcli -> /snap/bin/network-manager.nmcli as valid implementations
of 'nmcli', so it can be exected in a minimal environment, without the PATH
variable being set (e.g. early boot or Ubuntu Core 20/22).

@ogayot ogayot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@daniloegea daniloegea left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@slyon

slyon commented Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

All tests green.

I've re-built a package in the above-mentioned PPA + core22 base snap + UC22 image and run the reproducer from the bug report:
=> It's still working as expected with the new os.environ.update() approach.

Merging.

@slyon slyon merged commit fc12872 into canonical:main Apr 3, 2023
daniloegea pushed a commit that referenced this pull request May 17, 2023
…et (LP: #1959570) (#336)

Use a controlled execution environment.
This is to allow execution in constrained environments, like early boot or
Ubuntu Core 20/22 where the PATH environment variable might not be set

Also, handle nmcli's "NetworkManager not running" error gracefully.
Considering /usr/bin/nmcli for normal NetworkManager and /snap/bin/nmcli -> /snap/bin/network-manager.nmcli for NetworkManager snap as valid implementations.

COMMITS:
* cli: use controlled execution environment

This is to allow execution in constrained environments, like early boot or
Ubuntu Core 20/22 where the PATH environment variable might not be set.

E.g. for the 'nmcli' binary:
It could be /usr/bin/nmcli for normal NetworkManager or
/snap/bin/nmcli -> /snap/bin/network-manager.nmcli for NM snap

* cli:status:ip: Introduce utils.nmcli_out()

* cli:apply: Handle 'nmcli' NM not running gracefully

Also, consider /usr/bin/nmcli and
/snap/bin/nmcli -> /snap/bin/network-manager.nmcli as valid implementations
of 'nmcli', so it can be exected in a minimal environment, without the PATH
variable being set (e.g. early boot or Ubuntu Core 20/22).
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.

3 participants