[autoscaler] AWS Autoscaler CloudWatch Dashboard support #20266

Zyiqin-Miranda · 2021-11-11T20:57:58Z

Why are these changes needed?

These changes add a set of improvements to enable automatic creation and update of CloudWatch dashboards when provisioning AWS Autoscaling clusters. Successful implementation of these improvements will allow AWS Autoscaler users to:

Get rapid insights into their cluster state via CloudWatch dashboards.
Allow users to update their CloudWatch dashboard JSON configuration files during Ray up execution time.

Notes:

This PR is a follow-up PR for [autoscaler] AWS Autoscaler CloudWatch Integration #18619, adds dashboard support.

Related issue number

Closes ##9644 #8967

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

DmitriGekhtman · 2021-11-12T03:35:00Z

I'm mostly just reviewing to check that existing Ray functionality is unaffected, which looks to be the case.

Will defer to @pdames for a more thorough review based on the domain knowledge.

python/ray/autoscaler/_private/aws/cloudwatch/cloudwatch_helper.py

pdames

This looks pretty good overall - thanks for making the updates to support cluster-level dashboards! I think if we now just make a few updates to the default/example dashboard config then we'll be in good shape!

python/ray/autoscaler/aws/cloudwatch/example-cloudwatch-dashboard-config.json

python/ray/autoscaler/aws/cloudwatch/example-cloudwatch-alarm-config.json

Zyiqin-Miranda · 2021-12-03T01:15:37Z

This looks pretty good overall - thanks for making the updates to support cluster-level dashboards! I think if we now just make a few updates to the default/example dashboard config then we'll be in good shape!

Thanks Patrick for the review, updated the dashboard configuration json file to auto generate a cluster-level example dashboard now.

pdames · 2021-12-09T19:31:56Z

The latest updates LGTM. @wuisawesome, @DmitriGekhtman - could one of you take a look?

DmitriGekhtman · 2021-12-09T23:50:34Z

Looks like the CI hit some (hopefully transient) problem. Could you rebase/merge master?

Seems like requested changes were made.

wuisawesome

Would it be possible to split this PR up into pieces? If I'm understanding it correctly, it seems like it should be possible to split into 3 or 4 pieces.

Agent related changes (the changes related to updating the head/worker nodes)
The refactor which generalizes the logic and introduces CloudwatchConfigType
Dashboard feature
Alarm feature

Also please let me know if I'm missing something here and it can't be split up.

Zyiqin-Miranda · 2021-12-14T21:37:01Z

Would it be possible to split this PR up into pieces? If I'm understanding it correctly, it seems like it should be possible to split into 3 or 4 pieces.

Agent related changes (the changes related to updating the head/worker nodes)

The refactor which generalizes the logic and introduces CloudwatchConfigType

Dashboard feature

Alarm feature

Also please let me know if I'm missing something here and it can't be split up.

Thanks Alex, the agent related updates are already included in the first PR that is merged and I can split this PR into:
1. dashboard support
2. alarm support
3. generalize and consolidate agent, dashboard and alarm logic with all unit tests.
May I know if it looks good to you?

pdames · 2021-12-15T21:53:23Z

The refactor to include only the changes relevant to the CloudWatch dashboard here look good to me. One thing to note is that we'll want to quickly follow this with another PR that adds unit tests to guard against regressions. Thanks @Zyiqin-Miranda!

wuisawesome · 2022-01-06T21:05:54Z

@DmitriGekhtman do you mind taking a look at this too

wuisawesome · 2021-12-20T21:27:26Z

python/ray/autoscaler/aws/example-cloudwatch.yaml

@@ -14,8 +14,9 @@ provider:
        # We depend on AWS Systems Manager (SSM) to deploy CloudWatch configuration updates to your cluster,
        # with relevant configuration created or updated in the SSM Parameter Store during `ray up`.

-        # The `AmazonCloudWatch-ray_agent_config_{cluster_name}` SSM Parameter Store Config Key is used to
-        # store a remote cache of the last Unified CloudWatch Agent config applied.
+        # We support three CloudWatch related config type under this cloudwatch section: agent, dashboard and alarm.


nit: we don't support alarm yet in this pr right?

Nice catch, removed alarm, thanks!

wuisawesome · 2021-12-20T21:32:56Z

