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

[Service] Alternative location for volumes #1105

Merged
merged 4 commits into from
Mar 21, 2024
Merged

[Service] Alternative location for volumes #1105

merged 4 commits into from
Mar 21, 2024

Conversation

ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Mar 19, 2024

Problem

The storage proposal allows to specify what to do in order to allocate any volume:

  • Use the default location (new partition or new LV)
  • Create it in a given disk as a new partition
  • Create it as a new LV in a new dedicated VG on top of a given disk
  • Reformat an existing device
  • Mount an existing device

Everything is possible, no matter whether the default target is a disk or a new LVM VG.

So far, it was not possible to use the Agama D-Bus settings to specify that to the proposal, so all volumes always used the default location.

Solution

This adds two new attributes to the D-Bus definition of a volume: Target and TargetDevice.

Internally, this is represented by a new class VolumeLocation with two attributes:

  • target: which specifies how to allocate the volume
  • device: the name of the target device, the exact meaning depends on target

The new class and the possible targets follow the same approach of SpaceSettings and its policies.

Testing

  • Added several unit tests
  • Tested manually by hacking the web UI a bit (see screenshots below)

Screenshots

Hacking the web UI to set Target to filesystem and TargetDevice to /dev/vdb5 (which contains a Btrfs file system).

mount-new

Hacking the web UI to set Target to new_vg and TargetDevice to /dev/vdc.

vg-new

@ancorgs ancorgs force-pushed the volume_location branch 3 times, most recently from 5e39176 to 9821f97 Compare March 20, 2024 08:02
@coveralls
Copy link

coveralls commented Mar 20, 2024

Coverage Status

coverage: 74.41% (+0.06%) from 74.346%
when pulling 5270b4b on volume_location
into 2c81aca on master.

@ancorgs ancorgs changed the title WIP: RFC [Service] Alternative location for volumes Mar 20, 2024
@ancorgs ancorgs force-pushed the volume_location branch 2 times, most recently from cde6b9b to 6c7673e Compare March 20, 2024 13:34
@ancorgs ancorgs marked this pull request as ready for review March 20, 2024 14:56
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

Looks good, but I miss a changelog entry. The only bad thing is that I will eat a lot of conflicts with #1068.


# What to do to allocate the volume
#
# @return [Symbol] see .targets
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it needs brackets, see {.targets}

TargetDevice s
TargetVG s
Target s
TargetDevice s (only makes sense if Target is not default)
Copy link
Contributor

Choose a reason for hiding this comment

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

The equivalent for the default device are called Target and Device. I am not sure if we should follow the same naming or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the current names because it shows the relationship between both fields. I mean, TargetDevice is actually some kind of "details" field for the Target one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense. Then, I will adapt my PR to follow the same approach.

Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@ancorgs ancorgs merged commit 3680ba4 into master Mar 21, 2024
6 checks passed
@ancorgs ancorgs deleted the volume_location branch March 21, 2024 11:21
@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.

3 participants