feat: if condition exists, updating it, otherwise inserting it to the conditions list in the CRD, in the reconcile function#260
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SpaceFace02 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideThis PR changes the reconcile flow to preserve existing status conditions by introducing a generic upsert helper, updates reconcile/tests to assert that foreign controller conditions are preserved and operator-owned conditions are updated in place, and applies several small readability/robustness cleanups across the codebase and test docs. Sequence diagram for reconcile status condition upsert flowsequenceDiagram
participant Reconcile as reconcile
participant Status as TrustedExecutionClusterStatus
participant Upsert as upsert_condition
participant Kube as update_status
Reconcile->>Status: read status.conditions
Status-->>Reconcile: Option<Vec<Condition>>
Reconcile->>Upsert: upsert_condition(conditions, address_condition)
Reconcile->>Upsert: upsert_condition(conditions, uninstall_condition)
Reconcile->>Upsert: upsert_condition(conditions, non_unique_condition)
Reconcile->>Upsert: upsert_condition(conditions, installing_condition)
Reconcile->>Upsert: upsert_condition(conditions, installed_condition)
Upsert-->>Reconcile: conditions updated in place
Reconcile->>Kube: update_status(TrustedExecutionClusterStatus { conditions })
Kube-->>Reconcile: Action::await_change / Action::requeue
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
upsert_conditionhelper currently identifies conditions only bytype_; if there’s any chance multiple controllers share a condition type, you may want to refine the match (e.g., include controller-specific markers or reason) to avoid unintentionally overwriting another controller’s condition of the same type. - In
reconcile,conditionsis cloned solely to build the intermediateinstallingstatus; you could avoid that extra allocation by updating in-place and reusing the sameconditionsvalue for both the installing and installed status updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `upsert_condition` helper currently identifies conditions only by `type_`; if there’s any chance multiple controllers share a condition type, you may want to refine the match (e.g., include controller-specific markers or reason) to avoid unintentionally overwriting another controller’s condition of the same type.
- In `reconcile`, `conditions` is cloned solely to build the intermediate `installing` status; you could avoid that extra allocation by updating in-place and reusing the same `conditions` value for both the installing and installed status updates.
## Individual Comments
### Comment 1
<location path="lib/src/lib.rs" line_range="158-155" />
<code_context>
}
+
+// Update condition if already present, otherwise append(insert) it into the conditions vector.
+pub fn upsert_condition(conditions: &mut Option<Vec<Condition>>, condition: Condition) {
+ let conditions_vec = conditions.get_or_insert_with(Vec::new);
+ if let Some(existing) = conditions_vec.iter_mut().find(|c| c.type_ == condition.type_){
+ *existing = condition;
+ } else {
+ conditions_vec.push(condition);
+ }
+}
\ No newline at end of file
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid overwriting identical conditions to preserve transition semantics and reduce unnecessary status churn.
Because `upsert_condition` always replaces the existing `Condition` when `type_` matches, it resets `last_transition_time` and other fields even when nothing has actually changed. This creates noisy status updates and obscures real transitions. Consider short-circuiting when the incoming condition is effectively the same (e.g., `status`, `reason`, and `message` unchanged), and only updating when there is a meaningful change to preserve transition semantics and avoid unnecessary PATCHes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
e883858 to
66267de
Compare
|
I believe we have a setting which makes |
Weird, we have this job that check the cargo and clippy. Might be that the version you have installed is more recent than the toolchain in the CI EDIT: from the CI the version is |
|
@SpaceFace02 thanks, just a couple of nits |
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
78ed44f to
105a3ca
Compare
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
upsert_conditionshould also be used byimage_add_reconcile- Please put fixes by
rustfmtin the same commit
Tip: If you put Fixes: #220 in a commit message, it will auto-link that issue. I've linked it for now.
| let run = if args.cert_path.is_some() && args.key_path.is_some() { | ||
| let config = OpenSSLConfig::from_pem_file(args.cert_path.unwrap(), args.key_path.unwrap()) | ||
| .expect("invalid PEM files"); | ||
| let run = if let (Some(cert_path), Some(key_path)) = (args.cert_path, args.key_path) { |
There was a problem hiding this comment.
(also in response to
I believe we have a setting which makes cargo fmt and clippy return warnings as errors, which made me change a few other parts of the code I did not modify for the feature. Is this intentional?
Weird, we have this job that check the cargo and clippy. Might be that the version you have installed is more recent than the toolchain in the CI
)
I agree with this change, and you're invited to keep making them (in separate commits like you did here), but let me give some context as to why it's not caught by CI in the first place: This has only become a lint recently, and we use a Rust version supported by RHEL/UBI to avoid downstream patching when using a Rust feature that's newer than that. We also use the linting as of that version because sometimes a new Rust feature also becomes a lint to use that feature relatively fast or even in the same version.
With that said, we could update to 1.92 (not for this PR). Ideally it's even possible to get dependabot to update to a version supported by e.g. registry.access.redhat.com/ubi9. This could maybe also go in a doc then.
| let body = String::from_utf8_lossy(&bytes); | ||
| assert!(body.contains(contains)); | ||
|
|
||
| for (expected_reason, expected_err_msg) in expected { |
There was a problem hiding this comment.
I feel the loop refactor is a little overkill for one use
There was a problem hiding this comment.
Yes, I didn't want to do it initially, but in order to test multiple conditions and to see whether all fields are preserved, I added this.
this method needs ownership of body, as it consumes it, and I didn't want to clone too many times in the caller function, so I thought this is a better way to handle this, albeit a bit verbose.
LMK
There was a problem hiding this comment.
Right, I see the problem, the Body isn't Clone because it could be a Wrap… honestly I'd just change the assert_body_contains function to something like
pub async fn get_body_string(req: Request<Body>) -> String {
let bytes = req.into_body().collect_bytes().await.unwrap().to_vec();
String::from_utf8_lossy(&bytes).to_string()
}and assert on that from the callers, saves a lot of lines:
diff --git a/operator/src/main.rs b/operator/src/main.rs
index 05b4868..682ca03 100644
--- a/operator/src/main.rs
+++ b/operator/src/main.rs
@@ -371,15 +371,8 @@ mod tests {
async fn test_reconcile_uninstalling() {
let clos = async |req: Request<Body>, ctr| match req.method() {
&Method::PATCH => {
- let uninstall_assertion_reason = "Request body did not contain Uninstall condition";
- assert_body_contains(
- req,
- &[(
- NOT_INSTALLED_REASON_UNINSTALLING,
- Some(uninstall_assertion_reason),
- )],
- )
- .await;
+ let body = get_body_string(req).await;
+ assert!(body.contains(NOT_INSTALLED_REASON_UNINSTALLING));
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
}
_ => panic!("unexpected API interaction: {req:?}, counter {ctr}"),
@@ -406,9 +399,8 @@ mod tests {
// Watchers
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
} else if ctr == 4 && req.method() == Method::PATCH {
- let err_msg = "Request body did not contain Non-Unique condition";
- assert_body_contains(req, &[(NOT_INSTALLED_REASON_NON_UNIQUE, Some(err_msg))])
- .await;
+ let body = get_body_string(req).await;
+ assert!(body.contains(NOT_INSTALLED_REASON_NON_UNIQUE));
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
} else {
panic!("unexpected API interaction: {req:?}, counter {ctr}");
@@ -453,24 +445,10 @@ mod tests {
let clos = async |req: Request<Body>, ctr| match req.method() {
&Method::PATCH => {
- assert_body_contains(
- req,
- &[
- (
- "ForeignCondition",
- Some("Request body did not contain ForeignCondition"),
- ),
- (
- "ExternalController",
- Some("Request body did not contain ExternalController"),
- ),
- (
- NOT_INSTALLED_REASON_UNINSTALLING,
- Some("Request body did not contain Uninstall condition"),
- ),
- ],
- )
- .await;
+ let body = get_body_string(req).await;
+ assert!(body.contains("ForeignCondition"));
+ assert!(body.contains("ExternalController"));
+ assert!(body.contains(NOT_INSTALLED_REASON_UNINSTALLING));
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
}
_ => panic!("unexpected API interaction: {req:?}, counter {ctr}"),
diff --git a/test_utils/src/mock_client.rs b/test_utils/src/mock_client.rs
index aa0a259..08e34e8 100644
--- a/test_utils/src/mock_client.rs
+++ b/test_utils/src/mock_client.rs
@@ -106,17 +106,9 @@ where
}
}
-pub async fn assert_body_contains(req: Request<Body>, expected: &[(&str, Option<&str>)]) {
+pub async fn get_body_string(req: Request<Body>) -> String {
let bytes = req.into_body().collect_bytes().await.unwrap().to_vec();
- let body = String::from_utf8_lossy(&bytes);
-
- for (expected_reason, expected_err_msg) in expected {
- let msg = expected_err_msg.unwrap_or("Given body doesn't contain the expected string");
- assert!(
- body.contains(expected_reason),
- "{msg}: expected '{expected_reason}', got: {body}"
- );
- }
+ String::from_utf8_lossy(&bytes).to_string()
}
pub async fn test_create_success<| "Request body did not contain ForeignCondition, got: {body}" | ||
| ); | ||
|
|
||
| if body.contains(INSTALLED_REASON) { |
There was a problem hiding this comment.
Why is this in a block, don't we always expect that there is exactly one Installed condition?
There was a problem hiding this comment.
Yes, there is only 1 installed condition, but 2 patch calls with different reasons (one with Installing reason and one for InstallationCompleted reason).
Here I am checking that in the last PATCH call (with body containing Reason=InstallationCompleted), does reconcile update existing Installed Condition with new status and reason (expected behaviour), or does it append another condition instead (what we don't want).
There was a problem hiding this comment.
Ah, right. In that case: nit: maybe do an early return here, the indenting is already really deep
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
f1f4e7f to
41e43e4
Compare
Have added the upsert_condition for both image_add_reconcile and ak_reconcile. |
… conditions list in the CRD, in the main reconcile function of operator, attestation key register ak reconcile and reference image_add_reconcile Fix: trusted-execution-clusters#220
…presence of a secret indicates http or https.
SpaceFace02
force-pushed
the
bugfix/upsert-existing-conditions-no-overwrite
branch
from
41e43e4 to
824d2ee
Compare
By adding this to commit message, it will reference the commit instead of the PR right, so once that commit checks in the main branch, will it close the PR and resolve the issue? 
|
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
Tip: If you put Fixes: #220 in a commit message, it will auto-link that issue. I've linked it for now.
By adding this to commit message, it will reference the commit instead of the PR right, so once that commit checks in the main branch, will it close the PR and resolve the issue?
The issue gets auto-resolved when a PR that was linked to it is merged, and one way to link the PR to the issue is to add a commit to the PR that references this issue with a Fixes: tag.
Another nit on the commit message: Try to keep the subject line short and put the rest of the description in the body (ideally, subject line is at most 50 characters, but that's not a hard and fast rule)
| "Request body did not contain ForeignCondition, got: {body}" | ||
| ); | ||
|
|
||
| if body.contains(INSTALLED_REASON) { |
There was a problem hiding this comment.
Ah, right. In that case: nit: maybe do an early return here, the indenting is already really deep
| let body = String::from_utf8_lossy(&bytes); | ||
| assert!(body.contains(contains)); | ||
|
|
||
| for (expected_reason, expected_err_msg) in expected { |
There was a problem hiding this comment.
Right, I see the problem, the Body isn't Clone because it could be a Wrap… honestly I'd just change the assert_body_contains function to something like
pub async fn get_body_string(req: Request<Body>) -> String {
let bytes = req.into_body().collect_bytes().await.unwrap().to_vec();
String::from_utf8_lossy(&bytes).to_string()
}and assert on that from the callers, saves a lot of lines:
diff --git a/operator/src/main.rs b/operator/src/main.rs
index 05b4868..682ca03 100644
--- a/operator/src/main.rs
+++ b/operator/src/main.rs
@@ -371,15 +371,8 @@ mod tests {
async fn test_reconcile_uninstalling() {
let clos = async |req: Request<Body>, ctr| match req.method() {
&Method::PATCH => {
- let uninstall_assertion_reason = "Request body did not contain Uninstall condition";
- assert_body_contains(
- req,
- &[(
- NOT_INSTALLED_REASON_UNINSTALLING,
- Some(uninstall_assertion_reason),
- )],
- )
- .await;
+ let body = get_body_string(req).await;
+ assert!(body.contains(NOT_INSTALLED_REASON_UNINSTALLING));
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
}
_ => panic!("unexpected API interaction: {req:?}, counter {ctr}"),
@@ -406,9 +399,8 @@ mod tests {
// Watchers
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
} else if ctr == 4 && req.method() == Method::PATCH {
- let err_msg = "Request body did not contain Non-Unique condition";
- assert_body_contains(req, &[(NOT_INSTALLED_REASON_NON_UNIQUE, Some(err_msg))])
- .await;
+ let body = get_body_string(req).await;
+ assert!(body.contains(NOT_INSTALLED_REASON_NON_UNIQUE));
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
} else {
panic!("unexpected API interaction: {req:?}, counter {ctr}");
@@ -453,24 +445,10 @@ mod tests {
let clos = async |req: Request<Body>, ctr| match req.method() {
&Method::PATCH => {
- assert_body_contains(
- req,
- &[
- (
- "ForeignCondition",
- Some("Request body did not contain ForeignCondition"),
- ),
- (
- "ExternalController",
- Some("Request body did not contain ExternalController"),
- ),
- (
- NOT_INSTALLED_REASON_UNINSTALLING,
- Some("Request body did not contain Uninstall condition"),
- ),
- ],
- )
- .await;
+ let body = get_body_string(req).await;
+ assert!(body.contains("ForeignCondition"));
+ assert!(body.contains("ExternalController"));
+ assert!(body.contains(NOT_INSTALLED_REASON_UNINSTALLING));
Ok(serde_json::to_string(&dummy_cluster()).unwrap())
}
_ => panic!("unexpected API interaction: {req:?}, counter {ctr}"),
diff --git a/test_utils/src/mock_client.rs b/test_utils/src/mock_client.rs
index aa0a259..08e34e8 100644
--- a/test_utils/src/mock_client.rs
+++ b/test_utils/src/mock_client.rs
@@ -106,17 +106,9 @@ where
}
}
-pub async fn assert_body_contains(req: Request<Body>, expected: &[(&str, Option<&str>)]) {
+pub async fn get_body_string(req: Request<Body>) -> String {
let bytes = req.into_body().collect_bytes().await.unwrap().to_vec();
- let body = String::from_utf8_lossy(&bytes);
-
- for (expected_reason, expected_err_msg) in expected {
- let msg = expected_err_msg.unwrap_or("Given body doesn't contain the expected string");
- assert!(
- body.contains(expected_reason),
- "{msg}: expected '{expected_reason}', got: {body}"
- );
- }
+ String::from_utf8_lossy(&bytes).to_string()
}
pub async fn test_create_success<| let body = get_body_string(req).await; | ||
| assert!( | ||
| body.contains("ForeignCondition"), | ||
| "Request body did not contain ForeignCondition" |
There was a problem hiding this comment.
nit: I think the increased lines of adding assertion messages (+3 because of formatting) distract more from reading the test code than they help understanding a failure, the failure will always contain a line number
|
|
||
| pub async fn assert_body_contains(req: Request<Body>, expected: &[(&str, Option<&str>)]) { | ||
| pub async fn assert_body_contains(req: Request<Body>, contains: &str) { | ||
| let bytes = req.into_body().collect_bytes().await.unwrap().to_vec(); |
There was a problem hiding this comment.
this could be refactored to use get_body_string, or you drop the function as a whole
3 participants
Bug fix for: #220
Instead of recreating the conditions from scratch each time the reconcile function runs, this makes sure that foreign controller conditions are not overwritten, and same type conditions are updated, rather then recreated; new conditions are inserted.
Summary by Sourcery
Preserve and incrementally update status conditions during reconcile instead of rebuilding them from scratch, while tightening related tests and minor code ergonomics.
Bug Fixes:
Enhancements:
Tests: