Skip to content
This repository has been archived by the owner on Jun 24, 2020. It is now read-only.

Remove all the deployments in the older version #257

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
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
26 changes: 26 additions & 0 deletions pkg/reconciler/knativeserving/knativeserving_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const (
creationChange = "creation"
editChange = "edit"
deletionChange = "deletion"
selectorKey = "serving.knative.dev/release"
)

var (
Expand All @@ -59,6 +60,7 @@ type Reconciler struct {
knativeServingLister listers.KnativeServingLister
config mf.Manifest
servings map[string]int64
oldVersion string
}

// Check that our Reconciler implements controller.Reconciler
Expand Down Expand Up @@ -106,6 +108,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
}
r.servings[key] = newGen

// Save the version of the original CR
r.oldVersion = original.Status.Version

// Don't modify the informers copy.
knativeServing := original.DeepCopy()

Expand Down Expand Up @@ -295,5 +300,26 @@ func (r *Reconciler) deleteObsoleteResources(manifest *mf.Manifest, instance *se
if err := manifest.Delete(resource, &metav1.DeleteOptions{}); err != nil {
return err
}

// If there used to be a CR at an older version, we need to remove the old resources with
// the label serving.knative.dev/release="<oldVersion>"
if r.oldVersion != "" && r.oldVersion != version.Version {
selector := fmt.Sprintf("%s=%s", selectorKey, r.oldVersion)
listOptions := metav1.ListOptions{ LabelSelector: selector }

// Remove all the deployments at the old version
deploymentList, err := r.KubeClientSet.AppsV1().Deployments(instance.GetNamespace()).List(listOptions)
if err == nil {
for _, dep := range deploymentList.Items {
if e := r.KubeClientSet.AppsV1().Deployments(instance.GetNamespace()).Delete(dep.Name,
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me nervous. Deleting a deployment instead of updating it introduces the potential for losing requests, I think. Besides, I prefer seeing in this function exactly which resources are obsolete and need to be deleted.

Copy link
Author

Choose a reason for hiding this comment

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

@jcrossley3 To find out all the resources to be removed, I can think of using the manifestival package to read the manifest of the older version, and compare to the manifest of the new version, to see if they still exist in the new version. Do you have any other suggestions?

&metav1.DeleteOptions{}); e != nil {
return e
}
}
} else if !apierrs.IsNotFound(err) {
return err
}
}

return nil
}