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

refactor(web): bring back zFCP support (new HTTP / JSON API, queries, and TypeScript) #1570

Merged
merged 121 commits into from
Sep 18, 2024

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Sep 3, 2024

Trello: https://trello.com/c/KMtaVltF (internal link)

This PR could be seen as a follow up of https://trello.com/c/2LC8JEZR (internal link) but in this case bringing back the support for managing zFCP devices on s390x architectures.

It includes:

  • Extending the HTTP API to expose zFCP devices management and zFCP related events.
  • Adapting the WEB UI for using Tanstack queries and the new HTTP API.
  • Adopting Typescript
  • Move to route pages.

HTTP / JSON API Examples

This section contains a few examples you can use to explore the API. For the ones using curl, it is required to have a headers.txt file containing the credentials. The file can be generated with:

#!/bin/bash

SERVER=myServer
PASSWORD=myPassword

TOKEN=$(curl --insecure -s -X POST ${SERVER}/api/auth -H 'Content-Type: application/json' -d '{"password": "'${PASSWORD}'"}' | jq -r ".token")
echo "Obtained token '${TOKEN}'"

echo -n "Authorization: Bearer " >headers.txt
echo $TOKEN >>headers.txt
zFCP operations

Probing

$ curl -s -k -X POST ${SERVER}/api/storage/zfcp/probe -H @headers.txt | jq
null

Check if zFCP is supported

$curl -s -k ${SERVER}/api/storage/zfcp/supported -H @headers.txt | jq
true

Check whether allow_lun_scan is globally enabled or not

$curl -s -k ${SERVER}/api/storage/zfcp/config -H @headers.txt | jq
{
  "allowLunScan": false
}

Get Controllers with complete information like WWPNs and LUNs

$ curl -s -k ${SERVER}/api/storage/zfcp/controllers -H @headers.txt | jq
[
  {
    "id": "1",
    "channel": "0.0.fa00",
    "lunScan": false,
    "active": true,
    "lunsMap": {
      "0x500507680d7e284a": [],
      "0x500507680d0e284a": [],
      "0x500507630b181216": [
        "0x4020404900000000"
      ]
    }
  },
  {
    "id": "2",
    "channel": "0.0.fc00",
    "lunScan": false,
    "active": true,
    "lunsMap": {
      "0x500507680d0e284b": [],
      "0x500507630b101216": [
        "0x0000000000000000",
        "0x0001000000000000"
      ],
      "0x500507680d7e284b": []
    }
  }
]

Get Active Disks

$ curl -s -k ${SERVER}/api/storage/zfcp/disks -H @headers.txt | jq
[
  {
    "name": "/dev/sda",
    "channel": "0.0.fa00",
    "wwpn": "0x500507630b181216",
    "lun": "0x4020404900000000"
  },
  {
    "name": "/dev/sdb",
    "channel": "0.0.fc00",
    "wwpn": "0x500507630b101216",
    "lun": "0x0000000000000000"
  }
]

Disk activation

CONTROLLERID=1
WWPN=0x500507630b181216
LUN=0x4020404900000000
curl -s -k -v -X POST ${SERVER}/api/storage/zfcp/controllers/${CONTROLLERID}/wwpns/${WWPN}/luns/${LUN}/activate_disk -H @headers.txt | jq
null

Web UI Screenshots

The web has been adapted according to the recommended components to be used for Pages and content as well the Disk Activation Form has been moved from a Popup to its own route and Page.

Screenshot from 2024-09-17 23-47-40
Screenshot from 2024-09-17 23-45-24
Screenshot from 2024-09-17 23-45-48
Screenshot from 2024-09-17 23-45-58

Additional notes

The PR also includes minor internal changes related to Storage DASD components, some of them related to page actions.

