Skip to content

Conversation

@movence
Copy link
Contributor

@movence movence commented May 5, 2025

Description of changes:

  • Merge Auto Monitor follow up changes #311 changes into main
  • Add integration tests for AutoMonitor
  • Speed up integration test runs by caching docker image between each integration test run

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

the-mann added 30 commits March 10, 2025 10:06
…h AutoMonitor and AnnotationMutators, check if safe to mutate before applying custom selector, run opt out tests for each type of workload, test opt out by disabling monitorallservices
CustomSelectors should obey autoRestart, autoAnnotateAutoInstrumentation shouldn't. Rename MonitorInterface to InstrumentationAnnotator. Add docs.
…ntation container validation by default.

This is required for automonitor to work, because it tries to apply all init containers to each selected pod.
…el custom selector should not update pod template
…art namespace for autoAnnotateAutoInstrumentation.
@the-mann
Copy link
Contributor

will fail/be flakey until #311 is merged in

* Implement patching and workload informers

* Update EnableMultiInstrumentationSupport and SkipMultiInstrumentationContainerValidation feature flags to be enabled by default

* incorporate feedback
Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

Copy link
Contributor

Choose a reason for hiding this comment

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

reviewed in #311

@the-mann the-mann changed the title Add integ tests for AppSignals AutoMonitor AppSignals AutoMonitor follow up + integration testing May 23, 2025
@movence movence closed this Jun 16, 2025
@the-mann the-mann reopened this Sep 25, 2025
@the-mann
Copy link
Contributor

reopening to see status of integ tests

@the-mann the-mann changed the title AppSignals AutoMonitor follow up + integration testing AppSignals AutoMonitor integration testing Sep 26, 2025
run: |
cd integration-tests/manifests/automonitor/ && \
echo "tests<<EOF" >> $GITHUB_OUTPUT
go test -list . | sed -e '$d' >> $GITHUB_OUTPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, can we call the test directly using absolute path?

Suggested change
go test -list . | sed -e '$d' >> $GITHUB_OUTPUT
go test -list integration-tests/manifest/automonitor/ | sed -e '$d' >> $GITHUB_OUTPUT

- name: Start minikube
uses: medyagh/setup-minikube@master

- name: Deploy cert-manager to minikube
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: at some point we should combine the start minikube, deploy cert-manager, and verify minikube and cert manager steps into their own workflow since they're used everywhere

}
}
// sleep to give pods time to come up
time.Sleep(20 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make a const

h.updateOperatorConfig(deploymentArg{jsonStr, "--auto-annotation-config="})
}

func (h *TestHelper) updateOperatorConfig(argList ...deploymentArg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of modifying an existing operator deployment can't we apply a deployment object? or use yaml to force an update on the operator?

flag string
}

func (h *TestHelper) ValidateNamespaceAnnotations(namespace string, shouldExist []string, shouldNotExist []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of having a shouldNotExist parameter, can't we just strictly check the shouldExist? Maybe we have some filter to only look at auto annotate prefixes?

@@ -0,0 +1,32 @@
apiVersion: apps/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

all of these manifests are pretty much identical but just different names. Can we re-use just 1?

const TimeBetweenRetries = 5 * time.Second

// WaitForNewPodCreation takes in a resource either Deployment, DaemonSet, or StatefulSet wait until it is in running stage
func WaitForNewPodCreation(clientSet *kubernetes.Clientset, resource interface{}, startTime time.Time) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants