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

Add Proposal for dependency resolution in kc #732

Merged
merged 2 commits into from
May 11, 2024

Conversation

praveenrewar
Copy link
Member

No description provided.

Copy link

netlify bot commented Mar 4, 2024

Deploy Preview for carvel ready!

Name Link
🔨 Latest commit 3f18788
🔍 Latest deploy log https://app.netlify.com/sites/carvel/deploys/663e61d55a914300088b4797
😎 Deploy Preview https://deploy-preview-732--carvel.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@praveenrewar praveenrewar changed the title WIP: Add Proposal for dependency resolution in kc Add Proposal for dependency resolution in kc Mar 5, 2024
Copy link

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

@praveenrewar Thanks for the proposal. This is a very useful feature to have in kapp-controller. Just a few more questions:

  1. Is the proposal intended to cover the design of the resolver itself? (In OLM we do use a SAT solver, so just curious if there is some investigation in progress to figure out the options).
  2. Do users creating package install have the option of doing a force install?
  3. Given the dependent package installs are exposed to the user, what happens when it is deleted directly? In the sense, do we have watches set up on dependent packages and how frequently does the resolution occur?

@praveenrewar
Copy link
Member Author

@varshaprasad96

Is the proposal intended to cover the design of the resolver itself?

We currently use a resolver based on the semver constraints in the package install api to resolve the versions, we are planning to re-use it for dependency resolution. (Does this answer your question?)

Do users creating package install have the option of doing a force install?

Do you mean if users can install packages without installing the dependencies? Then, yes, users can set the spec.dependencies.managedBy field to User (default is Auto). However, kc would try to validate if the dependent packages (not package installs) are present on the cluster or not.

Given the dependent package installs are exposed to the user, what happens when it is deleted directly? In the sense, do we have watches set up on dependent packages and how frequently does the resolution occur?

In the first few iterations at least, we won't have an watches set for the dependencies, but we could think about it in future enhancements. This is a good point, I will add it to the list of future enhancements. Thanks!

@vrabbi
Copy link
Contributor

vrabbi commented Mar 6, 2024

comments/questions:

The doc contradicts itself. In the crd spec you have User as the default mode but in the example the comment says the default will be Auto in terms of dependency installation. Which is the approach you are planning on implementing?

What if packages are not in the same namespace? Does this require them to all be in the same namespace and or be in the global namespace?

Do all dependencies use the same service account as the main package install or is a service account able to be specified for different dependencies?

How can overlays be applied to dependant packages? Would this require manual dependency installation?

Does this work for targetting a different cluster as well or only for in cluster package installations?

Would canceling/pausing the main packagr install have an impact and cascade to dependencies or not?

Are changes made manually to auto installed dependent package installs reverted at next reconcile? Can one manually pause/cancel reconcilliation of a dependency?

Is the sync period of the dependencies copied from the main app or always set to the default value?

The doc says that when a package already is installed but not of the version which is needed for the new package install, kapp controller will try to install the new version, does this mean it will try to update the version of the existing package install or create another one? If its upgrading the existing one, how does kapp controller know the newer version is safe for the other root packages depending on the original dependency version? If it tries to install another pkgi side by side, what if the install type is once per cluster or once per namespace?

The spec of the package CRD is not inline with the example. Thr example has a packageRef section with version constraints which is great whereas the CRD spec has a version field only under a package key which seems to not support version ranges and constraints. Which approach is planned to be implemented?

@praveenrewar
Copy link
Member Author

