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

Update to latest manifestival to apply strategic merge patches to its resources. #243

Merged
merged 2 commits into from
Dec 10, 2019
Merged

Update to latest manifestival to apply strategic merge patches to its resources. #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
515b55c
Pull in latest manifestival client-go branch
jcrossley3 Dec 6, 2019
dc4f282
Eliminate superfluous updates on Deployments and Images among others
jcrossley3 Dec 6, 2019
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
12 changes: 8 additions & 4 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/reconciler/knativeserving/common/certs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"go.uber.org/zap"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/kubernetes/scheme"
servingv1alpha1 "knative.dev/serving-operator/pkg/apis/serving/v1alpha1"
Expand Down Expand Up @@ -54,6 +55,8 @@ func CustomCertsTransform(instance *servingv1alpha1.KnativeServing, log *zap.Sug
if err := scheme.Scheme.Convert(deployment, u, nil); err != nil {
return err
}
// Avoid superfluous updates from converted zero defaults
u.SetCreationTimestamp(metav1.Time{})
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious if these would now also be fixed due to the three-way-merge semantics? I.e., do we need to manually set them still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're relying on this logic to actually remove the field. Without it, we attempt to apply the Time's zero-value set by the typed object.

Copy link
Contributor

Choose a reason for hiding this comment

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

The need for this seems like it could be non-obvious for those implementing new transforms. Would it make sense to move logic which ignores creation timestamp (or something similar) to a location that allows it to be re-used (manifestival, or somewhere else in this package)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I view it as a tradeoff for being able to treat any type in the manifest as Unstructured. I would prefer not to special-case any fields at this time, if we can avoid it.

And though I share your concern about the non-obviousness, keep these in mind:

  • It's only a problem if you convert the unstructured to a structured type and back again. Most transforms (currently) don't do this.
  • The only risk to not clearing the zeroed fields is executing unnecessary updates; annoying and slow, but logically harmless.
  • It will be very obvious to anyone looking at the log that unnecessary updates are occurring because we now log the diff showing why we're updating any resource. I would expect this to trigger the transform author, so it may be less non-obvious than we think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Thanks.

}
return nil
}
Expand Down
8 changes: 8 additions & 0 deletions pkg/reconciler/knativeserving/common/images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"go.uber.org/zap"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/client-go/kubernetes/scheme"
caching "knative.dev/caching/pkg/apis/caching/v1alpha1"
Expand Down Expand Up @@ -75,6 +76,9 @@ func updateDeployment(instance *servingv1alpha1.KnativeServing, u *unstructured.
if err != nil {
return err
}
// The zero-value timestamp defaulted by the conversion causes
// superfluous updates
u.SetCreationTimestamp(metav1.Time{})

log.Debugw("Finished conversion", "name", u.GetName(), "unstructured", u.Object)
return nil
Expand Down Expand Up @@ -109,6 +113,10 @@ func updateCachingImage(instance *servingv1alpha1.KnativeServing, u *unstructure
if err != nil {
return err
}
// Cleanup zero-value default to prevent superfluous updates
u.SetCreationTimestamp(metav1.Time{})
delete(u.Object, "status")
Copy link
Contributor

Choose a reason for hiding this comment

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

The need to delete status here again seems like something that would be hard for someone to know to do. Would it make sense to have manifestival ingore status when comparing for equality? Or is this not a generic/universal enough need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was curious about this, too, because the core types don't exhibit this behavior. Note that converting to appv1.Deployment did not set a default status. Because Image is a knative type, I expect there may be some critical difference between the core and knative generated functions. Could it be a bug with Image that it's setting that status field at all?


log.Debugw("Finished conversion", "name", u.GetName(), "unstructured", u.Object)
return nil
}
Expand Down
32 changes: 31 additions & 1 deletion third_party/VENDOR-LICENSE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,37 @@ OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.


===========================================================
Import: knative.dev/serving-operator/vendor/github.com/evanphx/json-patch

Copyright (c) 2014, Evan Phoenix
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:

* Redistributions of source code must retain the above copyright notice, this
list of conditions and the following disclaimer.
* Redistributions in binary form must reproduce the above copyright notice
this list of conditions and the following disclaimer in the documentation
and/or other materials provided with the distribution.
* Neither the name of the Evan Phoenix nor the names of its contributors
may be used to endorse or promote products derived from this software
without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.



===========================================================
Import: knative.dev/serving-operator/vendor/github.com/go-logr/logr

Expand Down Expand Up @@ -8524,4 +8555,3 @@ DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

51 changes: 14 additions & 37 deletions vendor/github.com/jcrossley3/manifestival/manifestival.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

131 changes: 131 additions & 0 deletions vendor/github.com/jcrossley3/manifestival/patch/patch.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading