Conversation
|
The "superlinter" found several yaml formatting issues. Can you please fix those? Since I'm in an off-site meeting this week, I will look over the code changes next week. |
|
I dont see any issue with the yaml formatting, I'm using a range for duplicating files which may cause to the error. I tested it, and it works fine.
|
kcantrel
left a comment
There was a problem hiding this comment.
I reviewed all the files, and went through all the steps for deploying on an EC2 instance. I did not use the CloudFormation template. In the end, however, Harvest could not access the credentials, I assume because I didn't know how to create, nor pass to the container the fetch_credentials file.
There was a problem hiding this comment.
To me it isn't clear what "SubnetType" does. Maybe add a note saying that selecting "public" means that a public IP will be allocated to the EC2 instance. You could update the description of the variable to match that as well.
There was a problem hiding this comment.
I have taken it from amazon doc.
will add more description about it
There was a problem hiding this comment.
The description of SubnetType is not clear what that parameters does.
There was a problem hiding this comment.
-
At the top you should mention the CloudFormation method of doing the installation so users will know about that easier option before going through all these steps. And for that matter, you should put the CloudFormation directory below the ec2 directory since it only pertains to it.
-
In the Prerequisites section:
- Change "NetApp FSxN" to "A FSx for ONTAP"
- Remove the EC2 instance (see below for why)
- If not running on an AWS Linux based EC2 instance, then ensure the AWS command installed.
-
Before step "1." there should be a Header named "Installation" or something like that.
-
For step 2.1 you might mention that you can get the harvest-policy.json file from the repo. Also, that same command has "Policy" capitalized, where the file in the repo is all lower case. You might also mention that you need to edit the file to add the ARN to your secret(s).
-
Step 3 says to create an EC2 instance, yet it was listed as a prerequisite, so I had already created one. I was going to suggest a lot of changes here to accommodate someone who already has an EC2 instance, but I think instead, it would be easily to simply not list an EC2 instance as a prerequisite. Then on step 2.2 say "If you already have an EC2 instance, then attached the policy to its instance profile role. If it doesn't already have an instance profile role, then perform the following steps to create one.
Then for step 3 say "If you don't already have an EC2 instance then create one using a t2.xlarge instance type with 20GB of disk, using a Red Hat derivative-based Linux (e.g. Amazon Linux). Note that some of the commands shown below assume a Red Hat based system.
Attach the instance profile created above. You can use the following command to attach an instance profile:
aws ec2 associate-iam-instance-profile --instance-id <instance-id> --iam-instance-profile Arn=<Instance-Profile-ARN>,Name=HarvestProfile
-
For step 4 you might put a comment noting that the commands assume a RedHat derivative-based O/S. If your EC2 instance is a Debian based Linux (e.g. Ubuntu), then please refer to the docker installation guide on how to install docker.
-
For step 5 Maybe replace with "The following steps show how to install Harvest. See the official Harvest documentation for further information."
-
For step 5.1: The indentation isn't correct for the "harvest.yml" file. You might mention that you can download the file from the repo.
Where does the /opt/fetch-credentials come from?
-
For step 5.2 you need to put 'sudo' in front of the 'docker' command since only root can run it.
-
For step 5.5 the .zip file you have uploaded into the repo was obviously created on a Mac because it has the __MACOSX directory structure that is only meaningful on a Mac and clutter on any other O/S.
Also, this step implies the directory inside the .zip file will be "fsx_dashboards" when it is just "dashboards".
-
For step 5.6 I think a brief description would be nice so people know what "yet-another-exporter" is. Something along the lines of:
AWS has useful metrics regarding the FSxN file system that ONTAP doesn't provide. Therefore, it is recommended to install an exporter that will expose these metrics. The following steps show how to install a recommended exporter.
Then steps 5.6 and both 5.7s, should be 5.6.1, 5.6.2 and 5.6.3.
There was a problem hiding this comment.
- You might mention in step 2.1 that you get the harvest-Policy.json file from the repo. Also mention you need to edit it to replace the ARN to the secrets.
- Should this file be in the repo, since it is generated with the docker command at step 5.2?
- If it does need to be here, you need to replace the "iscsifsx" SECRET_NAME with <your_secret_1>. Same for the region setting under it.
- Maybe put a comment stating: "An example of a fully filled-out harvest-compose.yaml file can be find in the repo."
- Since step 5.7 is concatenating to the prometheus.yml file using a relative path, you might should mention from which directory that needs to be run from. I assume step 5.2 download that. I apologize, I am writing these comments before I have actually run through the steps, which I do plan on doing, but I wanted to read over the READMEs first.
There was a problem hiding this comment.
Since I assume this is generated, does this need to be in the repo? My concern is that who knows when whatever generates it decides to change it, and someone tries to use this "old" one and it breaks things.
There was a problem hiding this comment.
for adding more FSx, you should modify the files, this is a sample of how to change the file.
There was a problem hiding this comment.
Should the regions be "hard-coded"? Or, should they be left for the user to fill in?
There was a problem hiding this comment.
I have changed to param
There was a problem hiding this comment.
-
The opening paragraph could be a little more descriptive. Maybe something like:
This subfolder contains a Helm chart to install NetApp Harvest into an AWS EKS cluster to monitor multiple FSx for ONTAP file systems using the Grafana + Prometheus stack. It uses the AWS Secrets Manager to obtain credentials for the FSxN file systems so those credentials aren't insecurely stored.
-
In the Prerequisites section:
- The first bullet needs a period at the end.
- On the second bullet, it should be "A FSx for ONTAP file system running in the same VPC as the EKS cluster." or "A FSxN running in the same VPC as the EKS cluster."
- On the third bullet, Prometheus is misspelled. Also, can you provide a link to how to install it? Like add: For instructions on how to install Prometheus refer to this document. Or, actually, I found this Helm chart that apparently installs both Grafana and Prometheus (https://github.com/prometheus-community/helm-charts/tree/main/charts/kube-prometheus-stack). So, maybe a sub bullet under the Grafana line that states "You can install both Prometheus and Grafana by deploying this Helm chart."
- Maybe reword the 5th bullet: "Existing
Secrets Managersecret in the same region as the FSxN file system."
-
In the Integration with AWS Secret Manager section:
- The proper name of the service is AWS SecretS Manager. I think that was misspelled in other places a swell.
- The second sentence should not end with "Manager", it should end with "secret." (i.e. "or new AWS Secrets Manager secret.")
-
In the Monitoring multiple FSxN section, you should replace the vales that someone should fill in with "<name_of_parameter>". For example: "management_lif: <FSx1_Management_LIF_IP>". Same for secretName, and region. I'm not sure about promPort, is that the default port number?
-
In the Installation section:
-
Instead of having (optional) in front of Create AWS Secrets Manager secret, leave that out but have a sentence underneath the title that states:
If you don't already have an AWS Secrets Manager secret, you can use the following command to create one: -
For step 2 can you add the same instructions as you did for adding the service account for the Import FSxN CloudWatch metrics into your monitoring stack*? Mostly that
POLICY_ARN=$(aws iam create-policy ...command. Then, replace "<POLICY_ARN>" with $POLICY_ARN.
-
-
For the Install yace-exporter helm chart section. Maybe worth mentioning that the changes listed below are based on the file in the repo. Otherwise, it is confusing, and I would think I could just "copy and paste" the contents in this READM file and I would have the entire file, which as I just noticed, there is another section of that file that isn't included in this README.
This suggestion really applies to most of the sections in this README file, that there should be some mention that you start with the file from the repo and make the changes listed.
There was a problem hiding this comment.
I like the version of this file in the README better. Mostly because it replaces $SECRET with:
"<your_secret_manager_arn_1>",
"<your_secret_manager_arn_2>"
There was a problem hiding this comment.
- I assume it is okay to have the prometheus section empty? Also, does it matter that it is misspelled?
- Should you have
<fill_in_your_value_here>in the appropriate places? (e.g. management_lif, secretName, region. - There is more here than in the README file. Is the extra stuff needed? Otherwise, I could see someone just "copying and pasting" what's in the README file and it not working correctly.
There was a problem hiding this comment.
Replace the hardcoded regions (ap-northeast-2) with <Region_Name>.
No description provided.