Added the running rosa spots ability #347

athiruma · 2023-08-01T05:35:29Z

Type of change

Description

Span the Rosa worker nodes with the help of machinepools by creating the default cluster with 3(M+W+I).

Related Tickets & Documents

Reference document: Link

Related Issue #
Closes #

Checklist before requesting a review

I have performed a self-review of my code.
If it is a core feature, I have added thorough tests.

Testing

Please describe the System Under Test.
Please provide detailed steps to perform tests related to this code change.
- Add the variable enable_spot_workers=true in the install config file of rosa.
How were the fix/results from this change verified? Please provide relevant screenshots or results.
- Ran the airflow cluster by creating a playground on the sailplane cluster.

krishvoor · 2023-08-01T06:57:52Z

@athiruma Can you share the Airflow DAG for review?

athiruma · 2023-08-01T07:00:04Z

@athiruma Can you share the Airflow DAG for review?

yes, it is attached to the word playground in the above description.

krishvoor

Minor suggestions, overall lgtm

krishvoor · 2023-08-01T06:58:53Z

dags/openshift_nightlies/tasks/install/rosa/defaults.json

@@ -41,5 +41,6 @@
    "staging_mgmt_provisioner_shards": "",
    "aws_region": "us-west-2",
    "oidc_config": "",
-    "extra_machinepool": []
+    "extra_machinepool": [],
+    "enable_spot_workers": false


Please add:
"openshift_workload_node_instance_type": null

Default , there is no variable with that name

You can declare a new variable with empty/null/default values in defaults.json file

krishvoor · 2023-08-01T06:59:54Z

dags/openshift_nightlies/config/install/rosa/ovn-small.json

+    "openshift_workload_node_instance_type": null,
+    "enable_spot_workers": false


Instead of modifying the existing json, please create a new json.

Add a default instance_type to openshift_workload_node_instance_type

I didn't get this

Please update the following:

"openshift_workload_node_instance_type": m5.2xlarge, "enable_spot_workers": false

dags/openshift_nightlies/scripts/install/rosa.sh

krishvoor · 2023-08-01T07:03:51Z

@athiruma Can you share the Airflow DAG for review?

yes, it is attached to the word playground in the above description.

Ahh, I see it now thank you!

krishvoor · 2023-08-01T10:05:01Z

dags/openshift_nightlies/config/install/rosa/ovn-small.json

@@ -12,5 +12,6 @@
    "openshift_network_type": "OVNKubernetes",
    "openshift_worker_instance_type": "m5.2xlarge",
    "machineset_metadata_label_prefix": "machine.openshift.io",
-    "openshift_workload_node_instance_type": null
+    "openshift_workload_node_instance_type": "m5.2xlarge",
+    "enable_spot_workers": false


Please create a new file: dags/openshift_nightlies/config/install/rosa/ovn-small-spot.json
And move the key/values to it.

if it is the case, I need to add this file to the manifest.yml to read properties of this file.

if it is the case, I need to add this file to the manifest.yml to read properties of this file.

Please update accordingly

Signed-off-by: Thirumalesh Aaraveti <[email protected]>

mukrishn

Need a code rebase because of this cleanup

mukrishn · 2023-08-02T13:43:07Z

dags/openshift_nightlies/scripts/install/rosa.sh

-        rosa create cluster --tags=User:${GITHUB_USERNAME} --cluster-name ${CLUSTER_NAME} --version "${ROSA_VERSION}" --channel-group=${MANAGED_CHANNEL_GROUP} --compute-machine-type ${COMPUTE_WORKERS_TYPE} --replicas ${COMPUTE_WORKERS_NUMBER} --network-type ${NETWORK_TYPE} ${INSTALLATION_PARAMS} ${ROSA_HCP_PARAMS}
+        if [ "$ENABLE_SPOT_WORKERS" == "true" ]; then
+            rosa create cluster --tags=User:${GITHUB_USERNAME} --cluster-name ${CLUSTER_NAME} --version "${ROSA_VERSION}" --channel-group=${MANAGED_CHANNEL_GROUP} --compute-machine-type ${COMPUTE_WORKERS_TYPE} --network-type ${NETWORK_TYPE} ${INSTALLATION_PARAMS} ${ROSA_HCP_PARAMS}
+            _wait_for_cluster_ready ${CLUSTER_NAME}


This will also check for node_ready, won't it fail here if it is not at desired size ?

oh, yes right. Will add the checks there.

mukrishn · 2023-08-02T13:45:07Z

dags/openshift_nightlies/scripts/install/rosa.sh

