[Malar2703]

PR Info:

Exposed an API to assign an IP address to the interface created by the VM when the PCIF is attached.

This PR accepts network information and the PCIF ID as input. The PCIF ID is used to identify the corresponding network interface name (e.g., ens327) configured on the VM. The IP address is then assigned to the identified interface by creating the appropriate ifcfg-en* configuration file in the OS-specific /etc directory. Finally, the VM must be restarted to apply the updated network configuration.

API Info:

POST /guests/{userid}/interface/ip

Configure the IP address and network settings for a Network Express (NETH)
interface on a running guest. The guest must be powered on and reachable
through IUCV, and the NETH PCI function (pcif) must already be attached
to the guest before this call is made.

The operation writes the network configuration file into the guest via the
SMT layer, then reboots the guest so that the new settings take effect.

Supported distributions: rhel6.x, rhel7.x, rhel8.x,
rhel9.x, sles11.x, sles12.x, sles15.x, ubuntu16.x,
ubuntu20.x, ubuntu22.x, ubuntu24.x and ubuntu25.x
(all case-insensitive). rhcos4.x is not supported by this API.

Request Body:

{
  "interface": {
    "os_version": "rhel9.2",
    "guest_network": {
      "method": "static",
      "ip_addr": "192.168.1.10",
      "cidr": "192.168.1.0/24",
      "gateway_addr": "192.168.1.1",
      "dns_addr": ["9.0.2.1", "9.0.3.1"]
    },
    "pcif": "00000110",
    "active": true
  }
}

[Bischoff]

Eric

Thanks a lot for this PR!

For the next ones, would it be possible to have a PR description? I only had the PR's title, and for the rest I had to guess your intentions.

I also notice no unit tests, no documentation.

Claude, prompted by Eric

I reviewed the current head of the branch (cfbfdb6). Configuring the IP address of a Network Express interface is a useful addition. However, I found a regression and several bugs, so I think the PR needs more work.

tox -e pep8 passes, and the existing unit tests show no new failure, but only because none of them covers the changed paths.

Bugs

1. Regression on FCP

In smtLayer/getVM.py, posOpsList = {...}, ends with a comma, so it becomes a tuple instead of a dict.

Positional operands of all GETVM subfunctions are then ignored, including the existing FCPINFO used by SMTClient.get_fcp_info_by_status(): parsing getvm TESTVM fcpinfo free gives parms = {} instead of {'status': 'free'}.

No unit test covers this path, which explains the issue went undetected.

2. Configuration files removed too broadly

NetworkOPS._create_neth_configuration_files() generates rm -f /etc/sysconfig/network-scripts/ifcfg-{intf_name[:2]}*. For enc1600, this
removes every ifcfg-en* file, including the configuration of the guest's regular NIC.

3. DHCP and optional fields

_create_neth_configuration_files() reads net['cidr'], net['gateway_addr'] and net['ip_addr'] unconditionally, so method dhcp (accepted by the validation) or a network without gateway raises KeyError or concatenates None.

4. Only one network

The loop overwrites cfg_str, so only the last entry of guest_networks is written.

5. pcif parameter

The REST schema does not list pcif as required, but VMHandler.configure_network_interface_ip() reads interface['pcif'], which gives a 500 error when it is missing.

6. PCIF matching

getVM.getNethIntInfo() looks for pcif[-4:] as a substring of the PCI address, which can match the wrong device. Comparing the domain field exactly would be safer.

Design questions

1. The generated file is /etc/sysconfig/network-scripts/ifcfg-*, which only works for RHEL up to 8, not for RHEL 9 and later (NetworkManager), SLES (/etc/sysconfig/network/) or Ubuntu (netplan), while os_version is required but not used.

Which distributions are meant to be supported?

2. zdirect_network_configuration() always reboots the guest, even with active=True. Is this intended? It should at least be documented.

3. The input validation in SDKAPI.guest_configure_network_interface_ip() is a copy/paste of guest_create_network_interface(), including the error messages that still mention guest_create_network_interface. I suggest adapting it. Optionally, factor it into a common helper.

Tests and documentation

1. There are no unit tests for the new API, the network configuration generation and getNethIntInfo().

2. Documentation: nothing in doc/source/restapi.rst, parameters.yaml or zvmsdk/tests/fvt/api_templates/ for POST /guests/{userid}/interface/ip. It would be nice to also document the prerequisites (guest running and reachable through IUCV, PCIF attached), the supported distributions, and the reboot.

3. PR description

That would help reviewers too.

[Rajat-0]

Review of #970 (head 6f1d7a6)

Review body

Thanks for adding NETH IP configuration. The main blockers are below; details are inline.

1. Because of an operator-precedence bug, the image clean command runs on every call, and it deletes the guest's existing NIC configs (networkops.py:384).
2. The REST client entry declares 3 required args, but the API takes 4, so the client can't call it (restclient.py:962).
3. The RHCOS branch fails silently, and when it does work it never reaches the guest (networkops.py:320).
4. The interface lookup assumes the lspci PCI domain equals the last 4 hex digits of the FID. That likely doesn't hold on s390; please confirm on hardware (getVM.py:751).

Smaller points:

• get_neth_cfg is copy-pasted three times (RHEL, SLES, Ubuntu). Each copy re-imports netaddr as _netaddr inline (dist.py:412), although netaddr is already imported at module level, and the copies have already drifted apart. For example, RHEL writes IPADDR/GATEWAY even for DHCP, while SLES and Ubuntu don't. A base-class helper for the parsing, with only the output format in each subclass, would avoid this.

---

Generated by Claude Code

Inline comments

zvmsdk/networkops.py:384 — clean_cmd = linuxdist._get_clean_command() or '' if first else ''

This parses as _get_clean_command() or ('' if first else ''), so the clean command runs on every call, not just the first. It is also wrong on the first call: it removes the guest's existing configs before the new file is written. On Ubuntu 20/22 that's rm -f /etc/netplan/*.yaml, which deletes the primary 00-zvmguestconfigure-config.yaml. On RHEL 8/9 it's rm -f .../ifcfg-enc*. After the forced reboot, the guest comes back with only the NETH interface. Suggest only removing the file for this interface, and adding parentheses if a conditional is still needed.

zvmconnector/restclient.py:962 — 'args_required': 3,

The API takes 4 required positional args (userid, os_version, guest_network, pcif), and req_guest_configure_network_interface_ip reads args[start_index + 2]. With 4 positional args the client raises ArgsFormatError: Too many args; with 3 it gets past the check but fails with IndexError. This should be 'args_required': 4, with a unit test that calls it positionally.

zvmsdk/networkops.py:320 — if self._smtclient.is_rhcos(os_version):

This branch can't work as written:

• create_coreos_parameter reads vif['mtu'], vif['gateway_addr'] and vif['ip_addr'] without defaults, and the REST schema (additionalProperties: False) doesn't allow mtu. Every RHCOS request therefore raises KeyError. create_coreos_parameter_temp_file swallows it and returns False, the return value is never checked, and the API returns 200.
• Even when it succeeds, it only writes a host-side fixed_ip_param temp file, which is used at deploy time. Nothing reaches the running guest, there's no reboot, and active is ignored.

Suggest raising an explicit "not supported for RHCOS" error until there is a real path.

smtLayer/getVM.py:751 — pcif_id_lower = pcif[-4:].lower().zfill(4)

This assumes the Linux PCI domain in lspci equals the last 4 hex digits of the FID. On s390 the domain is the UID (when UID checking is enabled) or a sequential index, not the FID. FID 00000110 with UID 0001 shows up as 0001:00:00.0, so this never matches and returns rc 4 / rs 99. Worse, if an unrelated device happens to have domain 0110, its interface gets this IP. Matching on /sys/bus/pci/devices/*/function_id instead would avoid this. Has this been tested on real hardware?

zvmsdk/dist.py:1260 — cfg_str += 'mtu ' + mtu + '\n'

The REST schema doesn't allow mtu, so mtu is normally '' here, and the Ubuntu 16 file gets a bare mtu line. ifupdown rejects the empty option, so the interface fails to come up after the reboot. Only write the line when mtu is set. The same pattern is at line 1277.

zvmsdk/dist.py:1627 — 'gateway4': gateway_v4,

Several problems in the netplan config:

• A static network with no gateway_addr produces gateway4: '', which netplan generate rejects, so the interface stays down.
• mtu is written as a string.
• Without cidr, the address becomes '192.168.1.10/'. The API doesn't enforce that ip_addr comes with cidr.
• method isn't lowercased here, so 'DHCP' is treated as static.

zvmsdk/dist.py:427 — cfg_str += 'NETTYPE=\"NETH\"\n'

The RHEL get_neth_cfg (and the SLES one) never write dns_addr. The existing RHEL _get_cfg_str writes DNS1/DNS2, but the new NETH writers don't. A request with "dns_addr": ["9.0.2.1"] returns 200 and echoes the DNS list back, but the guest's resolver isn't configured.

zvmsdk/tests/fvt/api_templates/test_guest_configure_network_interface_ip.tpl:14 — "active": True

True is a Python literal, not JSON. It should be true. This template is also included in restapi.rst as the documented request sample, so anyone who copies it sends a body that util.extract_json can't parse.

zvmsdk/dist.py:664 — nic_default_name: Any = ...

This variable annotation, the new typing import, and the new f-strings in dist.py and getVM.py need Python 3.6+, but setup.py still lists 2.7 and 3.5. On those versions, importing zvmsdk.dist fails with SyntaxError, which breaks the whole SDK. The Any annotation has no effect anyway. Either drop it and use %/.format(), or update the supported versions in setup.py in a separate change.

[Malar2703]

@Rajat-0

Thank you for your comments

Updated all the review comments. Kindly check it

[Rajat-0]

Open

1.	The first call still deletes the guest's other network configs. On a guest whose DB row has net_set=0, _create_neth_configuration_files runs _get_clean_command():
•	RHEL 8/9 run rm -f /etc/sysconfig/network-scripts/ifcfg-enc*.
•	Ubuntu 20+ run rm -f /etc/netplan/*.yaml, which also removes 50-cloud-init.yaml and 00-zvmguestconfigure-config.yaml.

A guest whose primary NIC was set up by cloud-init or the image, not by guest_create_network_interface, has net_set=0. After the forced reboot it comes back with only the NETH interface. This API writes one file per interface, so it doesn't need the clean at all. Suggest dropping it, and with it the update_guestdb_with_net_set side effect.
2. DNS is still ignored on SLES. The SLES writer puts DNS1=/DNS2= into ifcfg-, but wicked ignores those keys. The existing SLES code sets DNS through NETCONFIG_DNS_STATIC_SERVERS in /etc/sysconfig/network/config (around dist.py:1129). The request returns 200 and echoes the DNS list, but name resolution on the guest is never configured.
3. Minor:
• method: static with no ip_addr passes both validation layers and writes IPADDR=""/NETMASK="" (or addresses: []). Make ip_addr required for static.
• The netplan file is named ifcfg-.yaml; something like 60-neth-.yaml would be clearer and gives a predictable order.
• The getVM tests mock IUCV, so the sysfs lookup script has only been checked for its text. Ask the author to confirm it once on real hardware with a NETH PCIF attached; the earlier question about hardware testing wasn't answered.

[Malar2703]

@Rajat-0
Fixed the review comments:

Fixed other review comments

1. Clean the file if already file exists with the same name. Not removing other files.

2. The interface lookup assumes the lspci PCI domain equals the last 4 hex digits of the FID. That likely doesn't hold on s390; please confirm on hardware - Verified on the device