Skip to content

feat(deployment): add sequencer readiness check stage to system test workflow #6641

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nadin-Starkware
Copy link
Collaborator

No description provided.

@nadin-Starkware nadin-Starkware marked this pull request as ready for review May 21, 2025 13:10
@reviewable-StarkWare
Copy link

This change is Reviewable

@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from faa9de2 to 6755a59 Compare May 21, 2025 13:16
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from 6755a59 to 6dc5a42 Compare May 21, 2025 13:40
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch from 6683d74 to 5ec500c Compare May 21, 2025 13:40
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch 2 times, most recently from 5bd0442 to 85b2699 Compare May 21, 2025 13:47
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch 2 times, most recently from e72c8d6 to b97bdd9 Compare May 21, 2025 14:11
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch 2 times, most recently from c251335 to 701000e Compare May 21, 2025 14:13
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch 2 times, most recently from fb8573a to e113bb8 Compare May 21, 2025 14:17
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from 701000e to 55ef9f1 Compare May 21, 2025 14:17
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion


scripts/system_tests/readiness_check.py line 43 at r2 (raw file):

def wait_for_services_ready(deployment_config_path: str, namespace: str):
    config.load_kube_config()  # Or load_incluster_config() if inside a pod

?

Code quote:

# Or load_incluster_config() if inside a pod

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @nadin-Starkware)


scripts/system_tests/readiness_check.py line 56 at r2 (raw file):

        controller = service["controller"]
        service_name_lower = service_name.lower()
        controller_lower = controller.lower()

This chunk is repeated, maybe create a fn?

Code quote:

        service_name = service["name"]
        controller = service["controller"]
        service_name_lower = service_name.lower()
        controller_lower = controller.lower()

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @nadin-Starkware)


scripts/system_tests/readiness_check.py line 72 at r2 (raw file):

            else:
                print(f"❌ Unknown controller: {controller}. Skipping...")
                continue

Is this not a reason to fail the test?

Code quote:

                print(f"❌ Unknown controller: {controller}. Skipping...")
                continue

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @nadin-Starkware)


scripts/system_tests/readiness_check.py line 78 at r2 (raw file):

                continue
            else:
                raise

What does this do?

Code quote:

        except ApiException as e:
            if e.status == 404:
                print(f"❌ {controller} {resource_name} not found. Skipping...")
                continue
            else:
                raise

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @nadin-Starkware)


scripts/system_tests/readiness_check.py line 104 at r2 (raw file):

                    ).status
                    ready = status.ready_replicas or 0
                    desired = status.replicas or 0

Please use "else if", and fail on the 3rd default branch

Code quote:

                else:  # deployment
                    status = apps_v1.read_namespaced_deployment_status(
                        resource_name, namespace
                    ).status
                    ready = status.ready_replicas or 0
                    desired = status.replicas or 0

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @nadin-Starkware)


scripts/system_tests/readiness_check.py line 131 at r2 (raw file):

        "--namespace",
        help="Kubernetes namespace",
    )

Mark as required

Code quote:

    parser.add_argument(
        "--namespace",
        help="Kubernetes namespace",
    )

Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @nadin-Starkware)


scripts/system_tests/readiness_check.py line 116 at r2 (raw file):

        else:
            print(
                f"⚠️ Timeout waiting for {controller} {resource_name} to become ready."

This is an error, right?

Code quote:

⚠️

@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch from e113bb8 to cd1f7ae Compare May 22, 2025 06:55
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from 55ef9f1 to 6399f64 Compare May 22, 2025 06:55
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch from cd1f7ae to 7c7f006 Compare May 22, 2025 07:04
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch 2 times, most recently from ae841e3 to f8a2b9f Compare May 22, 2025 08:06
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch 2 times, most recently from 53a42cb to e20b74b Compare May 22, 2025 08:12
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from f8a2b9f to 3a68e16 Compare May 22, 2025 08:12
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch from e20b74b to f365ce3 Compare May 22, 2025 08:14
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch 2 times, most recently from 711b52f to f89200a Compare May 22, 2025 08:20
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch 2 times, most recently from aa5feaf to 1632d57 Compare May 22, 2025 08:21
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch 2 times, most recently from 04e7fc7 to 8d3ec34 Compare May 22, 2025 08:31
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch from 1632d57 to d089e8e Compare May 22, 2025 08:31
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from 8d3ec34 to d81b38e Compare May 22, 2025 08:31
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch 2 times, most recently from 19acdf0 to 787c114 Compare May 22, 2025 10:02
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from d81b38e to 57d2097 Compare May 22, 2025 10:02
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch from 787c114 to 0a37708 Compare May 22, 2025 10:10
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from 57d2097 to daefad8 Compare May 22, 2025 10:11
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow branch from 0a37708 to 8a409b7 Compare May 22, 2025 10:11
@nadin-Starkware nadin-Starkware changed the base branch from 05-21-feat_deployment_add_sequencer_deployment_stage_to_system_test_workflow to graphite-base/6641 May 25, 2025 07:23
@nadin-Starkware nadin-Starkware force-pushed the 05-21-feat_deployment_add_sequencer_readiness_check_stage_to_system_test_workflow branch from daefad8 to 569333f Compare May 25, 2025 07:23
@nadin-Starkware nadin-Starkware changed the base branch from graphite-base/6641 to main May 25, 2025 07:23
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.

3 participants