+        if [ "$ENABLE_SPOT_WORKERS" == "true" ]; then
+            rosa create cluster --tags=User:${GITHUB_USERNAME} --cluster-name ${CLUSTER_NAME} --version "${ROSA_VERSION}" --channel-group=${MANAGED_CHANNEL_GROUP} --compute-machine-type ${COMPUTE_WORKERS_TYPE} --network-type ${NETWORK_TYPE} ${INSTALLATION_PARAMS} ${ROSA_HCP_PARAMS}
+            _wait_for_cluster_ready ${CLUSTER_NAME}
+            rosa create machinepool -c ${CLUSTER_NAME} --name="${CLUSTER_NAME}-spot-pool" --replicas=${COMPUTE_WORKERS_NUMBER} --instance-type="${COMPUTE_WORKERS_TYPE}" --labels="rosa-spots=true" --use-spot-instances


Just Curious, why don't we create replicas $((COMPUTE_WORKERS_NUMBER-3)) to keep it user requested size, but by doing that would it affect performance result as it may spread the load across both spot as well as non-spot instances ?
I guess we are using same instance type for both machinepools, it should be fine right ?

We thought of having total spot instances instead of on-demand.

krishvoor · 2023-08-01T10:41:23Z

dags/openshift_nightlies/manifest.yaml

+      - name: sts-ovn-small-spot-cp
+        schedule: "0 12 * * 3"
+        config:
+          install: rosa/ovb-small-spot.json


This should be:
install: rosa/ovb-small-spot.json --> install: rosa/ovn-small-spot.json

codecov-commenter · 2023-08-07T11:25:21Z

Codecov Report

Merging #347 (69506c3) into master (967454d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #347   +/-   ##
=======================================
  Coverage   79.06%   79.06%           
=======================================
  Files          22       22           
  Lines        1003     1003           
=======================================
  Hits          793      793           
  Misses        210      210

Flag	Coverage Δ
gha	`79.06% <ø> (ø)`
python-3.9	`79.06% <ø> (ø)`
unit	`79.06% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

krishvoor requested review from krishvoor and mukrishn August 1, 2023 05:38

krishvoor reviewed Aug 1, 2023

View reviewed changes

athiruma force-pushed the rosa-spots branch 3 times, most recently from dc7f0d8 to b1e02fe Compare August 1, 2023 09:50

krishvoor reviewed Aug 1, 2023

View reviewed changes

athiruma force-pushed the rosa-spots branch 3 times, most recently from c784a5b to a0e3a1b Compare August 1, 2023 10:39

Added the running rosa spots ability

e88dff5

Signed-off-by: Thirumalesh Aaraveti <[email protected]>

athiruma force-pushed the rosa-spots branch from a0e3a1b to e88dff5 Compare August 1, 2023 10:40

mukrishn reviewed Aug 2, 2023

View reviewed changes

krishvoor requested changes Aug 7, 2023

View reviewed changes

Merge branch 'master' into rosa-spots

69506c3

athiruma closed this Aug 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the running rosa spots ability #347

Added the running rosa spots ability #347

athiruma commented Aug 1, 2023 •

edited

Loading

krishvoor commented Aug 1, 2023

athiruma commented Aug 1, 2023

krishvoor left a comment

krishvoor Aug 1, 2023 •

edited

Loading

athiruma Aug 1, 2023

krishvoor Aug 1, 2023 •

edited

Loading

athiruma Aug 1, 2023

krishvoor Aug 1, 2023

krishvoor Aug 1, 2023 •

edited

Loading

athiruma Aug 1, 2023

krishvoor Aug 1, 2023

krishvoor commented Aug 1, 2023

krishvoor Aug 1, 2023

athiruma Aug 1, 2023

krishvoor Aug 1, 2023

athiruma Aug 1, 2023

mukrishn left a comment

mukrishn Aug 2, 2023

athiruma Aug 3, 2023

mukrishn Aug 2, 2023

athiruma Aug 3, 2023

krishvoor Aug 1, 2023

codecov-commenter commented Aug 7, 2023

		"openshift_workload_node_instance_type": null,
		"enable_spot_workers": false

Added the running rosa spots ability #347

Added the running rosa spots ability #347

Conversation

athiruma commented Aug 1, 2023 • edited Loading

Type of change

Description

Related Tickets & Documents

Checklist before requesting a review

Testing

krishvoor commented Aug 1, 2023

athiruma commented Aug 1, 2023

krishvoor left a comment

Choose a reason for hiding this comment

krishvoor Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishvoor Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishvoor Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishvoor commented Aug 1, 2023

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

mukrishn left a 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

codecov-commenter commented Aug 7, 2023

Codecov Report

athiruma commented Aug 1, 2023 •

edited

Loading

krishvoor Aug 1, 2023 •

edited

Loading

krishvoor Aug 1, 2023 •

edited

Loading

krishvoor Aug 1, 2023 •

edited

Loading