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

refactor: use main event handler and defer it if Knative CRDs are not found #276

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Jan 21, 2025

The Knative Serving and Eventing charms have a strong dependency on knative-operator, as it is the application that applies the KnativeServing and KnativeEventing CRDs and has the business logic to reconcile such CRs. Because the Knative Serving and Eventing charms depend entirely on the existence of those CRDs, this commit refactors the charms logic to:

  1. Use a main event handler that can be deferred in case the required CRDs are not present
  2. Remove any operation for the on_install event, and use the main event handler for config_changed as it is guaranteed that it'll be triggered right after on_install

Fixes #156

Manual testing

  1. Deploy knative-{serving|eventing}
  2. Depending on which charm, you may need to run some configs, for example, for serving:
juju config knative-serving namespace="knative-serving"
  1. Observe that in the absence of knative-operator, the charm goes to waiting status:
knative-serving/0*       waiting   idle   10.1.205.36         Waiting for knative-operator CRDs to be present.
  1. Deploy the knative-operator charm
juju deploy knative-operator --channel 1.12/stable --trust
  1. (optional) If you want to speed up things, you can update the hook interval
juju model-config update-status-hook-interval=5s
  1. Observe that the message goes away and the error is not present any more
knative-operator/0*      active    idle   10.1.205.35
knative-serving/0*       active    idle   10.1.205.36

… found

The Knative Serving and Eventing charms have a strong dependency on knative-operator,
as it is the application that applies the KnativeServing and KnativeEventing CRDs and
has the business logic to reconcile such CRs. Because the Knative Serving and Eventing
charms depend entirely on the existence of those CRDs, this commit refactors the charms
logic to:
1. Use a main event handler that can be deferred in case the required CRDs are not present
2. Remove any operation for the on_install event, and use the main event handler for config_changed
as it is guaranteed that it'll be triggered right after on_install

Fixes #156
@DnPlas DnPlas force-pushed the KF-6270-check-for-crds-before-applying branch from a9b5ae0 to 2c527ed Compare January 21, 2025 06:19
@DnPlas DnPlas marked this pull request as ready for review January 21, 2025 06:19
Copy link
Contributor

@mvlassis mvlassis left a comment

Choose a reason for hiding this comment

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

Minor comments, great work as always!

@DnPlas DnPlas merged commit 141feb7 into main Jan 24, 2025
23 checks passed
@DnPlas DnPlas deleted the KF-6270-check-for-crds-before-applying branch January 24, 2025 13:55
DnPlas added a commit that referenced this pull request Jan 28, 2025
… found (#276)

* refactor: use main event handler and defer it if Knative CRDs are not found

The Knative Serving and Eventing charms have a strong dependency on knative-operator,
as it is the application that applies the KnativeServing and KnativeEventing CRDs and
has the business logic to reconcile such CRs. Because the Knative Serving and Eventing
charms depend entirely on the existence of those CRDs, this commit refactors the charms
logic to:
1. Use a main event handler that can be deferred in case the required CRDs are not present
2. Remove any operation for the on_install event, and use the main event handler for config_changed
as it is guaranteed that it'll be triggered right after on_install

Fixes #156
DnPlas added a commit that referenced this pull request Jan 29, 2025
…rving

refactor: use main event handler and defer it if Knative CRDs are not found (#276)
fix: fix the name of CRD that knative-eventing looks for (#281)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

knative-serving error applying manifests when knative-operator is not done installing
2 participants