Fix handling of managedResourceTypes #1851
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Piojo
force-pushed
the
log-managedresourcetypes
branch
3 times, most recently
from
4dea32c
to
f296dba
Compare
added 4 commits
September 17, 2021 01:30
They make following the code a lot easier.
If there is a new kind `Invalid value:` error when running `oc.apply` the current code ignores it silently. We add a fallback exception to discover such situations.
We were silently dropping any desired resources on a namespace that doesn't expect it. This made it terribly difficult for openshift_resources_base to report to users that it was skipping some of their resources. And openshift_resources_base was expecting such an exception to do the reporting. So, we raise the exception and suppress it in the places that didn't care about it in the first place.
I've had to refactor the namespaces part of init_specs_to_fetch so that we can iterate by resources instead of by resource types. It results in some more loops, but less nesting and logs become possible. The added unit tests pass both on current master and here.
Piojo
force-pushed
the
log-managedresourcetypes
branch
from
f296dba
to
d64b6b7
Compare
esron
approved these changes
Sep 17, 2021
Piojo
force-pushed
the
log-managedresourcetypes
branch
from
d64b6b7
to
9d0d970
Compare
maorfr
added a commit
that referenced
this pull request
Sep 22, 2021
Piojo
pushed a commit
to Piojo/qontract-reconcile
that referenced
this pull request
Sep 22, 2021
This reverts commit 7b14258 (app-sre#1862)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were silently skipping resource types we weren't managing, making
troubleshooting misconfigurations very hard.
First, we add type hints to
openshift_base.init_specs_to_fetch, tomake it easier to understand and to refactor.
Then, we can log skipped managedResourceNames in this method. This PR
refactors the namespace handling path to achieve that. There are unit
tests to confirm the behaviour is the same as it used to be, but
please have a look and feel free to suggest relevant configurations
that need extra tests.
Next, we raise an exception if
oc.apply. There is a failure mode inwhich an invalid input can be discarded and not raise any exceptions,
we now have a fall-back exception if that happens. I am not sure
unmanaged types can reach the
applyphase - probably not - but Ifound this bug in my investigation and it's trivial to sort out.
Afterwards,
openshift_resourcesexpectsResourceInventory.add_desiredto raise aKeyErrorif it tries toadd for application a resource of a kind that shouldn't be
managed. However,
add_desiredwas silently dropping that exceptionon the floor, a behaviour all remaining callers expected. So, I have
restored the exception and handled or suppressed it where appropriate.
Finally, saasherder also does its own filtering when processing SaaS
files - until now we've only handled namespaces. So we refactor a loop
in there to have more verbose logging. Writing unit tests for this one
is extremely hard, and the expansion of this list comprehension to a
loop should be easy-ish for a human reviewer to critique.
Fixes https://issues.redhat.com/browse/APPSRE-3668