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

Externally built Integrations are deployed without a command in Camel-K 2.2.0 #5112

Closed
Exether opened this issue Jan 25, 2024 · 13 comments · Fixed by #5151
Closed

Externally built Integrations are deployed without a command in Camel-K 2.2.0 #5112

Exether opened this issue Jan 25, 2024 · 13 comments · Fixed by #5151
Assignees
Labels
kind/bug Something isn't working
Milestone

Comments

@Exether
Copy link

Exether commented Jan 25, 2024

What happened?

While creating an Integration with an Image specified in Container, I noticed that the corresponding Deployment created by the operator is lacking the command line resulting in the pod not able to start.

Steps to reproduce

  1. Start a basic Integration
    kamel run my-integration.yaml
  2. Once it is started tag the image
    docker tag docker tag 07cd1c6e2b31 nexus-camel-k:48083/fm/integrations/my-integration:2.2.0
  3. Also save your integration to a file
    kamel run my-integration.yaml -o yaml > itg.my-integration.yaml
  4. Update itg.my-integration.yaml file with the image in the container part
     container:
	    image: nexus-camel-k:48083/fm/integrations/my-integration:2.2.0
  1. Remove all integrations (but not the images)
    kamel delete --all
  2. Deploy the integration with the image:
    kubectl apply -f itg.my-integration.yaml
  3. Check the status
    integrationKit is ok
    deployment is created but does not contain a startup command
    pod remains CreateContainerError ('Error response from daemon: No command specified.' error message)

The same procedure with version 2.1.0 or 1.10.2 works well.

Relevant log output

No response

Camel K version

2.2.0

@Exether Exether added the kind/bug Something isn't working label Jan 25, 2024
@squakez
Copy link
Contributor

squakez commented Jan 26, 2024

I think it's a side effect of #4831 In order this to work, we check if the image was generated by a camel-k-kit-* regex [1]. In your case, as you're renaming the kit, the operator thinks it's an external executable kit, for which it does not require the startup command. I am not sure that renaming a kit is a wise thing to do, in general. From a design perspective it is thought to be working as an inner part of the operator. I'd suggest to maintain the convention, or, at least use a name which would match [1].

[1]

// It's a user provided image if it does not match the naming convention used by Camel K Integration Kits.
func (t *containerTrait) hasUserProvidedImage() bool {
return t.Image != "" && !strings.Contains(t.Image, "camel-k-kit-")
}

@squakez squakez added the status/waiting-for-feedback Needs some feedback label Jan 26, 2024
@Exether
Copy link
Author

Exether commented Jan 29, 2024

Thanks for the feedback.
In our use case we build the images on a specific namespace, then we retag them for later we reuse on various namespaces. We use the project-route name and version to tag the image because our project names are long already.
We can still prepend 'camel-k-kit-' but wouldn't it make sense to add another container trait e.g. deployWithCommand to use in the above function and overload this behaviour when it is true ?

@cazzoo
Copy link

cazzoo commented Jan 29, 2024

What about providing a variable for "camel-k-kit-" that would be overridable per user will?

@lburgazzoli
Copy link
Contributor

@squakez is checking the name of the image really needed ? I recall that when a container image is explicitly set, then the operator creates an IntegrationKit of type external which can be use to decide how to compute the command line instead of relying on the image name

@squakez
Copy link
Contributor

squakez commented Jan 30, 2024

@lburgazzoli yeah, we can try to improve the check to leverage the Kit instead of the Integration image name. I am not 100% sure if at the time of checking, the Kit is already available in the environment, but it's worth to make some validation and apply the change if it really works.

@squakez squakez removed the status/waiting-for-feedback Needs some feedback label Jan 30, 2024
@squakez squakez added this to the 2.3.0 milestone Jan 30, 2024
@gansheer
Copy link
Contributor

gansheer commented Feb 1, 2024

@squakez I'll take care of this.

@gansheer
Copy link
Contributor

gansheer commented Feb 6, 2024

While the #4831 change had the effect to deactivate the jvm trait, it did not change the fact the use case described here resulted to an external integration kit even in 2.1.x version.
And in the case of an container image explicitly set, we don't really know what created the image (camel-k operator or custom). Also changing from using the naming convention of the image to the fact that we have an external kit would just disable the workaround given by the naming convention check.

@squakez
Copy link
Contributor

squakez commented Feb 6, 2024

Yes, either we rely on the name or the presence of the "external" type IntegrationKit would lead to the same result. The operator cannot know for sure if a container was created by the user. We may think to let the user enforce the usage of the jvm trait by not disabling if this is explicitly set to enabled (ie, -t jvm.enabled=true). I guess that's a check we can do, but would require the user to forcefully add this option if she does not want the operator to apply it.

@lburgazzoli
Copy link
Contributor

Also changing from using the naming convention of the image to the fact that we have an external kit would just disable the workaround given by the naming convention check.

still, I think that we should remove the container image name check as it is redundant and not in line with the design principle of the IntegrationKit.

The operator cannot know for sure if a container was created by the user.

Yes for an external integration kit we should assume that the container image has a proper entrypoint and disable some traits such as the jvm one (and maybe some others)

The proper way for reuse a container image should be to also copy the related IntegrationKit and instruct the integration to use it.

@squakez
Copy link
Contributor

squakez commented Feb 6, 2024

That's the way it works now. A "synthetic" kit is created after the image name provided by the user. We may check the "external" type property to make it more consistent, but, from a practical point of view, nothing would change. I agree, the usage of a "raw" container backed by a previously built kit is not a good practice. As you say, the ideal world (also for re-usability) would be to copy the Kit resource.

@lburgazzoli
Copy link
Contributor

That's the way it works now. A "synthetic" kit is created after the image name provided by the user.

This is true if you provide an image but if I remember correctly, if you do something like kamel run --kit foo then camel-k will use the foo IntegrationKit to run the integration so that it is capable to properly compute the classpath.

@gansheer
Copy link
Contributor

gansheer commented Feb 6, 2024

I think it would be better for now to :

  • change the check to evaluate if the kit is external or not
  • let the possibility for the user to explicitly enable the -t jvm.enabled=true and maybe add a condition to inform the user that it is disabled by default and enabling the trait is a risk

That way the users that where using the workaround can still do it without having to have an internal kit.
Also as a personal rule I prefer when any explicit configuration from the user is not overriden by an "hidden" mecanism. This leads to confusion.

@lburgazzoli
Copy link
Contributor

Just for info, in the past we had an option to create a standalone/runnable image, may be worth thinking to re-introduce it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants