Dasd unattended #2302
Dasd unattended #2302
Conversation
imobachgs
left a comment
imobachgs
left a comment
There was a problem hiding this comment.
In general, it looks good. However, unless I am missing something, the OpenAPI does not include the new API.
| #[error("Unsupported SSL Fingerprint algorithm '#{0}'.")] | ||
| UnsupportedSSLFingerprintAlgorithm(String), | ||
| #[error("DASD with channel '#{0}' not found.")] | ||
| DASDChannelNotFound(String), |
There was a problem hiding this comment.
We should avoid adding more variants (especially when they are specific ones) to this already big enum. I improved things for HTTP clients (#2292) but D-Bus clients are still pending.
There was a problem hiding this comment.
yeap, I thought the same. So I am looking for your adaptation. I can do it specially for DASD client, but it would feel a bit inconsistent to have just single one.
| mod settings; | ||
| mod store; | ||
| pub mod settings; | ||
| pub(crate) mod store; |
| pub async fn set_config(&self, config: DASDConfig) -> Result<(), ServiceError> { | ||
| // at first probe to ensure we work on real system info | ||
| self.probe().await?; | ||
| self.config_activate(&config).await?; |
There was a problem hiding this comment.
np: Three different calls with the same argument (the whole configuration)... looks a bit weird to me.
There was a problem hiding this comment.
well, I just want logically split steps. It needs whole configuration as it is list of configuration for devices and each step apply part of configuration. But splitting config to that parts will be non-trivial code.
| Ok(()) | ||
| } | ||
|
|
||
| async fn config_activate(&self, config: &DASDConfig) -> Result<(), ServiceError> { |
There was a problem hiding this comment.
A description, although it is not a public function, would help. The same for the rest.
| .iter() | ||
| .filter(|(system, _config)| system.enabled == false) | ||
| .filter(|(_system, config)| { | ||
| config.state.clone().unwrap_or_default() == DASDDeviceState::Active |
There was a problem hiding this comment.
If you derive Copy for DASDDeviceState, you can omit the clone.
| .collect(); | ||
| self.enable(&to_activate).await?; | ||
|
|
||
| let pairs = self.config_pairs(config).await?; |
There was a problem hiding this comment.
Any reason to read the devices again? I wonder whether it was possible to use a single call to config_pairs.
There was a problem hiding this comment.
Ah, I planned to have config_pair produce iterator instead of vector, so in that case it will be consumed. But maybe it will be more efficient to just use original vector.
| "/org/opensuse/Agama/Storage1/jobs", | ||
| ) | ||
| .await?; | ||
| while !finished { |
There was a problem hiding this comment.
I guess this one works just fine, but I wonder whether it could be possible to rely on signals (I do not know if jobs emit signals).
There was a problem hiding this comment.
yeap, that is what I mention on daily. I can use that stream and format produce a lot of Property change signals, but it is quite some code for it and I found this solution easier to use. The usage of jobs signals is at https://github.com/agama-project/agama/blob/master/rust/agama-server/src/storage/web/dasd/stream.rs#L190 but its usage is non trivial. And be aware that this is dbus client, not http one, so we need to observe dbus properties and not http events.
| devices_map | ||
| .get(c.channel.as_str()) | ||
| .ok_or(ServiceError::DASDChannelNotFound(c.channel.clone()))? | ||
| .clone(), |
There was a problem hiding this comment.
Is it possible that you find the same channel in more than one device? If it is possible, please, ignore the rest of this message :-)
If it is not possible, you can avoid that clone by using remove instead of get (and making the HashMap mutable, of course).
| } | ||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] |
jreidinger
force-pushed
the
dasd_unattended
branch
from
ad7524a to
1cc1abf
Compare
jreidinger
force-pushed
the
dasd_unattended
branch
from
1cc1abf to
3de3235
Compare
Prepare to release Agama 15: * #2258 * #2270 * #2277 * #2279 * #2283 * #2284 * #2285 * #2286 * #2287 * #2288 * #2291 * #2292 * #2293 * #2295 * #2297 * #2299 * #2300 * #2301 * #2302 * #2303 * #2305 * #2306 * #2307 * #2308 * #2309 * #2313 * #2314 * #2315 * #2317 * #2318 * #2319 * #2320 * #2321 * #2322 * #2323 * #2324 * #2325 * #2328 * #2329 * #2330 * #2331 * #2335 * #2336 * #2337 * #2338 * #2339 * #2340 * #2342 * #2345 * #2346 * #2348 * #2349 * #2350 * #2351 * #2352 * #2353 * #2354 * #2355 * #2357 * #2358 * #2359 * #2360 * #2361 * #2362 * #2363 * #2364 * #2365 * #2366 * #2368 * #2369 * #2370 * #2371 * #2372 * #2374 * #2377 * #2378 * #2379 * #2380 * #2381 * #2382 * #2384 * #2385 * #2386 * #2388 * #2389 * #2390 * #2391 * #2392 * #2394 * #2397 * #2398 * #2401 * #2403
Problem
DASD can be setup only interactive. There is no support in agama profile.
trello: https://trello.com/c/cV10En3b/4658-5-unattended-support-for-dasd
Solution
Implement support for applying configuration in agama profile for DASD. Note that it is now missing support for exporting it as we plan to take consistent approach with iSCSI and it is still not yet decided how it will work in the end.
Testing
Documentation
Remember to look at it from the user's perspective. Yes you have made the compiler happy.
But will the humans even know about your contribution? Sometimes they cannot miss it,
other times they need advertisement and explanation.
Look for relevant sections and adjust:
git ls-files '*.md'