python/ray/autoscaler/_private/aws/cloudwatch/cloudwatch_helper.py

        except botocore.exceptions.WaiterError as e:
            logger.error(
                "Failed while waiting for EC2 instance checks to complete: {}".
                format(e.message))
            raise e

-    def _update_cloudwatch_agent_config(self, is_head_node: bool) -> None:
+    def _update_cloudwatch_config(self, is_head_node: bool,
+                                  config_type: str) -> None:
        """ check whether update operations are needed.


Can we make this description a bit more detailed (it seems like this applies update operations in addition to checking?), and include a description of the args?

Sure, added description and args doc string.

wuisawesome · 2021-12-20T21:34:33Z

python/ray/autoscaler/_private/aws/cloudwatch/cloudwatch_helper.py

        """ check whether update operations are needed.
        """
        cwa_installed = self._setup_cwa()
-        param_name = self._get_ssm_param_name()
+        param_name = self._get_ssm_param_name(config_type)
        if cwa_installed:
            if is_head_node:


Can we dedupe some of this logic? it seems like both cases are checkign if hashes map and then restarting the cloduwatch agent?

Btw just checking, is

elif config_type == "dashboard": self._put_cloudwatch_dashboard()

not valid on a non-head node?

Yes, just head node is responsible for putting a cluster-level dashboard.

For hashes comparison, head node compares the hash of local file with the hash of remote file stored at aws Systems Manager parameter store; worker nodes compare its ec2 hash tag value with head node ec2 hash tag value.

wuisawesome · 2021-12-20T21:36:28Z

python/ray/autoscaler/_private/aws/cloudwatch/cloudwatch_helper.py

            DocumentName=document_name,
            Parameters=parameters,
-            MaxConcurrency=str(min(len(node_ids), 100)),
+            MaxConcurrency=str(min(len(node_id), 100)),


should this just be 1 now?

Yes, updated to 1, thanks Alex!

wuisawesome · 2022-01-10T18:18:47Z

Test failure (client proxy) is known flaky and looks unrelated, merging

These changes add a set of improvements to enable automatic creation and update of CloudWatch alarms when provisioning AWS Autoscaling clusters. Successful implementation of these improvements will allow AWS Autoscaler users to: Setup alarms against Ray CloudWatch metrics to get notified about increased load, service outage. Update their CloudWatch alarm JSON configuration files during Ray up execution time. Notes: This PR is a follow-up PR for #20266, which adds CloudWatch alarm support.

Zyiqin-Miranda changed the title ~~[autoscaler] AWS Autoscaler CloudWatch Dashboard/Alarm support~~ [autoscaler] AWS Autoscaler CloudWatch Dashboard and Alarm support Nov 11, 2021

Zyiqin-Miranda force-pushed the cloudwatch branch 2 times, most recently from e61d068 to 64e273a Compare November 11, 2021 21:43

wuisawesome assigned pdames, wuisawesome and DmitriGekhtman Nov 11, 2021

Zyiqin-Miranda force-pushed the cloudwatch branch 3 times, most recently from 2aa6ff7 to 28ea734 Compare November 12, 2021 03:25

Zyiqin-Miranda force-pushed the cloudwatch branch from 28ea734 to 362a53e Compare November 13, 2021 01:43

pdames reviewed Nov 17, 2021

View reviewed changes

Zyiqin-Miranda force-pushed the cloudwatch branch 2 times, most recently from ed9deab to e98c50e Compare November 24, 2021 21:39

pdames previously requested changes Nov 24, 2021

View reviewed changes

pdames reviewed Nov 24, 2021

View reviewed changes

python/ray/autoscaler/aws/cloudwatch/example-cloudwatch-alarm-config.json Outdated Show resolved Hide resolved

Zyiqin-Miranda force-pushed the cloudwatch branch 2 times, most recently from 63fd476 to cbe5f96 Compare December 3, 2021 01:13

Zyiqin-Miranda requested a review from pdames December 3, 2021 01:18

Zyiqin-Miranda force-pushed the cloudwatch branch from cbe5f96 to 88ee45d Compare December 6, 2021 08:08

Zyiqin-Miranda force-pushed the cloudwatch branch 4 times, most recently from 36056fe to b32469a Compare December 13, 2021 02:48

Zyiqin-Miranda added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Dec 13, 2021

wuisawesome reviewed Dec 14, 2021

View reviewed changes

Zyiqin-Miranda force-pushed the cloudwatch branch 2 times, most recently from fa6021e to 3486a26 Compare December 15, 2021 21:35

Zyiqin-Miranda changed the title ~~[autoscaler] AWS Autoscaler CloudWatch Dashboard and Alarm support~~ [autoscaler] AWS Autoscaler CloudWatch Dashboard support Dec 15, 2021

Zyiqin-Miranda added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed tests-ok The tagger certifies test failures are unrelated and assumes personal liability. labels Dec 15, 2021

Zyiqin-Miranda force-pushed the cloudwatch branch 2 times, most recently from c1d494a to edc823a Compare December 16, 2021 18:45

Zyiqin-Miranda requested a review from wuisawesome December 16, 2021 18:47

Zyiqin-Miranda closed this Dec 17, 2021

Zyiqin-Miranda deleted the cloudwatch branch December 17, 2021 05:27

Zyiqin-Miranda restored the cloudwatch branch December 17, 2021 05:32

Zyiqin-Miranda reopened this Dec 17, 2021

Zyiqin-Miranda force-pushed the cloudwatch branch 2 times, most recently from 0714ece to 948ddaf Compare December 20, 2021 19:19

wuisawesome reviewed Jan 7, 2022

View reviewed changes

[autoscaler] AWS Autoscaler CloudWatch Dashboard/Alarm support

5d02400

Zyiqin-Miranda force-pushed the cloudwatch branch from 948ddaf to 5d02400 Compare January 7, 2022 07:30

pdames approved these changes Jan 7, 2022

View reviewed changes

wuisawesome approved these changes Jan 10, 2022

View reviewed changes

DmitriGekhtman approved these changes Jan 10, 2022

View reviewed changes

wuisawesome merged commit 71fae21 into ray-project:master Jan 10, 2022

Zyiqin-Miranda mentioned this pull request Jan 11, 2022

[autoscaler] Add AWS Autoscaler CloudWatch Alarm support #21523

Merged

6 tasks

Zyiqin-Miranda mentioned this pull request Feb 3, 2022

[autoscaler] Consolidate CloudWatch agent/dashboard/alarm support; Add unit tests for AWS autoscaler CloudWatch integration #22070

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[autoscaler] AWS Autoscaler CloudWatch Dashboard support #20266

[autoscaler] AWS Autoscaler CloudWatch Dashboard support #20266

Zyiqin-Miranda commented Nov 11, 2021 •

edited

Loading

DmitriGekhtman commented Nov 12, 2021

pdames left a comment

Zyiqin-Miranda commented Dec 3, 2021 •

edited

Loading

pdames commented Dec 9, 2021

DmitriGekhtman commented Dec 9, 2021

wuisawesome left a comment

Zyiqin-Miranda commented Dec 14, 2021 •

edited

Loading

pdames commented Dec 15, 2021

wuisawesome commented Jan 6, 2022

wuisawesome Dec 20, 2021

Zyiqin-Miranda Jan 7, 2022

wuisawesome Dec 20, 2021

Zyiqin-Miranda Jan 7, 2022

wuisawesome Dec 20, 2021

Zyiqin-Miranda Jan 7, 2022

Zyiqin-Miranda Jan 7, 2022

wuisawesome Dec 20, 2021

Zyiqin-Miranda Jan 7, 2022

wuisawesome commented Jan 10, 2022

[autoscaler] AWS Autoscaler CloudWatch Dashboard support #20266

[autoscaler] AWS Autoscaler CloudWatch Dashboard support #20266

Conversation

Zyiqin-Miranda commented Nov 11, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

DmitriGekhtman commented Nov 12, 2021

pdames left a comment

Choose a reason for hiding this comment

Zyiqin-Miranda commented Dec 3, 2021 • edited Loading

pdames commented Dec 9, 2021

DmitriGekhtman commented Dec 9, 2021

wuisawesome left a comment

Choose a reason for hiding this comment

Zyiqin-Miranda commented Dec 14, 2021 • edited Loading

pdames commented Dec 15, 2021

wuisawesome commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuisawesome commented Jan 10, 2022

Zyiqin-Miranda commented Nov 11, 2021 •

edited

Loading

Zyiqin-Miranda commented Dec 3, 2021 •

edited

Loading

Zyiqin-Miranda commented Dec 14, 2021 •

edited

Loading