chore: added ods version compatibility check by JoergZeidler · Pull Request #41 · omnect/omnect-ui

JoergZeidler · 2025-05-13T14:43:43Z

No description provided.

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

JoergZeidler · 2025-05-13T14:46:28Z

src/api.rs

+
+        let info = VERSION_CHECK.lock().unwrap().clone();
+
+        match info {
+            Some(result) => {
+                if result.is_below_min {
+                    HttpResponse::InternalServerError().json(result)
+                } else {
+                    HttpResponse::Ok().json(result)
+                }
+            }
+            None => HttpResponse::Ok().body("No version check performed yet"),
+        }


healthcheck must be called in another PR during startup and handle the result

src/main.rs

src/common.rs

src/api.rs

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

src/api.rs

src/common.rs

JanZachmann · 2025-05-13T15:17:06Z

src/common.rs

+    let min_version = Version::parse(MIN_ODS_VERSION).expect("parse MIN_ODS_VERSION");
+    let current_version = Version::parse(&ods_version).expect("parse ods_version");
+    let is_below_min = current_version < min_version;
+    {
+        let mut version_check = VERSION_CHECK.lock().unwrap();
+        *version_check = Some(VersionCheckResult {
+            min_version: MIN_ODS_VERSION.to_string(),
+            current_version: ods_version.clone(),
+            is_below_min,
+        });
+    }
+
+    Ok(())
+}


we should get rid of expect and unwrap

OnceLock would be better as LazyLock. You could move the code into get_or_init function

refactored to used OnceLock and removed unnecessary Mutex handling.

#41 (comment)

src/main.rs

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

empwilli · 2025-05-14T07:50:30Z

src/common.rs

+    let min_version = Version::parse(MIN_ODS_VERSION)
+        .map_err(|e| anyhow!("failed to parse MIN_ODS_VERSION: {e}"))?;


If we use a VersionReq for the target version, we are more flexible.

empwilli · 2025-05-14T07:53:47Z

src/main.rs

+    fs::exists(&ods_socket_path).unwrap_or_else(|_| {
+        panic!(
+            "omnect device service socket file {} does not exist",
+            &ods_socket_path
+        )
+    });


IMHO we don't need this check. There might be cases where the unix domain socket exists and is deleted later on. We should instead ensure, that the open call to the unix domain socket does not attempt to create a file if the file does not exist, and handle the error on opening.

Ah, I just saw, that this is pre-existing code. Still: just drop the check :)

@JanZachmann @ronny-standtke

@empwilli what do you mean by "...attempt to create a file..."?

empwilli · 2025-05-14T08:04:39Z

src/main.rs

+pub const MIN_ODS_VERSION: &str = "0.39.0";
+


If we use VersionReq instead, I'd adjust the name so that it is REQ_ODS_VERSION

empwilli · 2025-05-14T08:13:19Z

src/common.rs

+    let Some(omnect_device_service_version) =
+        &status_response.system_info.omnect_device_service_version
+    else {
+        bail!("failed to get omnect_device_service_version from status response")
+    };


We expect the response of the ODS to contain omnect_device_service_version in any case and error out if it is not set. IMHO we don't set the type as Option then.

empwilli · 2025-05-14T08:18:41Z

src/common.rs

-use std::{fs, io::Write, path::Path};
+use std::{fs, io::Write, path::Path, sync::OnceLock};
+
+pub static VERSION_CHECK: OnceLock<VersionCheckResult> = OnceLock::new();


IMHO use a lazy lock here: this would drop the case in the healthcheck route above for the case where VERSION_CHECK.get() returns None.

@JoergZeidler @empwilli @ronny-standtke
I had a closer look and we shouldn't use any (std::sync::***Lock) at all. Therefore we should add a new function to Api and call get_status once and parse and save all data we need later on in Api calls. Thus we can get rid of further calls of get_status (e.g. for fleet_id) and we need no global static std::sync::***Lock

removed OnceLock handling. Imho refactoring with new get_status() handline should be done after refinement with @ronny-standtke and @JanZachmann

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

src/api.rs

JanZachmann · 2025-05-14T17:12:26Z

src/api.rs

+        if let Some(result) = VERSION_CHECK.get() {
+            if result.version_mismatch {
+                HttpResponse::ServiceUnavailable().json(result)
+            } else {
+                HttpResponse::Ok().json(result)
+            }
+        } else {
+            HttpResponse::Ok().body("No version check performed yet")
+        }


can be done all in one match

match VERSION_CHECK.get() { Some(result) if result.version_mismatch => { HttpResponse::ServiceUnavailable().json(result) } Some(result) => HttpResponse::Ok().json(result), _ => HttpResponse::Ok().body("No version check performed yet"), }

JanZachmann · 2025-05-14T18:18:15Z

src/common.rs

-use std::{fs, io::Write, path::Path};
+use std::{fs, io::Write, path::Path, sync::OnceLock};
+
+pub static VERSION_CHECK: OnceLock<VersionCheckResult> = OnceLock::new();


@JoergZeidler @empwilli @ronny-standtke
I had a closer look and we shouldn't use any (std::sync::***Lock) at all. Therefore we should add a new function to Api and call get_status once and parse and save all data we need later on in Api calls. Thus we can get rid of further calls of get_status (e.g. for fleet_id) and we need no global static std::sync::***Lock

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

chore: added ods version compatibility check

5fc7a00

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

JoergZeidler requested review from JanZachmann, empwilli and ronny-standtke May 13, 2025 14:43

JoergZeidler commented May 13, 2025

View reviewed changes

ronny-standtke requested changes May 13, 2025

View reviewed changes

src/main.rs Show resolved Hide resolved

src/common.rs Outdated Show resolved Hide resolved

src/api.rs Outdated Show resolved Hide resolved

qa

5d5aa99

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

JoergZeidler requested a review from ronny-standtke May 13, 2025 15:15

JanZachmann requested changes May 13, 2025

View reviewed changes

ronny-standtke requested changes May 13, 2025

View reviewed changes

src/main.rs Show resolved Hide resolved

JoergZeidler added 2 commits May 14, 2025 08:00

qa

1ad8ff7

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

qa

8801416

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

JoergZeidler requested review from JanZachmann and ronny-standtke May 14, 2025 07:31

ronny-standtke approved these changes May 14, 2025

View reviewed changes

empwilli approved these changes May 14, 2025

View reviewed changes

JoergZeidler added 2 commits May 14, 2025 12:26

use semver VersionReq mechanism

e336899

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

get rid of omnect_device_service_version Option handling

150380a

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

JanZachmann requested changes May 14, 2025

View reviewed changes

get rid of OnceLock

c2a7c85

Signed-off-by: Joerg Zeidler <62105035+JoergZeidler@users.noreply.github.com>

JoergZeidler requested a review from JanZachmann May 15, 2025 15:21

JanZachmann approved these changes May 15, 2025

View reviewed changes

JoergZeidler merged commit b557cab into omnect:main May 19, 2025
2 checks passed

		let min_version = Version::parse(MIN_ODS_VERSION)
		.map_err(\|e\| anyhow!("failed to parse MIN_ODS_VERSION: {e}"))?;

Conversation

JoergZeidler commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanZachmann May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanZachmann May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JanZachmann May 14, 2025 •

edited

Loading

JanZachmann May 14, 2025 •

edited

Loading