-
Notifications
You must be signed in to change notification settings - Fork 43
create a proposal for addon dependency management #157
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: elgnay The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // The default is false (soft dependency). | ||
| // +optional | ||
| // +kubebuilder:default=false | ||
| Required bool `json:"required,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boolean needs to be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced Required with Type, using explicit values "Optional" and "Required".
|
|
||
| - **Soft dependencies (default, required=false)**: The addon can still function with reduced functionality when the dependency is missing. When a soft dependency is not satisfied, the addon-manager will set `Degraded=True` with reason `DependencyNotSatisfied`, but the klusterlet-agent will not modify the `Available` condition, allowing the addon to remain available if its health checks pass. | ||
|
|
||
| - **Hard dependencies (required=true)**: The addon cannot function at all without the dependency. When a hard dependency is not satisfied, the addon-manager will set `Degraded=True` with reason `RequiredDependencyNotSatisfied`, and the klusterlet-agent will detect this specific reason and set `Available=False`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the addon will not be deployed under this case, like we should have a status that condition are or met, then addon-framework will deploy the resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the addon will not be deployed under this case
Should this work on a fresh install only, or rollout as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the addon is already installed and its dependent addon becomes unavailable, should the addon be uninstalled in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think so, but we should have a degrade condition with the message that some dependencies are missing. With the second thought, maybe we do not need to disable installation, a degraded status condition will be good.
03d652c to
8954e74
Compare
| // An empty list means the add-on has no dependencies. | ||
| // The default is an empty list. | ||
| // +optional | ||
| Dependencies []AddonDependency `json:"dependencies,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there is a case: for one addon, it has different dependencies on different type of managed clusters, for instance, on the OCP clusters it depends on the managed-serviceaccount addon, but on the eks clusters, it does not or even depends on other addons. I am asking because we have a similar case for the addon health checking recently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At present, we don’t have such a requirement, and we aim to keep the initial design as simple as possible. The case you mentioned is very interesting. In the future, the current API could be extended by adding a new field in AddonDependency — for example, when — that allows specifying a CEL expression to determine whether an addon actually depends on another.
|
/assign @jnpacker |
| ### Non-Goals | ||
|
|
||
| - Automatic installation of dependencies (users still need to explicitly install required addons) | ||
| - Dependency resolution and ordering during installation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this means if you have two dependencies set there is not ordering of the two dependencies. If you want ordering then you create a chain of dependent addons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that’s right. There’s no implicit ordering between multiple dependencies — creating a dependency chain is the way to enforce sequence.
|
|
||
| There are two types of dependencies: | ||
|
|
||
| - **Optional dependencies (default, type=Optional)**: The addon can still function with reduced functionality when the dependency is missing. When an optional dependency is not satisfied, the addon-manager will set `Degraded=True` with reason `DependencyNotSatisfied`, but the klusterlet-agent will not modify the `Available` condition, allowing the addon to remain available if its health checks pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any examples of this?
Will you publish some type of warning message in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any examples of this?
The work-manager is one of the MultiCluster Engine add-ons. Its pod log viewing feature depends on the managed-serviceaccount add-on. If that add-on is missing, other features—such as cluster view and cluster action—will still work.
Will you publish some type of warning message in this case?
Added an optional field, message, to capture this type of warning or informational message.
| description: "An addon that optionally uses ManagedServiceAccount API" | ||
| dependencies: | ||
| - name: managed-serviceaccount | ||
| # type: Optional is the default, can be omitted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to say that optional is default, but without any other type of cue, I would have expected it to be optional: false from just reading the keys. you have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Changed the default from Optional to Required.
| // condition should be set to False when the dependency is not satisfied. | ||
| // +optional | ||
| // +kubebuilder:validation:Enum=Optional;Required | ||
| // +kubebuilder:default=Optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I would have assumed it was optional by default.
Also do we need or want to consider any versioning semantics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I would have assumed it was optional by default.
Changed the default from Optional to Required.
Also do we need or want to consider any versioning semantics?
Since the ClusterManagementAddOn itself does not include any add-on version information, it’s not possible to define dependencies based on specific versions.
Signed-off-by: Yang Le <[email protected]>
8954e74 to
4624636
Compare
Related issue: #156