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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary KnativeServing code and finishing touches. #39

Merged
merged 3 commits into from
Apr 28, 2020
Merged

Remove unnecessary KnativeServing code and finishing touches. #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e012c1b
Remove unnecessary code from the serving reconciler.
markusthoemmes Apr 28, 2020
388a65e
Replace explit exits with Fatal.
markusthoemmes Apr 28, 2020
ed0f7ca
Align code.
markusthoemmes Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions pkg/reconciler/knativeeventing/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
injection.GetConfig(ctx),
mf.UseLogger(zapr.NewLogger(logger.Desugar()).WithName("manifestival")))
if err != nil {
logger.Error(err, "Error creating the Manifest for knative-eventing")
os.Exit(1)
logger.Fatalw("Error creating the Manifest for knative-eventing", zap.Error(err))
}

c := &Reconciler{
Expand Down
3 changes: 2 additions & 1 deletion pkg/reconciler/knativeeventing/knativeeventing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ type Reconciler struct {
kubeClientSet kubernetes.Interface
// kubeClientSet allows us to talk to the k8s for operator APIs
operatorClientSet clientset.Interface
config mf.Manifest
// config is the manifest of KnativeEventing
config mf.Manifest
// Platform-specific behavior to affect the transform
platform common.Platforms
}
Expand Down
20 changes: 8 additions & 12 deletions pkg/reconciler/knativeserving/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (

"go.uber.org/zap"

servingclient "knative.dev/operator/pkg/client/injection/client"
operatorclient "knative.dev/operator/pkg/client/injection/client"
kubeclient "knative.dev/pkg/client/injection/kube/client"
"knative.dev/pkg/injection"
"knative.dev/pkg/logging"
Expand Down Expand Up @@ -64,24 +64,20 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
logger.Fatal(err)
}

c := &Reconciler{
kubeClientSet: kubeClient,
knativeServingClientSet: servingclient.Get(ctx),
knativeServingLister: knativeServingInformer.Lister(),
servings: map[string]int64{},
platform: common.GetPlatforms(ctx),
}

koDataDir := os.Getenv("KO_DATA_PATH")
config, err := mfc.NewManifest(filepath.Join(koDataDir, "knative-serving/"),
injection.GetConfig(ctx),
mf.UseLogger(zapr.NewLogger(logger.Desugar()).WithName("manifestival")))
if err != nil {
logger.Error(err, "Error creating the Manifest for knative-serving")
os.Exit(1)
logger.Fatalw("Error creating the Manifest for knative-serving", zap.Error(err))
}

c.config = config
c := &Reconciler{
kubeClientSet: kubeClient,
operatorClientSet: operatorclient.Get(ctx),
platform: common.GetPlatforms(ctx),
config: config,
}
impl := knsreconciler.NewImpl(ctx, c)

logger.Info("Setting up event handlers")
Expand Down
83 changes: 10 additions & 73 deletions pkg/reconciler/knativeserving/knativeserving.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
mf "github.com/manifestival/manifestival"
clientset "knative.dev/operator/pkg/client/clientset/versioned"

"go.uber.org/zap"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -33,10 +32,8 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/cache"
servingv1alpha1 "knative.dev/operator/pkg/apis/operator/v1alpha1"
knsreconciler "knative.dev/operator/pkg/client/injection/reconciler/operator/v1alpha1/knativeserving"
listers "knative.dev/operator/pkg/client/listers/operator/v1alpha1"
"knative.dev/operator/pkg/reconciler/common"
ksc "knative.dev/operator/pkg/reconciler/knativeserving/common"
"knative.dev/operator/version"
Expand All @@ -58,13 +55,10 @@ var (
type Reconciler struct {
// kubeClientSet allows us to talk to the k8s for core APIs
kubeClientSet kubernetes.Interface
// knativeServingClientSet allows us to configure Serving objects
knativeServingClientSet clientset.Interface

// Listers index properties about resources
knativeServingLister listers.KnativeServingLister
config mf.Manifest
servings map[string]int64
// operatorClientSet allows us to configure operator objects
operatorClientSet clientset.Interface
// config is the manifest of KnativeServing
config mf.Manifest
// Platform-specific behavior to affect the transform
platform common.Platforms
}
Expand All @@ -77,17 +71,8 @@ var _ knsreconciler.Finalizer = (*Reconciler)(nil)
func (r *Reconciler) FinalizeKind(ctx context.Context, original *servingv1alpha1.KnativeServing) pkgreconciler.Event {
logger := logging.FromContext(ctx)

key, err := cache.MetaNamespaceKeyFunc(original)
if err != nil {
logger.Errorf("invalid resource key: %s", key)
return nil
}
if _, ok := r.servings[key]; ok {
delete(r.servings, key)
}

// List all KnativeServings to determine if cluster-scoped resources should be deleted.
kss, err := r.knativeServingClientSet.OperatorV1alpha1().KnativeServings("").List(metav1.ListOptions{})
kss, err := r.operatorClientSet.OperatorV1alpha1().KnativeServings("").List(metav1.ListOptions{})
if err != nil {
return fmt.Errorf("failed to list all KnativeServings: %w", err)
}
Expand Down Expand Up @@ -124,35 +109,13 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, original *servingv1alpha1

// ReconcileKind compares the actual state with the desired, and attempts to
// converge the two.
func (r *Reconciler) ReconcileKind(ctx context.Context, original *servingv1alpha1.KnativeServing) pkgreconciler.Event {
logger := logging.FromContext(ctx)

// Convert the namespace/name string into a distinct namespace and name
key, err := cache.MetaNamespaceKeyFunc(original)
if err != nil {
logger.Errorf("invalid resource key: %s", key)
return nil
}
r.servings[key] = original.Generation

// Reconcile this copy of the KnativeServing resource and then write back any status
// updates regardless of whether the reconciliation errored out.
err = r.reconcile(ctx, original)
if err != nil {
return err
}
return nil
}

func (r *Reconciler) reconcile(ctx context.Context, ks *servingv1alpha1.KnativeServing) error {
func (r *Reconciler) ReconcileKind(ctx context.Context, ks *servingv1alpha1.KnativeServing) pkgreconciler.Event {
logger := logging.FromContext(ctx)
ks.Status.InitializeConditions()
Copy link
Contributor

Choose a reason for hiding this comment

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

You sure we want to do this all the time? It's not going to overwrite anything important?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, serving's controllers do the same thing.


reqLogger := logger.With(zap.String("Request.Namespace", ks.Namespace)).With("Request.Name", ks.Name)
reqLogger.Infow("Reconciling KnativeServing", "status", ks.Status)

logger.Infow("Reconciling KnativeServing", "status", ks.Status)
stages := []func(context.Context, *mf.Manifest, *servingv1alpha1.KnativeServing) error{
r.ensureFinalizerRemoval,
r.initStatus,
r.install,
r.checkDeployments,
r.deleteObsoleteResources,
Expand All @@ -169,7 +132,7 @@ func (r *Reconciler) reconcile(ctx context.Context, ks *servingv1alpha1.KnativeS
return err
}
}
reqLogger.Infow("Reconcile stages complete", "status", ks.Status)
logger.Infow("Reconcile stages complete", "status", ks.Status)
return nil
}

Expand Down Expand Up @@ -197,32 +160,6 @@ func (r *Reconciler) transform(ctx context.Context, instance *servingv1alpha1.Kn
return r.config.Transform(transforms...)
}

// Update the status subresource
func (r *Reconciler) updateStatus(instance *servingv1alpha1.KnativeServing) error {
afterUpdate, err := r.knativeServingClientSet.OperatorV1alpha1().KnativeServings(instance.Namespace).UpdateStatus(instance)

if err != nil {
return err
}
// TODO: We shouldn't rely on mutability and return the updated entities from functions instead.
afterUpdate.DeepCopyInto(instance)
return nil
}

// Initialize status conditions
func (r *Reconciler) initStatus(ctx context.Context, _ *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
logger := logging.FromContext(ctx)

logger.Debug("Initializing status")
if len(instance.Status.Conditions) == 0 {
instance.Status.InitializeConditions()
if err := r.updateStatus(instance); err != nil {
return err
}
}
return nil
}

// Apply the manifest resources
func (r *Reconciler) install(ctx context.Context, manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
logger := logging.FromContext(ctx)
Expand Down Expand Up @@ -303,7 +240,7 @@ func (r *Reconciler) ensureFinalizerRemoval(_ context.Context, _ *mf.Manifest, i
return fmt.Errorf("failed to construct finalizer patch: %w", err)
}

patcher := r.knativeServingClientSet.OperatorV1alpha1().KnativeServings(instance.Namespace)
patcher := r.operatorClientSet.OperatorV1alpha1().KnativeServings(instance.Namespace)
if _, err := patcher.Patch(instance.Name, types.MergePatchType, patch); err != nil {
return fmt.Errorf("failed to patch finalizer away: %w", err)
}
Expand Down