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.
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.
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
Add the recreateOption to the object template #253
Add the recreateOption to the object template #253
Changes from all commits
91aa552
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this log mismatch message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if reflect.DeepEqual(dryRunUpdatedObj.Object, existingObjectCopy.Object)
then the log message is"A mismatch was detected but a dry run update didn't make any changes. Assuming the object " + "is compliant.",
is this intended?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is existing code but it was moved in this PR.
This code is detecting the case where the config-policy-controller thought there was a difference but the dry run update request showed that it was not different after all. This can happen when empty values are not shown in the API output but are set in the policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about policy latency (if that's the right term) with this loop.
Say I have
x
number of theserecreate
policies, but the objects they work with have finalizers. Then I think each ends up waiting 10 seconds every config-policy-controller evaluation loop. There is some concurrency,c
, (by default 2 goroutines I think), but it means that the loop takes a minimum offloor(x/c)*10
seconds. They could degrade the performance of the other policies in the cluster, since they would have to wait that long between evaluations.What happens if there isn't a loop, can it just try the
Create
immediately after theDelete
call returns, and if it fails just get it on the next evaluation? I think theshouldEvaluatePolicy
logic could check for this to ensure it keeps getting evaluated.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JustinKuli I'll have it try three times instead and then give up. I'm worried about the deletion just taking a couple of seconds but then it leading to a long time before the object is recreated if the config-policy-controller is saturated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
shouldEvaluatePolicy
logic already immediately schedules a policy with the timeout status message.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this for loop, it doesn't delete the obj. so the message should be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message is correct because this only happens if there is an error and the error is because the object still exists.