-
Notifications
You must be signed in to change notification settings - Fork 0
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
Helmcharts not easy to consume, missing logic in processing one of Localization, Configuration or Resource #68
Helmcharts not easy to consume, missing logic in processing one of Localization, Configuration or Resource #68
Comments
Hello. :) Resource is a standalone thing. I'll have to read this again to figure out what you are looking for. Please include the setup you are trying to attain / test in yaml format. You don't need the Resource object for Helm Charts. You should be able to define the resource for Configuration or Locatlization. The Resource is only used if you don't want to do any of the two others ( Loc or Conf ). |
Hey @Skarlso I have attached component-and-config.zip which contains :
These should be the file versions I was using when I first ran into problems. Obviously with these files I was hoping my helm chart would be installed. However additional goals were
The attached almost worked. Using crane I could see that localization and configuration worked and a helm repo and helm release were created. However helm was unable to use the output from the localization because it requires a chart be in a namespace repo. This work around was put in place to make helm happy by adding a namespace to the repo in the case that an extra 'helm' identify has been added to the resource. However as I mentioned in the description above the only way to trigger this work around is to use a Resource. And as stated above, even if you do this you still don't get something that works for the other reasons I mentioned in the initial description. |
Hm, can you provide something that I can use to reproduce your problem locally? |
From the component it looks like your chart resource isn't a chart resource. It's just a tarred up folder. You need to define a |
Hey @Skarlso
I think it should be pretty easy to the scenario we want to have work. This by either
|
Localization already has an output snapshot. 🤔 There is no point on having a Resource after localization. I'm still not sure what you are trying to accomplish. If the resource is not marked as a helm chart it will not be handled as a helm chart. We can't detect if it's a helm chart otherwise. |
I'm not sure I follow this. Can you elaborate what your mean by separate repo? And you "don't want that"? |
For reference, here is a working helm chart example https://github.com/Skarlso/ocm-helm I'll analyze the rest of the issue later. :) Not sure about that latest tag thing. :D |
In the attached image I have circled in red the extra repo. |
Hey @Skarlso Btw, in my case Localization and Configuration are updating the values.yaml of the helm chart. Not sure if that is clear with the data I provided so far. As stated in the bug description, to get something to work for my PoC I followed the ocm-setup-demo. Of course that deviates from our use case quite a bit so isn't something we would actually adopt. |
ocm-setup-demo was waaaay before we started supporting helm chart so that "cheats" a bit by doing some manual things with the charts and preloading them. :) uh, it used to work. 🤔 but we changed to cue lang for values. So you're probably right that the values file things no longer works. We ran out of time to update all samples sadly. |
I'll try testing helm access once I get to this issue. :) The first intended case should have worked 🤔 Not sure why Thanks for reporting it. Helm chart support is very much an alpha feature. I'm sure it has some hiccups especially since the requirement for named repositories. |
Okay so, the helm chart downloading and creating a snapshot that is a proper helm chart repo with resource is working. However... The configuration and localization functionality for helm charts is missing because it doesn't re-package it properly and creates a new helm chart out of it after configuring or updating a values.yaml file. This is a missing feature, basically. We didn't quite have the time to finish it. The Which creates the following snapshot value: Status:
Conditions:
Last Transition Time: 2024-03-26T12:01:45Z
Message: Snapshot with name 'ocm-with-helm-deployment-mw73dth' is ready
Observed Generation: 1
Reason: Succeeded
Status: True
Type: Ready
Digest: sha256:31b957024d57c2b4768c2a5ff9004fe0acdbbcf6974167fa0a95d858c21ea985
Observed Generation: 1
Repository URL: https://registry.ocm-system.svc.cluster.local:5000/sha-785370814667106374/podinfo
Tag: 6.3.5
|
@Skarlso and @dee0 , the current way to do configuration and localization for me feels a bit to cumbersome. Thinking of people that want to deploy their Helm Chart, but understood that it makes sense to model their product in OCM should have a very simple possibility to configure and localize their application, similar to how they would by modifying the values.yaml, to specify their intent. If in the background we require CRs with a complex structure and content that establish the intent in the system, then we at least should check if it is possible to at least offer a more simple interface at the surface. Any ideas to solve this Gordian knot are welcome :-) |
That is not going to be an easy task. :/ But I think it's gonna be fine maybe. It's just the re-packaging that's missing. A user would have a separate resource that contains configuration values perhaps, and we could merge that with existing ones, I don't know. Another idea was to basically allow people to describe how to configure something by having the WASM scripts in a resource that would do the configuration for us. They are tiny binaries after all and we could just run them as is and it would configure the requested resource. That's also an option. |
WASM was and still is an interesting idea for the configuration and also still a hot topic out there. I would even buy something super simple for the bread and butter cases and something more complex for the other cases, but I really think that adoption will not raise when we keep the interface as complex as it is. |
That might be true, yeah. I have to do some brainstorming with myself and see if I can come up with something. Neverthless, a pluggable infrastructure is still the way to go I believe. It has been wildly successful in other areas after all. |
Hey @Skarlso & @morri-son Want to restate the use case in our division and then present my naive thoughts on supporting them. Use case :
From my naive user perspective, what is missing to support the above 'nicely' is:
And with these mods I could have the simple 'pipeline' of That said, even a change as simple as what is in this PR would make it so my use case is mostly supported. The only catches would be
Fwiw, I was hoping to step through the code to see how hard the naive solution above would be to implement. This is why I left this PR in the 'work in progress' state. Didn't find the time to do it today but maybe I'll get to it this week sometime. |
Hello, let me try to clarify some things.
I assume you are talking about our local registry? That is not something the service or the infrastructure team would be aware off at all. That registry is ocm-controller's local cache and an integration point with flux itself. The FluxDeployer creates Flux components to be able to deploy... with Flux. :) We are aiming to create other deployers like ArgoDeployer or something that can provide other kind of options, but that's in the future. For now, all you would need to be concerned about is defining the properties for a helm chart.
The extra-identity is used by us. OCM the library isn't aware of such a thing. So specifying it there, does nothing. That's why you are getting that error.
The reason it doesn't work today is because this feature is incomplete as of now. Like I detailed in my comment, the Resource can produce a valid helm chart, but the others can't yet. :) Once it's implemented your scenario should work nicely. 🤞 😄
The version is coming from here: components:
- name: github.com/open-component-model/helm-test
version: "v1.0.0"
provider:
name: ocm.software
resources:
- name: charts
type: helmChart
version: 6.3.5
input:
type: helm
version: 6.3.5
path: charts/podinfo-6.3.5.tgz So technically you can define whatever version you like when building the helm component using OCM the library. Then, the resource uses that version as well when it's being reconciled: apiVersion: delivery.ocm.software/v1alpha1
kind: Resource
metadata:
name: ocm-with-helm-deployment
namespace: ocm-system
spec:
interval: 10m
sourceRef:
kind: ComponentVersion
name: ocm-with-helm
namespace: ocm-system
resourceRef:
name: charts
version: 6.3.5
extraIdentity:
helmChart: podinfo
Hopefully, once everything is implemented, the Resource part could simply be left off. 🤞 |
Hey @Skarlso First, let me say 'thank you' for taking the time to respond and having the patience deal with me. :) "I assume you are talking about our local registry? " Nope. I explained this above. Please see the screenshot above of artifactory. I circled the extra repository that was created and I explained why this is a problem. "The extra-identity is used by us. OCM the library isn't aware of such a thing. So specifying it there, does nothing. That's why you are getting that error." You must be able to see this makes zero sense from my perspective, yes? Consider what I am seeing as an outsider looking in
Regarding this example
it is problematic for these reasons I mentioned above :
So again, from perspective, this should be an easy fix that doesn't break the model
Then in my component descriptor yaml I can put
and everything will 'just work' :) Btw, the above is giving me the following in my componentdescripto, note the extraIdentity and version. So the information needed to do the right thing is there, just Localize and Configuration need to use it
|
Okay, so I think we have to make something clear. The controller is using OCM LIBRARY. Whatever the CLI does is the CLI's thing. :) The controller is using the Library. So whatever the CLI does is not necessarily something the controller does. The controller has different goals and purposes. This means that your component description doesn't need the extra identity the controller needs. Here: https://github.com/open-component-model/ocm-controller?tab=readme-ov-file#helmchart-type-resource it's explained that the controller is using this because it cannot be inferred. This is the only reason for it. I could extend this of course if it doesn't make any sense. :) Sorry, I might be dense, but where / what is creating that extra repo?
But why? So like I wrote, for now, configuration and localization isn't working with charts yet. I need to add that functionality. Resource though works with charts as long as OCM can fetch them. Whatever it creates internally, is not a concern of the user. |
For completeness sake, the FluxDeployer above was from a helm template. Here is the rendered result
|
Hey @Skarlso |
Gonna need some more info about your CRs. Or logs or something. Otherwise you're on your own. |
Logs don't show really show anything more than what is in that status message. Each resource type is in its own *.yml file. Fwiw this is what our testing infrastructure is spitting out at the end of a failed test run. Fwiw, this was working until the upgrade. |
This: resourceRef:
extraIdentity:
helmChart: dmi-broker
is no longer correct. This is also no longer correct for the flux deployer: helmReleaseTemplate:
chart:
spec:
chart: dmi-broker Please check out the repository I linked. There is a working example for both helm chart without and with configuration. :) |
Hey @Skarlso Thanks, but I think your sample may be broken and I don't think it is representative for my case. I think it is broken because the Localization appears to refer to a non-existent Resource. It refers to As for why I don't think the samples are representative of my case there are these discrepancies
Please provide some guidance on how to make this work with whatever change you made to the ocm-controller. |
Ahhh sorry. I should have said that only configuration was altered to be working. The localization wasn't touched. So sorry Dan. |
I'll try to whip up a localization for you that's working. But generally I think it should once you alter it the same way configuration does it. Only the location of the image and tag needs to follow the value.yaml's format. |
Thanks @Skarlso Btw, the things I am concerned about are
Also is FluxDeployer able to work with a helm chart that is stored as a dir resource with in the component or must the type be helm chart? This is what I have been using so far ( successfully given the other files attached ) .
|
Hum. That shouldn't matter I think. Dir should work. The requirement is there because it will try and produce a snapshot that the helm controller than needs to understand. And we do that previously already but that information isn't taken any further. Unless the resource is the same version as the chart than that extra info is not needed. |
Hey @Skarlso just want to confirm what I think you said... In my component description if the resource version matches the helm chart version then I don't have to specify a version in the Resource or the FluxDeployer. So for example, in my component yaml if I have
then I won't need to specify a chart version in the Resource or the FluxDeployer. Do I have that correct? |
Yeah, that should be right. As long as the tag in the snapshot matches the chart, Flux's helm controller shouldn't complain. 🤞 |
Nope, something went wrong.
I have no idea where 19090 is coming from. Looking at the component descriptor with
I see
|
|
Hey @Skarlso I think The error I mentioned above was after changing my POC to look like Yesterday I grabbed the latest code with the intent of putting in the place the logic I have expected from the beginning. ( Search for 'festooned' in this description ) However before changing anything, while stepping through the debugger I hit a new problem 🔢
ResourceReconciler:reconcile gets this when it is trying to get the resource. This is happening because cv.Status.ReplicatedRepositoryURL is empty string. My ComponentVersion is
Fwiw, think I will rebase my branch onto 0.23.7 to avoid this problem. |
I'm on vacation for a while, would you mind putting this into a separate issue please? Thanks. |
Was |
Ah yes. I'll think of something to mitigate this. |
This issue was marked as stale because it has not had recent activity. |
Not stale |
## Description Fix for issue [ #68](open-component-model/ocm-project#68) in ocm-project. At least as of 0.23.7 there was primary problem and a follow on problem that appeared after the first was fixed. My setup was like this ![image](https://github.com/user-attachments/assets/c6015cee-5c88-4644-915e-852e0b94bc15) With the setup and v0.23.7 the first problem was I would get an error like this `ocm-system dmi-broker 24m False artifact revision 19090 does not match chart version 1.3.0-1085.xeae3fba.14` This because snapshot writer is using the resourceversion of of the kubernetes object that owns the snapshot. Given my setup, 19090 happened to be the resourceversion of my Configuration. As I mentioned when I opened [#68](open-component-model/ocm-project#68), at least for helmcharts this is silly as there are several spots that are better sources for a version tag. So I have fixed this storying the resource version, taken from the component descriptor, in the identity. I am only doing this if there isn't already a value stored under ResourceVersionKey. Fixing this revealed a problem when you have more than 1 mutation, in my case a Localization and a Configuration, getting their config a resource. The mutation objects get their identity from the resource they reference in `configRef`. Both of my reference the same config.yaml in my components. The identity is used to compute the snapshot's OCI repository name. Prior to my above tag fix being in place, the Localization and Configuration were both sticking their snapshots into the same repository. Since I was 'lucky' and the two objects never had the same resourceversion the tags were used on the images stored in the repository were different and I never noticed a problem. However once I put the above fix in place I started seeing sporadic failures. In fact, due to issue #224, I would see my HelmRelease go back and forth between being unhealthy and healthy ( but mostly unhealthy ). It would be unhealthy if/when the FluxDeployer grabbed the data from the snapshot image when the Localization was the last one to write the snapshot image. To fix this added I 'mutation-object-uuid' to the identity of resources that are output from the mutations ( Localization, Configuration ). This way each will have its own snapshot repo and they will no longer overwrite each other's images when the same resource is referenced in `configRef`. ## What type of PR is this? (check all applicable) - [ ] 🍕 Feature - [x ] 🐛 Bug Fix - [ ] 📝 Documentation Update - [ ] 🎨 Style - [ ] 🧑💻 Code Refactor - [ ] 🔥 Performance Improvements - [ ] ✅ Test - [ ] 🤖 Build - [ ] 🔁 CI - [ ] 📦 Chore (Release) - [ ] ⏩ Revert ## Related Tickets & Documents <!-- Please use this format link issue numbers: Fixes #123 https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword --> - Fixes [68](open-component-model/ocm-project#68) ## Added tests? - [ ] 👍 yes - [ ] 🙅 no, because they aren't needed - [ ] 🙋 no, because I need help - [ ] Separate ticket for tests # (issue/pr) As of 2024-07-11 I have just used the fixed build in my POC and confirmed that my components are being deployed. If/when I get time I'll try to update the test suite. ## Added to documentation? - [ ] 📜 README.md - [ ] 🙅 no documentation needed ## Checklist: - [x ] My code follows the style guidelines of this project - [ x] I have performed a self-review of my code - [ x] I have commented my code, particularly in hard-to-understand areas - [ x] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ x] Any dependent changes have been merged and published in downstream modules --------- Co-authored-by: Gergely Brautigam <[email protected]>
I would like to have objects 'feeding' each other like so
ComponentVersion -> Localization -> Configuration -> FluxDeployer
Where
This doesn't work because Flux/Helm require that helm charts be in a namespaced repo instead of the being at the root of the registry.
The helm-controller complains about a 404 due to the path it is looking for not existing.
Currently this can be worked around by changing the above to be
ComponentVersion -> Localization -> Configuration -> Resource -> FluxDeployer
where Resource has
extraIdentity:
helmChart: my-helm-chart-name
So the first question, why can't placement of extraIdentity on Configuration have the desired effect?
Second problem is, even after introducing a Resource as described above the deploy fails because the only tag in the repo is 'latest'.
Helm complains that no tags were found in the repo.
This is because Flux/helm require that the tags match a strict semver check and 'latest' doesn't pass this. I don't understand why the tag is 'latest' when my ComponentVersion is festooned with version info. Why can't one of those version numbers be utilized?
You can hack around this by adding a version spec on the resource like so
sourceRef:
kind: Configuration
name: *name
resourceRef:
name: chart
version: "1.0.37"
Of course this isn't maintainable so this isn't a solution that one can live with.
And in any case, even adding the version still doesn't give you something that actually is usable. This because the Resource is picking up the helm chart from the ComponentVersion instad of the Snapshot created from the Configuration. Comparing resource_controller.go and localization_controller.go ( see the findObjects func ) provides an idea of why Resource is not producing the desired results.
I can try and work around all of this by adding
Of course the HelmRelease would have a values section that would receive the config updates.
This is what the ocm demo here is doing. However the problem with this approach is that I am now assuming that ops is using Flux for deploying my service. I can accept the assumption that helm and ocm are in use but assuming Flux is too much.
Anyhoo as it stands at the processing of at least of the types Localization, Configuration or Resource needs to be changed in order to make it easier to consume helm charts as part of an ocm component.
I'll attach my component.yaml and the manifest file with the ComponentVersion etc.
The text was updated successfully, but these errors were encountered: