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.

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

chore: chainsaw improvements #2642

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Conversation

eddycharly
Copy link
Contributor

@eddycharly eddycharly commented Feb 16, 2024

Chainsaw improvements (based on comments in #2630)

  • simplified makefile
  • used resource/namespace templating instead of kubectl commands

@eddycharly eddycharly requested a review from a team February 16, 2024 15:44
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@swiatekm swiatekm added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 16, 2024
@eddycharly eddycharly force-pushed the chainsaw-3 branch 2 times, most recently from d9d0c5b to 1cfc4b2 Compare February 16, 2024 16:13
@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 16, 2024

Things like this could probably be eliminated too

# Name of the daemonset to check
DAEMONSET_NAME="prometheus-kubernetessd-collector"

# Get the desired and ready pod counts for the daemonset
read DESIRED READY <<< $(kubectl get daemonset -n $NAMESPACE $DAEMONSET_NAME -o custom-columns=:status.desiredNumberScheduled,:status.numberReady --no-headers)

# Check if the desired count matches the ready count
if [ "$DESIRED" -eq "$READY" ]; then
  echo "Desired count ($DESIRED) matches the ready count ($READY) for $DAEMONSET_NAME."
else
  echo "Desired count ($DESIRED) does not match the ready count ($READY) for $DAEMONSET_NAME."
  exit 1
fi

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@eddycharly
Copy link
Contributor Author

Did i break a unit test ? 🤔

@swiatekm
Copy link
Contributor

Did i break a unit test ? 🤔

No, I'm fixing it here: #2643. It does seem to break much more frequently here than in any other PR, which made it easier to hunt down.

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Nice, this looks much better than before.

@eddycharly
Copy link
Contributor Author

Honestly your tests are of good quality, I often see a lot of hacks to workaround kuttl limitations but not in your tests ! 💪

@eddycharly
Copy link
Contributor Author

Like this one Kuttl-In-Kuttl 😂 🙈

@swiatekm
Copy link
Contributor

Honestly your tests are of good quality, I often see a lot of hacks to workaround kuttl limitations but not in your tests ! 💪

Have you looked at the target allocator tests with jobs running curl in a loop to check if metrics are available? 🙈

@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 16, 2024

Have you looked at the target allocator tests with jobs running curl in a loop to check if metrics are available? 🙈

Not yet ! But metrics is an area we want to address, this is often not well end to end tested and chainsaw could easily be adapted to assert on metrics we believe.

@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 16, 2024

I will address this one next https://github.com/open-telemetry/opentelemetry-operator/blob/main/tests/e2e-targetallocator/targetallocator-kubernetessd/check-daemonset.sh

We really can do better than that ! (not in this PR, i'll do another one)

@jaronoff97 jaronoff97 merged commit 09cac33 into open-telemetry:main Feb 16, 2024
29 checks passed
@eddycharly eddycharly deleted the chainsaw-3 branch February 16, 2024 19:07
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Co-authored-by: Mikołaj Świątek <mail+sumo@mikolajswiatek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants