Skip resources that are NotFound for dependency calculation #1519
Conversation
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82
requested review from
alenkacz,
gerred,
kensipe,
nfnt and
zen-dog
as code owners
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
| annotations, _, _ := unstructured.NestedMap(unstructMap, "spec", "template", "metadata", "annotations") | ||
| assert.NotNil(t, annotations, "Statefulset pod template spec contains no annotations") | ||
|
|
||
| hash := annotations[kudo.DependenciesHashAnnotation] |
…nal object Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
| @@ -138,16 +139,25 @@ func (de *dependencyCalculator) getHashForDependency(d resourceDependency) (hash | |||
| if err != nil { | |||
| return hashBytes{}, fmt.Errorf("failed to get dependeny %s/%s: %v", d.namespace, d.name, err) | |||
| } | |||
|
|
|||
| if dep == nil { | |||
There was a problem hiding this comment.
usually there is an apierrors.IsNotFound(err) or the like... how do we know that dep==nil is NotFound?
It isn't clear if the message below is correct
| // we may encounter objects that are not deployed by KUDO and therefore do not have the LastAppliedConfigAnnotation. | ||
| // This is a best effort attempt to make the hash as stable as possible | ||
| v := int64(0) | ||
| obj.SetResourceVersion("") |
There was a problem hiding this comment.
I would be interesting in how to created this list
It is unclear to me if this "overkill" , "insufficient" or "just right"
There was a problem hiding this comment.
It's certainly overkill :) I basically went through all setters on Unstructured...
| @@ -190,6 +222,9 @@ func (de *dependencyCalculator) resourceDependency(d resourceDependency) (*unstr | |||
| } | |||
|
|
|||
| err := de.Client.Get(context.TODO(), key, dep) | |||
| if apierrors.IsNotFound(err) { | |||
There was a problem hiding this comment.
this is semantics or opinion (so not blocking to me) but I would rather have the isNotFound(err) by the calling client than pass a (nil, nil) as something meaningful
There was a problem hiding this comment.
I've decided against it. I've tried it, and i'd have to either:
return err here, without adding an fmt.Errorf("some nice message %w", err). At least it's a non-public func.
or I have to do
if (apierrors.IsNotFound(errors.Unwrap(err)) { on the caller side.
Both solutions don't really sit well with me.
I get where you're coming from, but in this case I prefer the nil, nil return.
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
What this PR does / why we need it:
The easiest fix for the referenced issue is to simply ignore resources that are reported as "NotFound" from the API server. The deployment/statefulset/etc can still be applied, and k8s will correctly use the referenced config maps/secrets as soon as they are available, therefore not blocking KUDO.
The downside is that the next time the deployment/statefulset/etc gets deployed the hash changes and triggers a pod reload, but I think that's an acceptable downside.
Additionally, we can't rely on the
LastAppliedTemplateAnnotation, as this annotation is only available on resources deployed by KUDO. We may encounter references that are applied either manually or by other controllers that don't have this annotation.The fix for this is to fall back to the object itself if the annotation is not available, and
nilmost fields on theUnstructuredobject that may affect the hash calculation.Fixes #1517 #1521
Signed-off-by: Andreas Neumann aneumann@mesosphere.com