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

Run Collect-info through Edgeview #4106

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

naiming-zededa
Copy link
Contributor

@naiming-zededa naiming-zededa commented Jul 25, 2024

there are a number of request to have edgeview support for collectinfo,
this patch has the list of implementation:

  • Add the implementation on running collect-info through Edgeview
  • Egeview is mostly only with read permission access to device filesystems
    and it does not want to import all the pkgs and replicate the collect-info.sh
    operations and maintain compatibility of two sets of the collect-info
  • this implementation has a protocol to setup a request for debug container
    to start run 'collect-info.sh', and remove the request file after it is done;
    Edgeview then uses existing copy file protocol to transfer the collect-info
    tarball file back to user's laptop; then Edgeview put another request for
    debug container to remove the created tarball file in /persist
  • debug container has added a background task to monitor the requests
  • Since collect-info is mostly live info, there is no need to get all the newlogs,
    only fetch the newlogs of the past 10 days.
  • Introduced debug/debug-services.sh around the exiting sshd and the new edgeview-collectinfo.sh

pkg/debug/ssh.sh Outdated Show resolved Hide resolved
pkg/debug/ssh.sh Outdated Show resolved Hide resolved
pkg/debug/ssh.sh Outdated Show resolved Hide resolved
@OhmSpectator
Copy link
Member

OhmSpectator commented Jul 25, 2024

Commit messages, pleeeease 😭😭😭

@naiming-zededa
Copy link
Contributor Author

Commit messages, pleeeease 😭😭😭

hmm, i thought the one before were commit messages, but I added something before the list, is that what you meant?

@OhmSpectator
Copy link
Member

hmm, i thought the one before were commit messages, but I added something before the list, is that what you meant?

No, I mean the messages that you add to any commit when creating it...
Like here: e74fae7
Or here: caad6b5

Without proper commit messages, it's hard to understand the changes made when looking through the history or git blame. It's nice you've added a PR description here on GitHub, but it will be lost after the PR is merged: it will be impossible to see it from an IDE or the command line...

@naiming-zededa
Copy link
Contributor Author

@OhmSpectator added in the commit. to see if this is good. thanks.

@naiming-zededa naiming-zededa force-pushed the naiming-ev-collectinfo branch 2 times, most recently from 9d0e64b to b1c0710 Compare July 27, 2024 05:33
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Note that the pathname for the eve-info files changed in #4125 so they are in the /persist/eve-info/ directory instead of just in /persist/

So presumably you need to add that eve-info component to the pathnames you use here.

@naiming-zededa
Copy link
Contributor Author

Note that the pathname for the eve-info files changed in #4125 so they are in the /persist/eve-info/ directory instead of just in /persist/

So presumably you need to add that eve-info component to the pathnames you use here.

missed that. will change.

@naiming-zededa
Copy link
Contributor Author

Changed to reflect the new /persist/eve-info/ as the base. I removed the 'mkdir -p "/persist/$INFO_DIR_SUFFIX"' in the collect-info.sh, since this created directory is not used, and never removed after.
I have tested this new change for both manual run and through the edgeview.

@eriknordmark
Copy link
Contributor

Changed to reflect the new /persist/eve-info/ as the base. I removed the 'mkdir -p "/persist/$INFO_DIR_SUFFIX"' in the collect-info.sh, since this created directory is not used, and never removed after. I have tested this new change for both manual run and through the edgeview.

Odd, because I had to add that mkdir to make the tar command not fail when I just did a 'eve enter debug' followed by collect-info.sh.
But if you tested that, then there must have been some fluke when I ran it.

@naiming-zededa
Copy link
Contributor Author

fixed a yetus complaining

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Do we have any automatic tests of edgeview? And how hard would it be to create some basic one to just check that it is functional?

@naiming-zededa
Copy link
Contributor Author

Do we have any automatic tests of edgeview? And how hard would it be to create some basic one to just check that it is functional?

Currently I don't have this infra. We need to create a task/jira item o this.

- Add the implementation on running collect-info through Edgeview
- Egeview is mostly only with read permission access to device filesystems
  and it does not want to import all the pkgs and replicate the collect-info.sh
  operations and maintain compatibility of two sets of the collect-info
- this implementation has a protocol to setup a request for debug container
  to start run 'collect-info.sh', and remove the request file after it is done;
  Edgeview then uses existing copy file protocol to transfer the collect-info
  tarball file back to user's laptop; then Edgeview put another request for
- debug container to remove the created tarball file in /persist
  debug container has added a background task to monitor the request-
- Since collect-info is mostly live info, there is no need to get all the newlogs,
  only fetch the newlogs of the past 10 days.
- change the debug/ssh.sh into debug/debug-tasks.sh to reflect it does
  not only handling the ssh task

Signed-off-by: Naiming Shen <naiming@zededa.com>
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknordmark
Copy link
Contributor

@naiming-zededa the regression/eden tests passed, but there might be some open comments/issues on the PR. Can you check if those are taken care of to satisfaction?

@naiming-zededa
Copy link
Contributor Author

@naiming-zededa the regression/eden tests passed, but there might be some open comments/issues on the PR. Can you check if those are taken care of to satisfaction?

@eriknordmark I went through them, they should all be resolved.

@eriknordmark eriknordmark merged commit 0aa8e94 into lf-edge:master Aug 8, 2024
35 checks passed
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.

6 participants