-
Notifications
You must be signed in to change notification settings - Fork 69
Dasd unattended #2302
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
Dasd unattended #2302
Changes from 12 commits
1a9bacf
b936d32
3d1a871
b39a7b5
1f46970
d0eab79
94bca88
eec6312
ba2736a
006cf92
d590619
6e71ca0
a6c6203
1c663a9
3de3235
22544a7
5cf985a
210e47f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| { | ||
| "dasd": { | ||
| "devices": [ | ||
| { | ||
| "channel": "0.0.0200", | ||
| "format": true, | ||
| "diag": true, | ||
| "state": "active" | ||
| }, | ||
| { | ||
| "channel": "0.0.0201", | ||
| "state": "offline" | ||
| } | ||
| ] | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,8 +24,8 @@ pub mod client; | |
| pub mod http_client; | ||
| pub mod model; | ||
| pub mod proxies; | ||
| mod settings; | ||
| mod store; | ||
| pub mod settings; | ||
| pub(crate) mod store; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
|
||
| pub use client::{ | ||
| iscsi::{ISCSIAuth, ISCSIClient, ISCSIInitiator, ISCSINode}, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,9 @@ | |
|
|
||
| //! Implements a client to access Agama's D-Bus API related to DASD management. | ||
|
|
||
| use std::{collections::HashMap, time::Duration}; | ||
|
|
||
| use tokio::time::sleep; | ||
| use zbus::{ | ||
| fdo::{IntrospectableProxy, ObjectManagerProxy}, | ||
| zvariant::{ObjectPath, OwnedObjectPath}, | ||
|
|
@@ -28,7 +31,12 @@ use zbus::{ | |
|
|
||
| use crate::{ | ||
| error::ServiceError, | ||
| storage::{model::dasd::DASDDevice, proxies::dasd::ManagerProxy}, | ||
| jobs::client::JobsClient, | ||
| storage::{ | ||
| model::dasd::DASDDevice, | ||
| proxies::dasd::ManagerProxy, | ||
| settings::dasd::{DASDConfig, DASDDeviceConfig, DASDDeviceState}, | ||
| }, | ||
| }; | ||
|
|
||
| /// Client to connect to Agama's D-Bus API for DASD management. | ||
|
|
@@ -37,6 +45,7 @@ pub struct DASDClient<'a> { | |
| manager_proxy: ManagerProxy<'a>, | ||
| object_manager_proxy: ObjectManagerProxy<'a>, | ||
| introspectable_proxy: IntrospectableProxy<'a>, | ||
| connection: Connection, | ||
| } | ||
|
|
||
| impl<'a> DASDClient<'a> { | ||
|
|
@@ -57,6 +66,7 @@ impl<'a> DASDClient<'a> { | |
| manager_proxy, | ||
| object_manager_proxy, | ||
| introspectable_proxy, | ||
| connection, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -66,6 +76,149 @@ impl<'a> DASDClient<'a> { | |
| Ok(introspect.contains("org.opensuse.Agama.Storage1.DASD.Manager")) | ||
| } | ||
|
|
||
| pub async fn get_config(&self) -> Result<DASDConfig, ServiceError> { | ||
| // TODO: implement | ||
| Ok(DASDConfig::default()) | ||
| } | ||
|
|
||
| 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?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. np: Three different calls with the same argument (the whole configuration)... looks a bit weird to me.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| self.config_format(&config).await?; | ||
| self.config_set_diag(&config).await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn config_activate(&self, config: &DASDConfig) -> Result<(), ServiceError> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A description, although it is not a public function, would help. The same for the rest. |
||
| let pairs = self.config_pairs(config).await?; | ||
| let to_activate: Vec<&str> = pairs | ||
| .iter() | ||
| .filter(|(system, _config)| system.enabled == false) | ||
| .filter(|(_system, config)| { | ||
| config.state.clone().unwrap_or_default() == DASDDeviceState::Active | ||
|
||
| }) | ||
| .map(|(system, _config)| system.id.as_str()) | ||
| .collect(); | ||
| self.enable(&to_activate).await?; | ||
|
|
||
| let pairs = self.config_pairs(config).await?; | ||
|
||
| let to_deactivate: Vec<&str> = pairs | ||
| .iter() | ||
| .filter(|(system, _config)| system.enabled == true) | ||
| .filter(|(_system, config)| config.state == Some(DASDDeviceState::Offline)) | ||
| .map(|(system, _config)| system.id.as_str()) | ||
| .collect(); | ||
| self.disable(&to_deactivate).await?; | ||
|
|
||
| if !to_activate.is_empty() || !to_deactivate.is_empty() { | ||
| // reprobe after calling enable. TODO: check if it is needed or callbacks take into action and update it automatically | ||
| self.probe().await?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn config_format(&self, config: &DASDConfig) -> Result<(), ServiceError> { | ||
| let pairs = self.config_pairs(config).await?; | ||
| let to_format: Vec<&str> = pairs | ||
| .iter() | ||
| .filter(|(system, config)| { | ||
| if config.format == Some(true) { | ||
| true | ||
| } else if config.format == None { | ||
| !system.formatted | ||
| } else { | ||
| false | ||
| } | ||
| }) | ||
| .map(|(system, _config)| system.id.as_str()) | ||
| .collect(); | ||
|
|
||
| if !to_format.is_empty() { | ||
| let job_path = self.format(&to_format).await?; | ||
| self.wait_for_format(job_path).await?; | ||
| // reprobe after calling format. TODO: check if it is needed or callbacks take into action and update it automatically | ||
| // also do we need to wait here for finish of format progress? | ||
| self.probe().await?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn wait_for_format(&self, job_path: String) -> Result<(), ServiceError> { | ||
| let mut finished = false; | ||
| let jobs_client = JobsClient::new( | ||
| self.connection.clone(), | ||
| "org.opensuse.Agama.Storage1", | ||
| "/org/opensuse/Agama/Storage1/jobs", | ||
| ) | ||
| .await?; | ||
| while !finished { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // active polling with 1 sec sleep for jobs | ||
| sleep(Duration::from_secs(1)).await; | ||
| let jobs = jobs_client.jobs().await?; | ||
| let job_pair = jobs | ||
| .iter() | ||
| .find(|(path, _job)| path.to_string() == job_path); | ||
| if let Some((_, job)) = job_pair { | ||
| finished = !job.running; | ||
| } else { | ||
| // job does not exist, so probably finished | ||
| finished = true; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn config_set_diag(&self, config: &DASDConfig) -> Result<(), ServiceError> { | ||
| let pairs = self.config_pairs(config).await?; | ||
| let to_enable: Vec<&str> = pairs | ||
| .iter() | ||
| .filter(|(_system, config)| config.diag == Some(true)) | ||
| .map(|(system, _config)| system.id.as_str()) | ||
| .collect(); | ||
| self.set_diag(&to_enable, true).await?; | ||
|
|
||
| let to_disable: Vec<&str> = pairs | ||
| .iter() | ||
| .filter(|(_system, config)| config.diag == Some(false)) | ||
| .map(|(system, _config)| system.id.as_str()) | ||
| .collect(); | ||
| self.set_diag(&to_disable, false).await?; | ||
|
|
||
| if !to_enable.is_empty() || !to_disable.is_empty() { | ||
| // reprobe after calling format. TODO: check if it is needed or callbacks take into action and update it automatically | ||
| // also do we need to wait here for finish of format progress? | ||
| self.probe().await?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn config_pairs( | ||
| &self, | ||
| config: &DASDConfig, | ||
| ) -> Result<Vec<(DASDDevice, DASDDeviceConfig)>, ServiceError> { | ||
| let devices = self.devices().await?; | ||
| let devices_map: HashMap<&str, DASDDevice> = devices | ||
| .iter() | ||
| .map(|d| (d.1.id.as_str(), d.1.clone())) | ||
| .collect(); | ||
| config | ||
| .devices | ||
| .iter() | ||
| .map(|c| { | ||
| Ok(( | ||
| devices_map | ||
| .get(c.channel.as_str()) | ||
| .ok_or(ServiceError::DASDChannelNotFound(c.channel.clone()))? | ||
| .clone(), | ||
|
||
| c.clone(), | ||
| )) | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| pub async fn probe(&self) -> Result<(), ServiceError> { | ||
| Ok(self.manager_proxy.probe().await?) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,9 @@ | |
| // find current contact information at www.suse.com. | ||
|
|
||
| //! Implements a client to access Agama's storage service. | ||
| use serde_json::value::RawValue; | ||
|
|
||
| pub mod dasd; | ||
| pub mod iscsi; | ||
|
|
||
| use crate::{ | ||
| base_http_client::{BaseHTTPClient, BaseHTTPClientError}, | ||
|
|
@@ -49,29 +51,3 @@ impl StorageHTTPClient { | |
| Ok(self.client.put_void("/storage/config", config).await?) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| pub enum ISCSIHTTPClientError { | ||
| #[error(transparent)] | ||
| ISCSI(#[from] BaseHTTPClientError), | ||
| } | ||
|
|
||
| pub struct ISCSIHTTPClient { | ||
| client: BaseHTTPClient, | ||
| } | ||
|
|
||
| impl ISCSIHTTPClient { | ||
| pub fn new(base: BaseHTTPClient) -> Self { | ||
| Self { client: base } | ||
| } | ||
|
|
||
| pub async fn get_config(&self) -> Result<Option<StorageSettings>, ISCSIHTTPClientError> { | ||
| // TODO: implement it as part of next step | ||
| //self.client.get("/storage/config").await | ||
| Ok(None) | ||
| } | ||
|
|
||
| pub async fn set_config(&self, config: &Box<RawValue>) -> Result<(), ISCSIHTTPClientError> { | ||
| Ok(self.client.post_void("/iscsi/config", config).await?) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Copyright (c) [2025] SUSE LLC | ||
| // | ||
| // All Rights Reserved. | ||
| // | ||
| // This program is free software; you can redistribute it and/or modify it | ||
| // under the terms of the GNU General Public License as published by the Free | ||
| // Software Foundation; either version 2 of the License, or (at your option) | ||
| // any later version. | ||
| // | ||
| // This program is distributed in the hope that it will be useful, but WITHOUT | ||
| // ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| // FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for | ||
| // more details. | ||
| // | ||
| // You should have received a copy of the GNU General Public License along | ||
| // with this program; if not, contact SUSE LLC. | ||
| // | ||
| // To contact SUSE LLC about this file by physical or electronic mail, you may | ||
| // find current contact information at www.suse.com. | ||
|
|
||
| //! Implements a client to access Agama's iscsi service. | ||
|
|
||
| use crate::{ | ||
| base_http_client::{BaseHTTPClient, BaseHTTPClientError}, | ||
| storage::settings::dasd::DASDConfig, | ||
| }; | ||
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum DASDHTTPClientError { | ||
| #[error(transparent)] | ||
| DASD(#[from] BaseHTTPClientError), | ||
| } | ||
|
|
||
| pub struct DASDHTTPClient { | ||
| client: BaseHTTPClient, | ||
| } | ||
|
|
||
| impl DASDHTTPClient { | ||
| pub fn new(base: BaseHTTPClient) -> Self { | ||
| Self { client: base } | ||
| } | ||
|
|
||
| pub async fn get_config(&self) -> Result<Option<DASDConfig>, DASDHTTPClientError> { | ||
| Ok(self.client.get("/storage/dasd/config").await?) | ||
| } | ||
|
|
||
| pub async fn set_config(&self, config: &DASDConfig) -> Result<(), DASDHTTPClientError> { | ||
| Ok(self.client.put_void("/storage/dasd/config", config).await?) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.