[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-1007333096 @astefanutti any last look before I can merge? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-1007333096 @astefanutti any last look before I can merge? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-1004700084 Very clever suggestion @astefanutti. I like how it looks like now. Please see 1b405ee which has captured the changes you proposed. Both `mount` and `openapi` will look for autogenerated configmaps and let the `owner` and `gc` performs their responsibility. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-1002132432 I've finished another iteration of development in order to address the points discussed previously. With d85257c we're now validating the input submitted to the trait: we won't allow anything but `configmap` or `secret`. With 78e62f4 we're introducing a new trait, called `mount` which can be used to _mount_ volumes from the different resources we allow. I prefer this name to a generic `resource` as it declare exactly its purpose. The last outstanding point is about garbage collecting the autogenerated configmaps that are used and later removed. Right now they will live beside the Integration, until this one is deleted. I cannot figure out how to manage differently and let them be garbage collected as soon as they are updated (unless we do a manual clean). IMO we can keep them along. @astefanutti please, have a further look and let me know what do you think. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-999536039 > > > > Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this. > > > > > My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle > > > > > > > > > > > > The idea we discussed was to go in the direction to [remove any arbitrary file content from the integration](https://github.com/apache/camel-k/issues/2320#issuecomment-922718785). With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via `kamel` CLI to simplify his life. If the user is not able to run an Integration via `kamel run`, in fact, the feature cannot be provided. > > > > > > > > > That is my background too. My question was more, why does the `file` prefix beyond the CLI, and the generated ConfigMap seems to be treated differently than the other "standard" ones? > > > > > > If I understood correctly, the question is about generated Configmap difference with user provided Configmap. The only difference is about the lifecycle of the Configmap itself, as my idea is to have it living together and exclusively within the Integration lifecycle. This is mimicking the previous behavior, when a Configmap was generated beside the Integration. The difference is that the resource is not directly stored in the Integration and the Configmap is generated by the CLI (as it must access the file content). Another possibility would be to autogenerate the Configmap but let the lifecycle (update/delete) up to the user. However, I think it completely overlaps what we already provide with the normal configmap, just saving the manual step to create the configmap from the file resource. > > Hope it answer your question, I'm not entirely sure we're talking about the same thing here :) > > I think we are talking about the same thing :) But that is general description, that I still fail to reconcile with the actual implementation :) Maybe this could be cleared if you could help me understand: > > * Why the file information leaks into the _container_ trait configuration, following the comment ` Syntax: [configmap|secret|file]:name[key], where name represents the local file path or the configmap/secret` and my interpretation of the current implementation? > Ah, this is a mistake. I copied the description from the run command. In the trait we cannot have a `file` but only `configmap|secret`. The run is in charge to convert a file into the related autogenerated configmap, ie , `-t container.resource=configmap:cm-xyz`. > * Also why the generated ConfigMap name is hashed based on local information? Could we mimic what `kubectl create configmap` does? > Originally I used the file name as a configmap name. But that leads to 2 problems: 1. We are not able to provide the same file name in the namespace, ie, `kamel run -n i1 -r file:test.txt` and `kamel run -n i2 -r file:test.txt` as the second would reuse the content of the previously created configmap 2. We are not able to detect easily a change in the content, as we need to inspect the content of the configmaps when `--sync` is enabled Using an hash solves both problems. > > I'm trying to understand if the consistency of the Integration configuration being created is still satisfied, meaning if user A create an integration with a local file resource, user B should still be able to operate the Integration. I hope that makes sense :) > > > > > > The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource? > > > > > > > > > > > > Good point, I did not consider this situation. The configmap is updated when there is a file change and `--sync/--dev` enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point. > > > > > > I made some test and with this implementation, the generated Configmap is kept until the Integration is living. We can p
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-999488606 > > Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this. > > > My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle > > > > > > The idea we discussed was to go in the direction to [remove any arbitrary file content from the integration](https://github.com/apache/camel-k/issues/2320#issuecomment-922718785). With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via `kamel` CLI to simplify his life. If the user is not able to run an Integration via `kamel run`, in fact, the feature cannot be provided. > > That is my background too. My question was more, why does the `file` prefix beyond the CLI, and the generated ConfigMap seems to be treated differently than the other "standard" ones? If I understood correctly, the question is about generated Configmap difference with user provided Configmap. The only difference is about the lifecycle of the Configmap itself, as my idea is to have it living together and exclusively within the Integration lifecycle. This is mimicking the previous behavior, when a Configmap was generated beside the Integration. The difference is that the resource is not directly stored in the Integration and the Configmap generated by the CLI (as it must access the file content). Another possibility would be to autogenerate the Configmap but let the lifecycle (update/delete) up to the user. However, I think it completely overlaps what we already provide with the normal configmap, just saving the manual step to create the configmap from the file resource manually. Hope it answer your question, I'm not entirely sure we're talking about the same thing here :) > > > > The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource? > > > > > > Good point, I did not consider this situation. The configmap is updated when there is a file change and `--sync/--dev` enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point. I made some test and with this implementation, the generated Configmap is kept until the Integration is living. We can provide an additional loop to reconcile the generated Configmap not needed right after the binding step. Or we can keep it the way it is, if we consider the patter to create an Integration with a file and later update the same integration without a file as a rare one. I don't have a strong opinion on this part. > > > What's the rational, from the functional standpoint, to use the container trait to host the configuration resources? It's already responsible for the compute resources, the container image, the service, and the (deprecated) health probes configuration. > > > > > > The container trait is already in charge to convert the configmaps/secret/... into mounted volumes ([we discussed it shortly as well](https://github.com/apache/camel-k/pull/2635#issuecomment-922720383)), so it felt the most appropriate trait to use. Otherwise we need somehow to move that logic elsewhere or duplicating it, which I don't think is a good idea. > > I'm trying to reason first from the end-user standpoint. The CLI hides it, still it's part of the API. I remember we had the discussion in #2635, and I was wondering whether that was the right moment to find a more "functionally oriented" trait to capture this concern, and disentangle the _container_ trait. The idea to move the _volumes_ logic into a separate trait could be interesting. However, we need to attach those volumes into the `corev1.Container`. If we create a new trait, then it will be very dependent on the container trait, requiring to be executed after it, and it will need to get the `corev1.Container` spec. After all, I think it is responsibility of the container trait to manage its own volumes. -- This is an automated message from the Apache Git Service. To re
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-998647128 Thanks a lot for the review @astefanutti , it's a long one, I really appreciate your help with this. > My understanding is that the implementation assumes the "local file" use case is different than the "standard" ConfigMap use case. It's possible you've already tried to explain the difference, though could you please clarify the rational behind that assumption. It seems that the file prefix / reference gets stored in the container trait configuration, and even if it's weakly linked to the local file, and resolved to the generated ConfigMap, that seems to break the "standalone" configuration principle The idea we discussed was to go in the direction to [remove any arbitrary file content from the integration](https://github.com/apache/camel-k/issues/2320#issuecomment-922718785). With this PR what we do is to automatically help the user convert a local file into a configmap, and then attach to the Integration. In my opinion what we're doing is to remove the support for local file, although we help the user to convert it in the supported storage via `kamel` CLI to simplify his life. If the user is not able to run an Integration via `kamel run`, in fact, the feature cannot be provided. > The generated ConfigMap is owned by the Integration, which makes the former gets garbage collected when the former is deleted. However, how is it garbage collected when the Integration is updated with the file resource removed, for example with a new kamel run invocation, without the resource? Good point, I did not consider this situation. The configmap is updated when there is a file change and `--sync/--dev` enabled, but I did not check what happens when the file is removed at all. I'll run some test and come back to this point. > What's the rational, from the functional standpoint, to use the container trait to host the configuration resources? It's already responsible for the compute resources, the container image, the service, and the (deprecated) health probes configuration. The container trait is already in charge to convert the configmaps/secret/... into mounted volumes ([we discussed it shortly as well](https://github.com/apache/camel-k/pull/2635#issuecomment-922720383)), so it felt the most appropriate trait to use. Otherwise we need somehow to move that logic elsewhere or duplicating it, which I don't think is a good idea. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-996749014 @astefanutti if you can find some time to review it, I'd appreciate it. The first part is the same you had a look some time ago, the rest of the commits implemented the different things planned originally. Thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-978997002 Thanks for the review @astefanutti, those are very good points to discuss. > A couple of points: > > * Would the ultimate place to reference these ConfigMaps be in a new _configuration_ trait, as discussed in #2320, maybe as a second iteration to this? That is partially correct. The idea is to move configmap/secret managment there as soon as this part would be completed. So, once autogenerated, the configmaps will be managed as any user provided configmap into the `configuration` trait. However, the generation part must be done in the CLI, because the trait is executed by the operator and won't be able to access the local file content. > > * Should the operator watch these owned ConfigMaps for changes, and reconcile the parent Integration? > > * If yes, as a sub-case, the Integration phase could be set to `Error` if a referenced ConfigMap got deleted by mistake. > * It relates to #1235 and #2106 possibly > I don't think this would apply to autogenerated configmaps. In my view, these kind of configmaps are just a support to the local file content. I've worked to support `sync` option and have an Integration redeploy when we detect a change in the local file, that will translate in a **new** configmap. However, in a production environment, I expect the user to provide "normal" configmaps if there is the possibility to change the underlying content (and in that case still apply the issues reported above). > * Are the generated ConfigMaps really immutable? They must be, IMO. The name is generated from a SHA based on the file content and nobody should alter them as they are thought as a representation of the local file. > > * Would that make sense to go the same direction for the `SourceSpec` field? Not sure if it makes sense. I think that the `SourceSpec` is a core part of the Integration, and we must allow anybody that want to provide an Integration with `kubectl` to do that without adding additional steps (ie, creating a configmap, then setting it in the Integration). A resource, instead, is already something complementary to the Integration. If we go in this direction, in fact, the user that creates directly Integration won't be allowed any longer to create a `ResourceSpec` with content and will need to create a configmap on his own. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2771: feat(cmd/run): autogenerated configmap for resource/config local files
squakez commented on pull request #2771: URL: https://github.com/apache/camel-k/pull/2771#issuecomment-976583410 > I'm not deep into the code at the moment to do a review but I'm +1 on the goal. What we have to think about is the backward compatibility so if we remove the ResourceSpec, we may need to bump APIs to v2 ? No problem, more than the code, the goal of the draft PR is to discuss about the feature. About bumping API version, I actually don't know which would be the recommended procedures to follow. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
