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

feat(rust): add an HTTP/JSON API to DASD operations #1532

Merged
merged 15 commits into from
Aug 27, 2024
Merged

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Aug 11, 2024

Trello: https://trello.com/c/2LC8JEZR/3743-8-agama-adapt-dasd-configuration-to-current-architecture

This PR is the first step to bring back support for DASD devices on s390x architectures. It includes:

  • Extending the HTTP API to expose DASD devices and formatting jobs.
  • Adding DASD-related events (adding, updating and changing DASD devices and format progress).

After some discussion, we decided to retain the original intention of the D-Bus API at HTTP level.

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 (similar to the following example, but replacing TOKEN with your actual token):

Content-Type: application/json
Authorization: Bearer TOKEN
DASD operations

Remember that the -k disables the SSL certificate checking.

Probing for DASD devices

curl -k -H @headers.txt -X POST https://$AGAMA_SERVER/api/storage/dasd/probe

Listing DASD devices

$ curl -k --silent -H @headers.txt -X GET https://$AGAMA_SERVER/api/storage/dasd/devices | jq
[
  {
    "id": "0.0.0160",
    "enabled": true,
    "device_name": "dasda",
    "formatted": true,
    "diag": false,
    "status": "active",
    "device_type": "ECKD",
    "access_type": "rw",
    "partition_info": ""
  }
]

Enabling DASD devices

[!NOTE]
We might consider using PATCH or even POST for enabling, disabling, setting diag and formatting the device.

$ curl -k -H @headers.txt -X POST https://$AGAMA_SERVER/api/storage/dasd/enable \
  -d '{"devices": ["0.0.0160"]}'

Disabling devices

$ curl -k -H @headers.txt -X POST https://$AGAMA_SERVER/api/storage/dasd/disable \
  -d '{"devices": ["0.0.0160"]}'

Setting the DIAG attribute

$ curl -k -H @headers.txt -X PUT https://$AGAMA_SERVER/api/storage/dasd/diag \
  -d '{"devices": ["0.0.0160"], "diag": true}'

Formatting devices

$ curl -k -H @headers.txt -X PUT https://$AGAMA_SERVER/api/storage/dasd/format \
  -d '{"devices": ["0.0.0160"]}'
Jobs operation

Listing jobs

$ curl -k -H @headers.txt -X GET https://$AGAMA_SERVER/api/storage/jobs | jq
[
  {
    "id": "/org/opensuse/Agama/Storage1/jobs/3",
    "running": false,
    "exit_code": 0
  },
  {
    "id": "/org/opensuse/Agama/Storage1/jobs/2",
    "running": false,
    "exit_code": 0
  }
]
Events

The PR adds the following events:

  • DASDDeviceAdded, DASDDeviceChaned and DASDDeviceRemoved.
  • JobAdded, JobChanged and JobRemoved.
  • DASDFormatProgress.

[!WARNING]
The Finished signal of the org.opensuse.Agama.Storage1.Job is not handled.

Let's see a few examples:

Formatting progress

{
  "type": "DASDFormatJobChanged",
  "job_id": "/org/opensuse/Agama/Storage1/jobs/9",
  "total": 30050,
  "step": 30050,
  "done": true
}

DASD device detected

{
  "type": "DASDDeviceAdded",
  "device": {
    "id": "0.0.0160",
    "enabled": true,
    "device_name": "dasda",
    "formatted": false,
    "diag": false,
    "status": "no_format",
    "device_type": "ECKD",
    "access_type": "rw",
    "partition_info": ""
  }
}

DASD device changes

{
  "type": "DASDDeviceChanged",
  "device": {
    "id": "0.0.0160",
    "enabled": true,
    "device_name": "dasda",
    "formatted": false,
    "diag": false,
    "status": "no_format",
    "device_type": "ECKD",
    "access_type": "rw",
    "partition_info": ""
  }
}

A notes about signals

We have discussed whether we are abusing standard D-Bus signals a bit. For instance, we rely heavily on PropertiesChanged, InterfacesAdded, InterfacesRemoved, etc., even for meaningful and important system events. Then, the client code needs to figure out what those signals actually mean. Also, unless you use a proxy (and that's not always possible), you cannot rely on the type checking that the introspection offers.

Of course, that's out of the scope of this pull request.

Code repetition

As you might notice, there is quite some repetitive code. At this point, I just wanted to have some working code. We can reduce the repetition later. Improving our signals would help reduce the amount of code on the client side.

@coveralls
Copy link

coveralls commented Aug 11, 2024

Coverage Status

coverage: 70.597% (-0.7%) from 71.319%
when pulling a226fec on http-dasd
into 55fabd7 on master.


/// Represents a DASD device (specific to s390x systems).
#[derive(Clone, Debug, Serialize, Default, utoipa::ToSchema)]
pub struct DASDDevice {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the fields in this struct (status, device_type and/or access_type) might deserve an enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw resulting json is not consistent with others. I think agreemend is snakeCase...so let me change it to #[serde(rename_all = "camelCase")]

Ok(devices)
}

pub async fn format(&self, ids: &[&str]) -> Result<String, ServiceError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

API looks strange, it allows formating multiple DASDs but it return only one path for job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

there is only one job, that is ok, but then we need to check the status for each device, I think the missing part is the job Summary call on formatProgress callback.

https://github.com/openSUSE/agama/blob/master/web/src/client/storage.js#L891

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -17,7 +17,7 @@ use tokio::sync::broadcast::{Receiver, Sender};
use super::common::Issue;

#[derive(Clone, Debug, Serialize)]
#[serde(tag = "type")]
#[serde(rename_all = "camelCase", tag = "type")]
Copy link
Contributor

Choose a reason for hiding this comment

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

good, I recheck that it affects only dasd format event. So it is safe.

@imobachgs imobachgs marked this pull request as ready for review August 27, 2024 10:02
@imobachgs imobachgs merged commit 005f61f into master Aug 27, 2024
2 checks passed
@imobachgs imobachgs deleted the http-dasd branch August 27, 2024 10:17
imobachgs added a commit that referenced this pull request Aug 28, 2024
* Revert a226fec as it affects the event names (`progress` vs
`Progress`).
* Add the changelog for #1532.
teclator added a commit that referenced this pull request Sep 5, 2024
Bring back the DASD management support adapting the web UI using the new HTTP/JSON API
 introduced by #1532

---------

Co-authored-by: Knut Anderssen <[email protected]>
Co-authored-by: Imobach González Sosa <[email protected]>
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
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.

4 participants