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

Adapt storage proposal to the new architecture #1175

Merged
merged 53 commits into from
May 13, 2024

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented May 2, 2024

This PR implements an HTTP/JSON API for managing the storage proposal and all the support methods needed by the corresponding web interface.

This works as a proxy over the real API which is implemented in D-Bus. That comes with some problems (especially regarding performance) that will need to be fixed in the future.

Testing

  • Existing unit tests adapted to the new API, except those related to signals (onDeprecate and onIssuesChange).
  • Tested manually

References

https://trello.com/c/HOkBiVHp/

Copy link

github-actions bot commented May 2, 2024

Coverage Status

coverage: 67.877% (-0.2%) from 68.078%
when pulling ef2ace2 on more_storage_http_api
into 259b625 on architecture_2024.

@jreidinger jreidinger changed the base branch from architecture_2024 to master May 6, 2024 11:49
@ancorgs ancorgs changed the title More storage http api Adapt storage proposal to the new architecture May 10, 2024
@ancorgs ancorgs marked this pull request as ready for review May 10, 2024 14:13
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.

I have finished checking the Rust part. There are a few things that I am pretty sure Clippy will complain, so we can address them in the short term. But by now it is fine.

rust/agama-lib/src/storage/client.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/storage/client.rs Show resolved Hide resolved
Ok(volume_hash.try_into()?)
}

pub async fn product_mount_points(&self) -> Result<Vec<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.

Add to your personal TODO list to document these methods :-) Actually, I expect some refactoring (because we might need an HTTP-based client) so it can be part of that change.

async fn build_drive(&self, object: &DbusObject) -> Result<Option<Drive>, ServiceError> {
let properties = self.get_interface(object, "org.opensuse.Agama.Storage1.Drive");

if let Some(properties) = properties {
Copy link
Contributor

Choose a reason for hiding this comment

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

np: This is perfectly fine, but I would have used a let-else construction. Nothing to change today, anyway.

device.sid = deviceProperties.SID.v;
device.name = deviceProperties.Name.v;
device.description = deviceProperties.Description.v;
const buildDevice = (jsonDevice, jsonDevices) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the impression that we are abusing inline functions. AFAIS, this function does not have any closure and, moreover, it defines many other internal functions. I would add this to our list of things to discuss.

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.

Just a few comments, nothing serious.


async fn build_device(&self, object: &DBusObject) -> Result<Device, ServiceError> {
Ok(Device {
block_device: self.build_block_device(object).await?,
Copy link
Contributor

@joseivanlopez joseivanlopez May 13, 2024

Choose a reason for hiding this comment

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

The conversions (e.g., for ProposalSettings) is done in the model by implementing the TryFrom trait. For the devices, the objects are directly created in the client. Probably it would be better to follow the same approach and implement TryFrom for the needed types in the model.

@joseivanlopez joseivanlopez merged commit 471dd9e into master May 13, 2024
5 checks passed
@joseivanlopez joseivanlopez deleted the more_storage_http_api branch May 13, 2024 08:58
imobachgs added a commit that referenced this pull request May 13, 2024
This PR implements an HTTP/JSON API for managing iSCSI devices. It works
as a proxy over the real API which is implemented in D-Bus.

The API is composed of these parts:

* A bunch of HTTP/JSON endpoints to manage the iSCSI configuration.
* A stream of iSCSI nodes changes (adding, changing or removing nodes).
* A stream of initiator changes (not implemented yet).

NOTE: this PR should be merged after #1175.

## To do

- [x] Adapt the client
- [x] Populate the cache of ISCSI nodes in the stream
- [x] Allow changing the initiator name
- [x] Allow changing the startup
- [x] Emit initiator changes
- [x] OpenAPI documentation
- [x] Remove calls to `unwrap()` in the initiator changes stream

## References

* Trello: https://trello.com/c/TEQvbxAU/
joseivanlopez added a commit that referenced this pull request May 14, 2024
## Problem

The UI is broken in a diskless system. It started failing after
migrating storage to the new architecture (see
#1175).

## Solution

Return correct default values if a HTTP call fails, fix the issues
dialog and fix storage events.

## Testing

* Added new unit tests
* Tested manually

## Screenshots

<details>
<summary>Toggle</summary>


![localhost_8080_](https://github.com/openSUSE/agama/assets/1112304/6d671cd3-9d12-481f-b27d-8ab4afe63e3f)

![localhost_8080_
(1)](https://github.com/openSUSE/agama/assets/1112304/58a3f0b3-3425-4657-aa0b-7a7c938fef40)

![localhost_8080_
(2)](https://github.com/openSUSE/agama/assets/1112304/3d0d9831-c345-462d-86d3-87f2b7a7fd6c)

</details>
@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
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