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

Diagnose Resource #7228

Merged
merged 1 commit into from
May 8, 2023
Merged

Diagnose Resource #7228

merged 1 commit into from
May 8, 2023

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Mar 12, 2023

Explain the changes

  1. Add the script that will be run by the job that was created in Diagnose Resource noobaa-operator#1072

For more information see Jira MCGI-143

Issues: Gap

  1. In this PR we don't handle the case of AWS STS (MCGI-149).

Testing Instructions:

Note: to actually run the tests you'll need to add the changes from this PR in noobaa-operator.

  1. Based on the instructions here - the steps of ‘Build images’ and ‘Deploy noobaa’.
    Note: nb is a alias that runs the local operator from build/_output/bin (alias created by devenv).
  2. For backingstore, Run:
    nb backingstore create aws-s3 <backing-store-name>
    nb diagnostics analyze backingstore <backing-store-name>
  3. For namespacestore, Run:
    nb namespacestore create aws-s3 <namespace-store-name>
    nb diagnostics analyze namespacestore <namespace-store-name>
  • It creates the log files and then deletes the created job.

Testes types: aws-s3, azure-blob, s3-compatible (with noobaa system as a server, the endpoint was taken from the field InternalDNS under S3 Addresses.

  • Doc added/updated
  • Tests added

@shirady shirady requested a review from dannyzaken March 12, 2023 11:34
@shirady shirady force-pushed the analyze-resource branch 2 times, most recently from cb68ef1 to 999f1cb Compare March 14, 2023 13:27
@nimrod-becker
Copy link
Contributor

Have you looked at s3cat.js ?
Might be some things we can share between the two instead of dupping

@shirady shirady self-assigned this Mar 14, 2023
@shirady
Copy link
Contributor Author

shirady commented Mar 14, 2023

Have you looked at s3cat.js ? Might be some things we can share between the two instead of dupping

@nimrod-becker, no I was not aware of it. I'll have a look at it, thanks!

@shirady
Copy link
Contributor Author

shirady commented Mar 15, 2023

Hi,
@nimrod-becker I looked at the file s3cat.js, and I'm not sure it is duplicated...
As we defined the tests that we wish to run as: list objects, head\read to one of the objects, and write (I added the delete operation so I would leave the written file). The file includes those operations (and many more) but it is not "tailored" as I will use it in the later stories - I want it to be simple and work the same way in other SDKs (azure and gcp).
It also passes the secret key and access key as arguments which I want to avoid.
Of course, if there is a function there that we want to reuse, we can (but anyway, it will be re-use only for aws-s3 sdk, and not for the other scripts gcp and azure).

@romayalon
Copy link
Contributor

@shirady I feel that the correct way to do this is to create "recipes" for the different kinds of tests you need to do from outside the s3 job area (or other cloud provider areas), also it'll help in the future if we would like to extend and make more tests/analysis, as it is today it's not very flexible in my opinion.
s3cat.js already implements the s3 client creation and the needed functions head, list, upload, and download when the resource is s3. it'll of course not help you with GCP and Azure because it implements s3 functions. What I expected is to have gcpcat/ blobcat for other cloud providers. NBcat is another reason to use this method, although it needs to be improved.

@romayalon
Copy link
Contributor

@shirady @dannyzaken @nimrod-becker

Putting here some points from my suggestion discussed with Shira -

  1. The Analyze CLI can be a very strong tool if it'll be flexible - which means it will be able to receive new types of tests in the future. (currently, this PR handles access checks only). I find this kind of tool helpful not only for support people but for the dev team for debugging purposes as well. (for example, imagine a performance test job that runs with a single CLI call)
  2. In order to keep this tool flexible, we need to decouple the "report" handling from the cloud provider's SDK calls as a start. This will reduce the repetitive "report" code of the next cloud providers analysis code or different tests we will hopefully create.
  3. Different types of tests can be created by the same job yaml as created here https://github.com/noobaa/noobaa-operator/pull/1072/files but with different parameters
  4. More test configurations might add more SDK calls, these can be simply added to the different cloud utils.
  5. I still believe that with more tests and more SDK calls, this code will be almost the same as s3cat.js with minor changes, so adapting it now might reduce the repetitive code later.

@shirady shirady force-pushed the analyze-resource branch 2 times, most recently from cba449f to 17e5989 Compare March 22, 2023 11:17
@shirady shirady changed the title Analyze Resource - Basic Script to Check AWS Connectivity Diagnose Resource (AWS only) Mar 22, 2023
@shirady shirady force-pushed the analyze-resource branch 3 times, most recently from 11aa478 to d5841f2 Compare March 27, 2023 10:08
@shirady shirady changed the title Diagnose Resource (AWS only) Diagnose Resource Mar 30, 2023
@shirady shirady marked this pull request as ready for review March 30, 2023 12:57
@shirady shirady requested a review from dannyzaken April 19, 2023 11:29
@nimrod-becker nimrod-becker requested review from a team and aspandey and removed request for romayalon and a team April 20, 2023 10:06
@shirady shirady force-pushed the analyze-resource branch 3 times, most recently from dc455e1 to 1b58bbf Compare May 1, 2023 06:35
@shirady shirady requested a review from dannyzaken May 1, 2023 06:38
@shirady shirady force-pushed the analyze-resource branch 2 times, most recently from f0203c5 to b40d134 Compare May 2, 2023 07:46
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady merged commit c5277f0 into noobaa:master May 8, 2023
6 checks passed
@shirady shirady deleted the analyze-resource branch July 2, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants