Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Detect changes/drift through a filtered diff of the apply #379

Closed
wants to merge 2 commits into from
Closed

Detect changes/drift through a filtered diff of the apply #379

wants to merge 2 commits into from

Conversation

JaneLiuL
Copy link
Contributor

@JaneLiuL JaneLiuL commented Jun 29, 2021 •
edited
Loading

This PR including below contents:

  1. run kubectl diff first and parse the output. .
  2. update the object events.
  3. test for the output

Detail could be check on: #352

@stefanprodan
Copy link
Member

We need a test that proves that we ignore objects that are changed only due to the checksum annotation. We also need to test diff failures, for example a namespace and configmap will error out on create, but will succeed on update.

@JaneLiuL
Copy link
Contributor Author

Get ~ I will add it now~

@hiddeco
Copy link
Member

hiddeco commented Jun 29, 2021

I think that as a future optimization it would also be an idea to create some type of wrapper around the working directory and/or commands we run so that we can omit the cd ... && (or make them simple methods that run specific commands, or something something).

@JaneLiuL
Detect changes/drift through a filtered diff of the apply
adfa310 
Signed-off-by: Jane Liu L <jane.l.liu@ericsson.com>
@JaneLiuL JaneLiuL force-pushed the main branch 3 times, most recently from 7c53b7d to ab3f40f Compare July 6, 2021 03:43
stefanprodan
stefanprodan reviewed Jul 6, 2021
View reviewed changes
controllers/utils.go Outdated

func containsChangeInSlice(tmpslice []string) bool {
for _, s := range tmpslice {
if strings.HasPrefix(s, "+ kustomize.toolkit.fluxcd.io/checksum:") || strings.HasPrefix(s, "- kustomize.toolkit.fluxcd.io/checksum:") || strings.HasPrefix(s, "--- /tmp") || strings.HasPrefix(s, "+++ /tmp") {
Copy link
Member

Choose a reason for hiding this comment

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

Please declare a var above like checksumAnnotation := fmt.Sprintf("%s/checksum", kustomizev1.GroupVersion.Group)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix done

@stefanprodan
Copy link
Member

stefanprodan commented Jul 6, 2021 •
edited
Loading

Some things to note about this PR:

  • if the diff returns no changeset, then the controller skips the apply
  • the diff excludes changes to the checksum annotation trying to reduce the event spam but it fails to do so for Kubernetes Deployments, Statefulsets and Daemonsets due to the fact that an annotation change bumps the object generation
  • the diff errors out with exit code 2 if newly created objects are part of a new namespace
  • we ignore the diff crash and fallback to the apply output to compose the changeset
  • the apply output is very different to the diff one, users will receive inconsistent events, when diff fails the changeset format is <object-name> <action>, if diff works then the format is <api-version>.<kind>.<namespace>.<name> <action>
  • diff fails if webhooks like OPA Gatekeeper are installed on the cluster that have sideEffects: Unknown
  • we have no idea about the performance impact of this change, in theory this would double the number of API calls made to the API server
  • we load the diff output into memory to parse it and extract the changeset, if the output contains thousands of newly created objects the reconciliation would become significantly slower

stefanprodan
stefanprodan reviewed Jul 6, 2021
View reviewed changes
controllers/kustomization_controller.go
return "", fmt.Errorf("apply failed: %s", applyErr)
}

resources := parseApplyOutput(output)
log.Info(
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't log that we applied something on the cluster if we didn't, please move the log below inside else if differr.ExitCode() == 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix done

@stefanprodan stefanprodan mentioned this pull request Jul 6, 2021
Closed
stefanprodan
stefanprodan reviewed Jul 6, 2021
View reviewed changes
controllers/kustomization_controller.go Outdated
// Kubectl or diff failed with an error.
if differr, ok := err.(*exec.ExitError); ok {
if differr.ExitCode() > 1 {
log.Info("Kubectl or diff failed with an error, will execute apply soon")
Copy link
Member

Choose a reason for hiding this comment

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

Make this a debug log please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix done~

controllers/kustomization_controller.go Outdated
} else if differr.ExitCode() == 1 {
resources = parseDiffOutput(diffoutput)
// Since we found difference in "diff" command, so we need to execute apply
log.Info("Differences found, will execute apply soon")
Copy link
Member

Choose a reason for hiding this comment

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

Make this a debug log please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix done

@stefanprodan
Copy link
Member

One unfortunate outcome of skipping the apply when the diff returns no changes is that enabling pruning will not be possible, as the the annotations will never be added.

@JaneLiuL JaneLiuL force-pushed the main branch 2 times, most recently from 8b9d7bf to 5b7d918 Compare July 12, 2021 02:19
@JaneLiuL
add UT for proves ignore objects that are changed only due to the che…
bc59ad3 
…cksum annotation and test diff failures

Signed-off-by: Jane Liu L <jane.l.liu@ericsson.com>
@JaneLiuL
Copy link
Contributor Author

JaneLiuL commented Aug 2, 2021

Any update for this PR? Should I still keep this PR or not? @stefanprodan

@hiddeco
Copy link
Member

hiddeco commented Aug 3, 2021 •
edited
Loading

👋

We have not forgotten about this PR, but are currently in the process of a bigger refactor which will drastically change the structure of all controller's reconcilers (see fluxcd/flux2#1601 for details), and improve the test coverage of the individual operations a controller performs.

Given one of the goals of this PR is to hypothetically improve performance, we think it is better for the refactor to land first, so that we are properly able to benchmark your proposal against the current way the controller does things.

Well aware this may be a bummer to you, and I am really sorry about that. 🌻

@stefanprodan stefanprodan mentioned this pull request Sep 11, 2021
Merged
@JaneLiuL
Copy link
Contributor Author

Close this PR since already have stefanprodan/kustomizer#17 to cove

@JaneLiuL JaneLiuL closed this Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanprodan stefanprodan stefanprodan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JaneLiuL @stefanprodan @hiddeco