Thanks a lot @vrabbi!! These are some really great questions! I will answer them inline here and then try to move them to the proposal (either update it or add the question in the "Answered questions sections).

In the crd spec you have User as the default mode but in the example the comment says the default will be Auto in terms of dependency installation.

Good catch! Auto will be the default value. I have updated it in the proposal.

What if packages are not in the same namespace? Does this require them to all be in the same namespace and or be in the global namespace?

For now the packages should be in the same namespace, in future we can think about expanding the search to the whole cluster, this will be important for packages which have installationType as OncePerCluster.

Do all dependencies use the same service account as the main package install or is a service account able to be specified for different dependencies?

Yes, for now, the dependencies will be using the same service account. In future, we can think of alternatives where a different SA is used for each dependency.

How can overlays be applied to dependant packages? Would this require manual dependency installation?

I feel that in this case we reach a stage where we are trying to control a lot of stuff on the dependency so should be manually installing the dependency, but if you think that we should allow adding overlays for dependencies directly, then we can think about having something similar to the data values for the overlays as well.

Does this work for targetting a different cluster as well or only for in cluster package installations?

In theory, it should work when you are targeting another cluster. You should have the dependency packages available on the cluster you are creating the current package install, and then if kapp-controller tries to use the same kubeconfig secret for installing the dependency packages on the target cluster, it should work.

Would canceling/pausing the main packagr install have an impact and cascade to dependencies or not?

There won't be any cascading of the paused field. Do you think we should pause the dependencies as well when the original pkgi is paused?

Are changes made manually to auto installed dependent package installs reverted at next reconcile? Can one manually pause/cancel reconcilliation of a dependency?

Yes, the changes would be reverted on the next reconciliation. We can pause both the original pkgi and the dependency pkgi if we want.

If it tries to install another pkgi side by side, what if the install type is once per cluster or once per namespace?

For this initial iterations, kapp-controller will fail when a certain version of a dependency is installed which doesn't satisfy the constraints provided by our package (one of the conditions in the example covers this). In future versions, we will try to install another version of the package if the installationType is NoRestrictions, if it is OncePerCluster or OncePerNamespace then we will fail with the same condition.

The spec of the package CRD is not inline with the example

Thanks. I have updated it. We will use packageRef.

@vrabbi
Copy link
Contributor

vrabbi commented Mar 6, 2024

Thanks for the detailed response.

I think overlays are important and in many cases are needed so having that option makes sense to me and i have a few use cases where this would make a lot of sense.

In terms of pausing dependencies i think its fine that it doesnt pause them it just needs to be clear in the docs/proposal. In an ideal world i could see value in having a paused field under each dependency in the main package that would be how to pause a dependency. This is better than pausing the dependency directly as all management of the dependency is through a single resource vs it being from multiple sources. Also it would work better for gitops solutions where only the main package is controlled by git and as such would allow for more advanced use cases. If this were done, i would expect changes done directly on the dependencies including setting the paused field to be overriden as the source of truth is the main packageinstall

Another new question is how could one "orphan" a dependency that was auto installed so it could be managed seperately. I can think of many cases where initially the user wants to manage a dependency simply via thr high level package but times may come when they want to manage it seperately as they have more specific use cases they want to have configurable. In this case i think that breaking the ownership would be a needed ability for users.

@praveenrewar
Copy link
Member Author

Thanks @vrabbi for the thoughts.

We can include overlays for dependencies as well, maybe in future iterations as we are evaluating if we should have a overlays as a first class api in the package install instead of annotations.

Makes sense regarding not pausing the dependencies, I will update the proposal to make it more clear.
I like the gitops idea, I will try to capture it in the future explorations section. It sounds good to control the dependencies from one source, we will have to think more about the scenarios where there the dependencies are also shared by other packages.

Another new question is how could one "orphan" a dependency that was auto installed so it could be managed seperately.

Hmm, definitely an interesting use case. I am thinking that from the current implementation perspective, removing the owner annotation manually from the dependency would be one way, although I think it would be useful to have a way to de-link the dependency from the owner pkgi. I will add this in the future explorations section as well.

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

I think we are in a good place with this proposal. We can remove the Open Question since it is already answered.
Let us also try to close all the open conversations, and if no outstanding comments are added I think we can mark it as approved somewhere middle next week.

Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com>
@praveenrewar
Copy link
Member Author

@cppforlife I have addressed your comments. Please give it another look when you get a chance.

@joaopapereira
Copy link
Member

@praveenrewar, please create a new commit with the change of status to approved since we have already passed the consensus period.

Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com>
@praveenrewar
Copy link
Member Author

@joaopapereira Done!

@joaopapereira joaopapereira merged commit 393a7d5 into develop May 11, 2024
8 checks passed
@joaopapereira joaopapereira deleted the kc-dependency-res-proposal branch May 11, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants