Skip to content
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

[#3537] Get K8s container id via K8s API. #3546

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

calohmn
Copy link
Contributor

@calohmn calohmn commented Sep 22, 2023

This fixes #3537 .

@calohmn calohmn added the C&C Command and Control label Sep 22, 2023
@calohmn calohmn added this to In progress in 2.5.0 via automation Sep 22, 2023
@calohmn
Copy link
Contributor Author

calohmn commented Sep 22, 2023

Can be tested with these adaptations to the Hono Helm chart: eclipse/packages#509 (K8s API will only be used if Cgroups v2 is used).

@calohmn
Copy link
Contributor Author

calohmn commented Sep 22, 2023

Note that the K8s API request handling is synchronous here. On application startup, the getContainerId method is first invoked from the main thread.

Call stack

org.eclipse.hono.client.command.KubernetesContainerUtil.getContainerId(KubernetesContainerUtil.java:81)
org.eclipse.hono.client.command.CommandRoutingUtil.getNewAdapterInstanceId(CommandRoutingUtil.java:57)
org.eclipse.hono.client.command.ProtocolAdapterCommandConsumerFactoryImpl.(ProtocolAdapterCommandConsumerFactoryImpl.java:77)
org.eclipse.hono.adapter.AbstractProtocolAdapterApplication.commandConsumerFactory(AbstractProtocolAdapterApplication.java:609)
org.eclipse.hono.adapter.AbstractProtocolAdapterApplication.setCollaborators(AbstractProtocolAdapterApplication.java:408)
org.eclipse.hono.adapter.amqp.app.Application.adapter(Application.java:51)
org.eclipse.hono.adapter.amqp.app.Application.adapter(Application.java:26)
io.vertx.core.impl.DeploymentManager.doDeploy(DeploymentManager.java:154)
io.vertx.core.impl.DeploymentManager.deployVerticle(DeploymentManager.java:72)
io.vertx.core.impl.VertxImpl.deployVerticle(VertxImpl.java:718)
io.vertx.core.impl.VertxImpl.deployVerticle(VertxImpl.java:698)
org.eclipse.hono.adapter.AbstractProtocolAdapterApplication.doStart(AbstractProtocolAdapterApplication.java:303)
org.eclipse.hono.service.AbstractServiceApplication.onStart(AbstractServiceApplication.java:186)
org.eclipse.hono.service.AbstractServiceApplication_Observer_onStart_9175e72ff8337562e079de521daf6d4270c94c01.notify(Unknown Source)

When deploying Hono on Minikube on my local machine, I see that the getContainerId method takes 7-8s.

A further optimization could be to defer the getContainerId invocation, letting it be done in a Vertx worker thread in the Future<Void> start() method of ProtocolAdapterCommandConsumerFactoryImpl. The corresponding refactoring doesn't look that straightforward though. I would therefore rather keep the first implementation here in this synchronous manner.
On could also argue that in terms of the time needed for all the Hono components to get ready on Helm chart deployment, the initial time spent in the protocol adapter pods isn't that relevant because the bottle neck is the startup time of Kafka and the device-registry, and the overall load on the K8s components. When restarting an adapter pod at a later point in time, when all other components are ready, the time needed for the getContainerId method was 1.6s on my machine.

EDIT: PR got changed now, determining the id on a vert.x worker thread.

@sophokles73
Copy link
Contributor

A further optimization could be to defer the getContainerId invocation, letting it be done in a Vertx worker thread in the Future start() method of ProtocolAdapterCommandConsumerFactoryImpl.

I would very much prefer this. My concern is that if you don't refactor this now, it will never get refactored accordingly ...

@calohmn
Copy link
Contributor Author

calohmn commented Sep 24, 2023

Ok, I'll have a closer look here.
Btw, since Quarkus 3.0, the quarkus-kubernetes-client dependency uses the fabric8 kubernetes-client version >= 6.4, which includes support for using the vert.x HTTP client under the hood instead of the OkHttp client. quarkus-kubernetes-client lets this vert.x HTTP client be used by default (see quarkusio/quarkus#30480).
This doesn't mean there is support for getting a Future<Pod> result from the fabric8 client (see this discussion thread), but nonetheless it seems to be preferable letting these HTTP requests be managed via the same Vert.x instance that is already used in Hono, also getting rid of the extra OkHttp dependency.
Have there been any attempts already to use Quarkus 3.x within Hono?

@sophokles73
Copy link
Contributor

Have there been any attempts already to use Quarkus 3.x within Hono?

Not yet. I have created #3547

@calohmn
Copy link
Contributor Author

calohmn commented Sep 28, 2023

@sophokles73 I've updated the PR, letting the container id be determined on a vert.x worker thread now.

/**
* Provides information about the Kubernetes container if this application is running in Kubernetes.
*/
public class KubernetesContainerInfoProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean it can't be mocked anymore (in ProtocolAdapterCommandConsumerFactoryTest).

return Future.succeededFuture(containerId);
}
final Promise<String> containerIdPromise = Promise.promise();
if (!containerIdPromiseRef.compareAndSet(null, containerIdPromise)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is such an expensive operation and you anticipate this operation to be invoked concurrently/multiple times, wouldn't it make sense to cache the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result is already cached via the containerId field value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad

if (containerId != null) {
return Future.succeededFuture(containerId);
}
final Context currentContext = Vertx.currentContext();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about passing a Context into the method instead of relying on the static method to obtain the context? That way, it would also be much easier to test this method, right?

Or maybe it would make even more sense to pass a Vertx instance to the getInstance method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now added a context parameter to the getContainerId() method.

@sophokles73 sophokles73 added this to the 2.5.0 milestone Oct 4, 2023
@sophokles73
Copy link
Contributor

@calohmn IMHO we also should backport this to 2.4.x and 2.3.x

@sophokles73 sophokles73 added this to In progress in 2.4.1 via automation Oct 7, 2023
@sophokles73 sophokles73 added this to In progress in 2.3.2 via automation Oct 7, 2023
@sophokles73 sophokles73 removed this from In progress in 2.3.2 Oct 7, 2023
@sophokles73 sophokles73 removed this from In progress in 2.4.1 Oct 7, 2023
@calohmn calohmn force-pushed the PR/container_id_via_k8s_api branch from ad5b26b to 191fda0 Compare October 8, 2023 09:00
@calohmn
Copy link
Contributor Author

calohmn commented Oct 8, 2023

IMHO we also should backport this to 2.4.x and 2.3.x

Yes, we can do that.

return Future.succeededFuture(containerId);
}
final Promise<String> containerIdPromise = Promise.promise();
if (!containerIdPromiseRef.compareAndSet(null, containerIdPromise)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad

* @param context The context.
* @throws NullPointerException if context is {@code null}.
*/
void setContext(final Context context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a particular reason why you do not want to pass in a Vertx instance to the constructor as we do in most other services?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that is the better solution. I've changed it now.

@calohmn calohmn force-pushed the PR/container_id_via_k8s_api branch from 191fda0 to 4d2bcaf Compare October 9, 2023 05:34
@calohmn calohmn requested a review from dejanb as a code owner October 9, 2023 05:34
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@calohmn calohmn merged commit dc13f1b into eclipse-hono:master Oct 9, 2023
7 checks passed
2.5.0 automation moved this from In progress to Done Oct 9, 2023
@calohmn calohmn deleted the PR/container_id_via_k8s_api branch October 9, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C&C Command and Control
Projects
No open projects
2.5.0
Done
Development

Successfully merging this pull request may close these issues.

Topics hono.command_internal.* are not being cleaned up on Docker versions >20.10
2 participants