web/src/components/storage/dasd/DASDPage.tsx Outdated Show resolved Hide resolved
web/src/components/storage/dasd/index.js Outdated Show resolved Hide resolved
web/src/components/storage/zfcp/index.js Outdated Show resolved Hide resolved
* Returns a query for retrieving the zFCP controllers
*/
const zfcpControllersQuery = () => ({
const zfcpControllersQuery = {
Copy link
Contributor

@dgdavid dgdavid Sep 17, 2024

Choose a reason for hiding this comment

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

BTW, maybe I miss something at some point but the only suggestion I see is about the naming: #1570 (review)

But not about the full structure. I've no strong opinion about writing them as function or object. But I though we were writing them always as function for consistence since they could receive args for building the query. As far as I can see, @imobachgs has wrote some as function and some as object in the storage part too. We can go ahead with such approach if you guys think it is better, but we should be consistent in all places.

PLEASE, do not take this comment as a suggestion for continue sending changes against this PR. Whatever we decide to choose should be addressed in a change unifying all the queries across the code.

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 was based on this another suggestion #1570 (comment), in that case objects were used, so, I also does not have an strong opinion and agree on unifying.

Copy link
Contributor

@dgdavid dgdavid Sep 17, 2024

Choose a reason for hiding this comment

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

Ok, I see.

As said, let's go ahead and talk about it later, once we finish the queries implementation.

Copy link
Contributor

@imobachgs imobachgs Sep 18, 2024

Choose a reason for hiding this comment

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

This change was based on this another suggestion #1570 (comment), in that case objects were used, so, I also does not have an strong opinion and agree on unifying.

Well, the point of my comment was not the queryKey thingie, but the overall approach of the whole function. I added another note about it (see #1570 (comment)).

rust/agama-lib/src/storage/client/zfcp.rs Outdated Show resolved Hide resolved
let mut devices: Vec<(OwnedObjectPath, ZFCPController)> = vec![];
for (path, ifaces) in managed_objects {
if let Some(properties) = ifaces.get("org.opensuse.Agama.Storage1.ZFCP.Controller") {
// TODO: maybe move it to model? but it needs also additional calls to dbus
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO: maybe move it to model? but it needs also additional calls to dbus
// TODO: maybe move it to model? but it needs also additional calls to D-Bus

/// edge case with malformed path that will panic in Owned path already
/// ```should_panic
/// let path = zbus::zvariant::OwnedObjectPath::try_from("mangled").unwrap();
/// let id = agama_lib::storage::client::zfcp::ZFCPClient::controller_id_from_path(&path);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to this comment, the call to OwnedPath::try_from will fail, right? So what is the point of this code?

/// let id = agama_lib::storage::client::zfcp::ZFCPClient::controller_id_from_path(&path);
/// assert_eq!(id, "mangled".to_string());
/// ```
pub fn controller_id_from_path(path: &OwnedObjectPath) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok(dbus)
}

// TODO: does all those method with controller id as parameter deserve to be moved to controller model?
Copy link
Contributor

Choose a reason for hiding this comment

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

np: Do you plan to do something with this comment? Otherwise, I would remove the TODO prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jreidinger I guess it is a question you wanted to raise and discuss, so, will remove the comment but maybe this thread could be used for having that discussion... by now it looks good to me to leave the definition where it is, @imobachgs, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is fine. I do not like that we use two different approaches, but let's move on.

rust/agama-server/src/storage/web/zfcp.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/storage/client/zfcp.rs Outdated Show resolved Hide resolved
web/src/api/zfcp.ts Outdated Show resolved Hide resolved
web/src/queries/zfcp.ts Outdated Show resolved Hide resolved
web/src/utils/zfcp.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

LGTM. Congrats!

@teclator teclator merged commit 0b66de2 into master Sep 18, 2024
6 checks passed
@teclator teclator deleted the zfcp_adapt branch September 18, 2024 10:50
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Sep 23, 2024
https://build.opensuse.org/request/show/1202590
by user IGonzalezSosa + anag+factory
- Version 10

- Change the license to GPL-2.0-or-later (gh#agama-project/agama#1621).

- Bring back zFCP management support (gh#agama-project/agama#1570).

- Consider TypeScript files for translation
  (gh#agama-project/agama#1604).
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Sep 23, 2024
https://build.opensuse.org/request/show/1202589
by user IGonzalezSosa + anag+factory
- Version 10

- Change the license to GPL-2.0-or-later (gh#agama-project/agama#1621).

- Expose the zFCP D-Bus API through HTTP (gh#agama-project/agama#1570).

- For CLI, use HTTP clients instead of D-Bus clients,
  final piece: Storage (gh#agama-project/agama#1600)
  - added StorageHTTPClient

- Add additional wireless settings (gh#agama-project/agama#1602).
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.

5 participants