Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Conversation

@davejrt
Copy link
Contributor

@davejrt davejrt commented Jul 11, 2023

Fixes the cadvisor entry point, which was being split onto new lines, meaning we were dropping some metrics like memory as reported by @michaellzc.

As part of the testing I tried just downloading the binary from github, rather than building it from scratch. That didn't turn out to the final solution so that's an OPTIONAL leave in, but to me seems like an easier approach than compiling ourselves.

With a rebuild of the base image we're also now running cadvisor 0.47.3

Old (working):

            "Entrypoint": [
                ....
                "-enable_metrics=cpu,diskIO,memory,network",
                ....
            ],

New (broken):

 "Entrypoint": [
                "/usr/bin/cadvisor",
                ....
                "-enable_metrics=cpu",
                "diskIO",
                "memory",
                "network",
                ....

Closes https://github.com/sourcegraph/devx-support/issues/68

Test plan

Tested on scaletesting.sgdev.org

image

vs sourcegraph.com

image

@cla-bot cla-bot bot added the cla-signed label Jul 11, 2023
@davejrt davejrt requested review from jhchabran and willdollman July 11, 2023 23:25
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Jul 11, 2023

Codenotify: Notifying subscribers in CODENOTIFY files for diff 76630f0...9a7d438.

Notify File(s)
@bobheadxi docker-images/cadvisor/BUILD.bazel
docker-images/cadvisor/entrypoint.sh

@willdollman
Copy link
Contributor

The current wolfi-images/cadvisor.yaml actually uses the chainguard cadvisor package rather than our built-from-source version. I switched it during development as Chainguard added cadvisor to their repo but didn't remove our custom config.

If you want to switch back to our custom package, you can change this line to cadvisor@sourcegraph

Copy link
Contributor

@willdollman willdollman left a comment

Choose a reason for hiding this comment

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

See note about Sourcegraph vs Wolfi cadvisor package, but if it's working as-is then I guess it doesn't need to be changed. Thanks for investigating @davejrt 🎉

@davejrt
Copy link
Contributor Author

davejrt commented Jul 12, 2023

See note about Sourcegraph vs Wolfi cadvisor package, but if it's working as-is then I guess it doesn't need to be changed. Thanks for investigating @davejrt 🎉

Both with the same result...so it's better we continue using the chainguard version. They released 0.47.3 yesterday so I've rebuilt and upgraded us, retested and everything looks good.

I'm removing the reference to the package to avoid any confusion going forward.

@davejrt davejrt enabled auto-merge (squash) July 12, 2023 14:44
@davejrt davejrt merged commit 0544394 into main Jul 12, 2023
@davejrt davejrt deleted the dt/cadvisor branch July 12, 2023 15:16
@michaellzc
Copy link
Member

thank you!

can we backport this to 5.1 as well?

@github-actions
Copy link
Contributor

The backport to 5.1 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-5.1 5.1
# Navigate to the new working tree
cd .worktrees/backport-5.1
# Create a new branch
git switch --create backport-54809-to-5.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 054439411834940cf45f1e5afa4e5c72722d439d
# Push it to GitHub
git push --set-upstream origin backport-54809-to-5.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-5.1

Then, create a pull request where the base branch is 5.1 and the compare/head branch is backport-54809-to-5.1.

@github-actions github-actions bot added backports failed-backport-to-5.1 release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases labels Jul 12, 2023
willdollman pushed a commit that referenced this pull request Jul 12, 2023
Fixes the cadvisor entry point, which was being split onto new lines,
meaning we were dropping some metrics like memory as reported by
@michaellzc.

As part of the testing I tried just downloading the binary from github,
rather than building it from scratch. That didn't turn out to the final
solution so that's an OPTIONAL leave in, but to me seems like an easier
approach than compiling ourselves.

With a rebuild of the base image we're also now running cadvisor 0.47.3

Old (working):
```
            "Entrypoint": [
                ....
                "-enable_metrics=cpu,diskIO,memory,network",
                ....
            ],
```

New (broken):
```
 "Entrypoint": [
                "/usr/bin/cadvisor",
                ....
                "-enable_metrics=cpu",
                "diskIO",
                "memory",
                "network",
                ....
```

Tested on scaletesting.sgdev.org

<img width="1121" alt="image"
src="https://github.com/sourcegraph/sourcegraph/assets/2067825/bedfbe7e-b83c-4a09-8527-5143412cb533">

vs sourcegraph.com

<img width="692" alt="image"
src="https://github.com/sourcegraph/sourcegraph/assets/2067825/0b221909-8c80-4665-9e28-59ba6e6a4722">

(cherry picked from commit 0544394)
camdencheek pushed a commit that referenced this pull request Jul 12, 2023
Fixes the cadvisor entry point, which was being split onto new lines,
meaning we were dropping some metrics like memory as reported by
@michaellzc.

As part of the testing I tried just downloading the binary from github,
rather than building it from scratch. That didn't turn out to the final
solution so that's an OPTIONAL leave in, but to me seems like an easier
approach than compiling ourselves.

With a rebuild of the base image we're also now running cadvisor 0.47.3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backports cla-signed release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants