-
Notifications
You must be signed in to change notification settings - Fork 127
Undeploy an API without having to delete the Host #1532
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
Conversation
@@ -182,4 +183,102 @@ impl ApiDeploymentApi { | |||
|
|||
Ok(Json("API deployment deleted".to_string())) | |||
} | |||
|
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.
This is the endpoint. /:site/:id/:version
@@ -457,10 +461,32 @@ impl<Namespace: GolemNamespace, AuthCtx: GolemAuthCtx> ApiDeploymentService<Auth | |||
} | |||
|
|||
if !remove_deployment_records.is_empty() { | |||
// 3. Get the specific API definitions being undeployed |
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.
Removing component constraints during undeploy. There is already a test in service_test. Here we do two loops, one to filter by name, so there is no namespace conflict. And another loop to get the actual api definition, it is possible to do this in a single loop.
I side stepped the Issue #1529 by adding those parts manually back in. Did Cargo Make fix to check if everything is working. |
Please also add a test for the new endpoint to https://github.com/golemcloud/golem/tree/main/integration-tests/tests/api |
@mschuwalow All local deployment test always fail for me, and this has been the case for a while, since 2-3 week old HEAD. Failed tests:
- api::api_security::get_api_security_scheme (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_security_scheme)
- api::api_security::create_api_security_scheme (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_security_scheme)
- api::api_deployment::create_multiple_api_deployments_and_update_component_1 (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_deployment)
- api::api_deployment::create_multiple_api_deployments_and_update_component_2 (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_deployment)
- api::api_deployment::get_all_api_deployments (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_deployment)
- api::invocation_context::invocation_context_test (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_deployment)
�[2m2025-04-16T15:52:29.295200Z�[0m �[32m INFO�[0m �[2mgolem_test_framework::components�[0m�[2m:�[0m [componentsvc] �[2m2025-04-16T15:52:29.295101Z�[0m �[34mDEBUG�[0m �[2mtonic::transport::channel::service::connection�[0m�[2m:�[0m connection task error: hyper::Error(Io, Custom { kind: BrokenPipe, error: "connection closed because of a broken pipe" })
- api::api_deployment::create_api_deployment_and_update_component (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_deployment)
- api::api_deployment::create_and_get_api_deployment (called `Result::unwrap()` on an `Err` value: not available on GRPC API: create_api_deployment) |
Those tests need to be run in http mode which is currently controlled by an environment variable (this is not ideal but has its reasons). You can see the details of it in the makefile |
f233a91
to
d174427
Compare
@@ -1247,6 +1247,18 @@ pub trait WorkerService: WorkerServiceInternal { | |||
} | |||
|
|||
async fn kill(&self); | |||
|
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.
added undeploy to test-framework
@mschuwalow This PR is ready for review, All checks passed |
let (host, subdomain) = if let Some(idx) = site.find('.') { | ||
let (subdomain, host) = site.split_at(idx); | ||
( | ||
host.trim_start_matches('.').to_string(), | ||
Some(subdomain.to_string()), | ||
) | ||
} else { | ||
(site, None) | ||
}; | ||
|
||
let api_site = ApiSite { host, subdomain }; | ||
|
||
// Check if the site exists | ||
let site_exists = self | ||
.deployment_service | ||
.get_by_site(&ApiSiteString::from(&api_site)) | ||
.await? | ||
.is_some(); | ||
|
||
if !site_exists { | ||
return Err(ApiEndpointError::not_found(safe( | ||
"Site not found".to_string(), | ||
))); | ||
} | ||
|
||
// Check if the API definition exists | ||
let api_definition_exists = self | ||
.deployment_service | ||
.get_by_id(&namespace, Some(api_definition_key.id.clone())) | ||
.await? | ||
.iter() | ||
.any(|deployment| { | ||
deployment.api_definition_keys.iter().any(|key| { | ||
key.id == api_definition_key.id && key.version == api_definition_key.version | ||
}) | ||
}); | ||
|
||
if !api_definition_exists { | ||
return Err(ApiEndpointError::not_found(safe( | ||
"API definition not found".to_string(), | ||
))); | ||
} | ||
|
||
let api_deployment = gateway_api_deployment::ApiDeploymentRequest { | ||
namespace: namespace.clone(), | ||
api_definition_keys: vec![api_definition_key], | ||
site: api_site, | ||
}; | ||
|
||
self.deployment_service | ||
.undeploy(&api_deployment, auth_ctx) | ||
.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.
Please move this logic out of the route into the ApiDeploymentService. Try to only do simple data transformations here.
The new function in the ApiDeploymentService should take a generic AuthCtx as a parameter, the concrete EmptyAuthCtx should be provided to the function when calling it here.
Looks good otherwise, thanks!
I moved the logic into service, The only thing in route, now is error/success message. {subdomain, domain} is converted into APIsite. All the logic of checking if a site or deployment is available has been moved to ApiDeploymentService. I am not sure why the api-and-integration-tests failed, which passed in the previous check, I haven't changed the code, I dont think it is related to my PR. Could you re-run it ? Thank you. or between this check and previous check all tests pass, and there has been no change between this and last. (Just removed a empty file) |
} | ||
|
||
// 4. Remove component constraints | ||
self.remove_component_constraints(definitions_to_undeploy, auth_ctx) |
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.
👍
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.
I hope you have a test around these
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.
Test for checking if component constraints have been removed, no not yet, I will add one
}; | ||
|
||
// Split the site string into host and subdomain parts | ||
let (host, subdomain) = if let Some(idx) = site.find('.') { |
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.
This is unreliable. I don't think you can really split host and site like that.
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.
I fix this, there should be a way to convert ApiSiteString to ApiSite already within golem and use that.
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.
Finding host and subdomain by splitting dots is unreliable and may not work in all cases.
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.
I don't think there is a way to convert ApiSiteString to ApiSite. I think it's good to avoid that need, if you see what I mean.
(site, None) | ||
}; | ||
|
||
let api_site = ApiSite { host, subdomain }; |
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.
I think ApiSiteString
may be of your help. may be your undeploy function can take this one instead of ApiSite
.
That to undeploy an API from a site, you really don't need to logically split domain and subdomain anymore.
@Nanashi-lab thanks a lot for this addition. LGTM, except for one thing. We should avoid the need to convert a site string to a host and subdomain. Also, excuse for the delay in review :) |
@afsalthaj Finished with the changes, you would have to rerun checks, failure is not related to the PR. Tested the changes locally. Original Golem
My Old PR
New PR
|
|| deployment_record.subdomain != deployment.site.subdomain | ||
|| deployment_record.host != deployment.site.host | ||
{ | ||
error!("Undeploying API definition - failed, site used by another API (under another namespace/API)"); |
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.
I removed this check, delete also doesn't have this check. It shouldn't be possible to have different namespace, but same subdomain and host.
Let's merge once the checks are good. LGTM now |
/closes #1494